| Message ID | 20210709025525.107314-1-xuanzhuo@linux.alibaba.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net,v2] xdp, net: fix use-after-free in bpf_xdp_link_release | expand |
On Fri, 9 Jul 2021 14:56:26 -0700 Andrii Nakryiko wrote: > On Fri, Jul 9, 2021 at 12:43 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Fri, 9 Jul 2021 10:55:25 +0800 Xuan Zhuo wrote: > > > The problem occurs between dev_get_by_index() and dev_xdp_attach_link(). > > > At this point, dev_xdp_uninstall() is called. Then xdp link will not be > > > detached automatically when dev is released. But link->dev already > > > points to dev, when xdp link is released, dev will still be accessed, > > > but dev has been released. > > > > > > dev_get_by_index() | > > > link->dev = dev | > > > | rtnl_lock() > > > | unregister_netdevice_many() > > > | dev_xdp_uninstall() > > > | rtnl_unlock() > > > rtnl_lock(); | > > > dev_xdp_attach_link() | > > > rtnl_unlock(); | > > > | netdev_run_todo() // dev released > > > bpf_xdp_link_release() | > > > /* access dev. | > > > use-after-free */ | > > > > > > This patch adds a check of dev->reg_state in dev_xdp_attach_link(). If > > > dev has been called release, it will return -EINVAL. > > > > Please make sure to include a Fixes tag. > > > > I must say I prefer something closet to v1. Maybe put the if > > in the callee? Making ndo calls to unregistered netdevs is > > not legit, it will be confusing for a person reading this > > code to have to search callees to find where unregistered > > netdevs are rejected. > > So I'm a bit confused about the intended use of dev_get_by_index(). It > doesn't seem to be checking that device is unregistered and happily > returns dev with refcnt bumped even though device is going away. Is it > the intention that every caller of dev_get_by_index() needs to check > the state of the device *and* do any subsequent actions under the same > rtnl_lock/rtnl_unlock region? Seems a bit fragile. It depends on the caller, right? Not all callers even take the rtnl lock. AFAIU dev_get_by_index() gives the caller a ref'ed netdev object. If all the caller cares about is the netdev state itself that's perfectly fine. If caller has ordering requirements or needs to talk to the driver chances are the lookup and all checks should be done under rtnl. Or there must be some lock dependency on rtnl (take a lock which unregister netdev of the device of interest would also take). In case of XDP we impose extra requirements on ourselves because we want the driver code to be as simple as possible. > I suspect doing this state check inside dev_get_by_index() would have > unintended consequences, though, right? It'd be moot, dev_get_by_index() is under RCU and unregister path syncs RCU, but that doesn't guarantee anything if caller holds no locks. > BTW, seems like netlink code doesn't check the state of the device and > will report successful attachment to the dev that's unregistered? Is > this something we should fix as well? Entire rtnetlink is under rtnl_lock, and so is unregistering a netdev so those paths can't race. > Xuan, if we do go with this approach, that dev->reg_state check should > probably be done in dev_xdp_attach() instead, which is called for both > bpf_link-based and bpf_prog-based XDP attachment. > > If not, then the cleanest solution would be to make this check right > before dev_xdp_attach_link (though it's not clear what are we gaining > with that, if we ever have another user of dev_xdp_attach_link beside > bpf_xdp_link_attach, we'll probably miss similar situation), instead > of spreading out rtnl_unlock. > > BTW, regardless of the approach, we still need to do link->dev = NULL > if dev_xdp_attach_link() errors out.
On Fri, Jul 9, 2021 at 5:20 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 9 Jul 2021 14:56:26 -0700 Andrii Nakryiko wrote: > > On Fri, Jul 9, 2021 at 12:43 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > > > On Fri, 9 Jul 2021 10:55:25 +0800 Xuan Zhuo wrote: > > > > The problem occurs between dev_get_by_index() and dev_xdp_attach_link(). > > > > At this point, dev_xdp_uninstall() is called. Then xdp link will not be > > > > detached automatically when dev is released. But link->dev already > > > > points to dev, when xdp link is released, dev will still be accessed, > > > > but dev has been released. > > > > > > > > dev_get_by_index() | > > > > link->dev = dev | > > > > | rtnl_lock() > > > > | unregister_netdevice_many() > > > > | dev_xdp_uninstall() > > > > | rtnl_unlock() > > > > rtnl_lock(); | > > > > dev_xdp_attach_link() | > > > > rtnl_unlock(); | > > > > | netdev_run_todo() // dev released > > > > bpf_xdp_link_release() | > > > > /* access dev. | > > > > use-after-free */ | > > > > > > > > This patch adds a check of dev->reg_state in dev_xdp_attach_link(). If > > > > dev has been called release, it will return -EINVAL. > > > > > > Please make sure to include a Fixes tag. > > > > > > I must say I prefer something closet to v1. Maybe put the if > > > in the callee? Making ndo calls to unregistered netdevs is > > > not legit, it will be confusing for a person reading this > > > code to have to search callees to find where unregistered > > > netdevs are rejected. > > > > So I'm a bit confused about the intended use of dev_get_by_index(). It > > doesn't seem to be checking that device is unregistered and happily > > returns dev with refcnt bumped even though device is going away. Is it > > the intention that every caller of dev_get_by_index() needs to check > > the state of the device *and* do any subsequent actions under the same > > rtnl_lock/rtnl_unlock region? Seems a bit fragile. > > It depends on the caller, right? Not all callers even take the rtnl > lock. AFAIU dev_get_by_index() gives the caller a ref'ed netdev object. > If all the caller cares about is the netdev state itself that's > perfectly fine. > > If caller has ordering requirements or needs to talk to the driver > chances are the lookup and all checks should be done under rtnl. > Or there must be some lock dependency on rtnl (take a lock which > unregister netdev of the device of interest would also take). > > In case of XDP we impose extra requirements on ourselves because we > want the driver code to be as simple as possible. > > > I suspect doing this state check inside dev_get_by_index() would have > > unintended consequences, though, right? > > It'd be moot, dev_get_by_index() is under RCU and unregister path syncs > RCU, but that doesn't guarantee anything if caller holds no locks. Yep. As Xuan also mentioned, if dev_get_by_index and attach happens under the same lock then we can't really get dev that's unregistered. Ok, all makes sense, thanks for explaining. > > > BTW, seems like netlink code doesn't check the state of the device and > > will report successful attachment to the dev that's unregistered? Is > > this something we should fix as well? > > Entire rtnetlink is under rtnl_lock, and so is unregistering a netdev > so those paths can't race. > > > Xuan, if we do go with this approach, that dev->reg_state check should > > probably be done in dev_xdp_attach() instead, which is called for both > > bpf_link-based and bpf_prog-based XDP attachment. > > > > If not, then the cleanest solution would be to make this check right > > before dev_xdp_attach_link (though it's not clear what are we gaining > > with that, if we ever have another user of dev_xdp_attach_link beside > > bpf_xdp_link_attach, we'll probably miss similar situation), instead > > of spreading out rtnl_unlock. > > > > BTW, regardless of the approach, we still need to do link->dev = NULL > > if dev_xdp_attach_link() errors out.
diff --git a/net/core/dev.c b/net/core/dev.c index c253c2aafe97..63c9a46ca853 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -9544,6 +9544,10 @@ static int dev_xdp_attach_link(struct net_device *dev, struct netlink_ext_ack *extack, struct bpf_xdp_link *link) { + /* ensure the dev state is ok */ + if (dev->reg_state != NETREG_REGISTERED) + return -EINVAL; + return dev_xdp_attach(dev, extack, link, NULL, NULL, link->flags); }
The problem occurs between dev_get_by_index() and dev_xdp_attach_link(). At this point, dev_xdp_uninstall() is called. Then xdp link will not be detached automatically when dev is released. But link->dev already points to dev, when xdp link is released, dev will still be accessed, but dev has been released. dev_get_by_index() | link->dev = dev | | rtnl_lock() | unregister_netdevice_many() | dev_xdp_uninstall() | rtnl_unlock() rtnl_lock(); | dev_xdp_attach_link() | rtnl_unlock(); | | netdev_run_todo() // dev released bpf_xdp_link_release() | /* access dev. | use-after-free */ | This patch adds a check of dev->reg_state in dev_xdp_attach_link(). If dev has been called release, it will return -EINVAL. [ 45.966867] BUG: KASAN: use-after-free in bpf_xdp_link_release+0x3b8/0x3d0 [ 45.967619] Read of size 8 at addr ffff00000f9980c8 by task a.out/732 [ 45.968297] [ 45.968502] CPU: 1 PID: 732 Comm: a.out Not tainted 5.13.0+ #22 [ 45.969222] Hardware name: linux,dummy-virt (DT) [ 45.969795] Call trace: [ 45.970106] dump_backtrace+0x0/0x4c8 [ 45.970564] show_stack+0x30/0x40 [ 45.970981] dump_stack_lvl+0x120/0x18c [ 45.971470] print_address_description.constprop.0+0x74/0x30c [ 45.972182] kasan_report+0x1e8/0x200 [ 45.972659] __asan_report_load8_noabort+0x2c/0x50 [ 45.973273] bpf_xdp_link_release+0x3b8/0x3d0 [ 45.973834] bpf_link_free+0xd0/0x188 [ 45.974315] bpf_link_put+0x1d0/0x218 [ 45.974790] bpf_link_release+0x3c/0x58 [ 45.975291] __fput+0x20c/0x7e8 [ 45.975706] ____fput+0x24/0x30 [ 45.976117] task_work_run+0x104/0x258 [ 45.976609] do_notify_resume+0x894/0xaf8 [ 45.977121] work_pending+0xc/0x328 [ 45.977575] [ 45.977775] The buggy address belongs to the page: [ 45.978369] page:fffffc00003e6600 refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x4f998 [ 45.979522] flags: 0x7fffe0000000000(node=0|zone=0|lastcpupid=0x3ffff) [ 45.980349] raw: 07fffe0000000000 fffffc00003e6708 ffff0000dac3c010 0000000000000000 [ 45.981309] raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000 [ 45.982259] page dumped because: kasan: bad access detected [ 45.982948] [ 45.983153] Memory state around the buggy address: [ 45.983753] ffff00000f997f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc [ 45.984645] ffff00000f998000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff [ 45.985533] >ffff00000f998080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff [ 45.986419] ^ [ 45.987112] ffff00000f998100: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff [ 45.988006] ffff00000f998180: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff [ 45.988895] ================================================================== [ 45.989773] Disabling lock debugging due to kernel taint [ 45.990552] Kernel panic - not syncing: panic_on_warn set ... [ 45.991166] CPU: 1 PID: 732 Comm: a.out Tainted: G B 5.13.0+ #22 [ 45.991929] Hardware name: linux,dummy-virt (DT) [ 45.992448] Call trace: [ 45.992753] dump_backtrace+0x0/0x4c8 [ 45.993208] show_stack+0x30/0x40 [ 45.993627] dump_stack_lvl+0x120/0x18c [ 45.994113] dump_stack+0x1c/0x34 [ 45.994530] panic+0x3a4/0x7d8 [ 45.994930] end_report+0x194/0x198 [ 45.995380] kasan_report+0x134/0x200 [ 45.995850] __asan_report_load8_noabort+0x2c/0x50 [ 45.996453] bpf_xdp_link_release+0x3b8/0x3d0 [ 45.997007] bpf_link_free+0xd0/0x188 [ 45.997474] bpf_link_put+0x1d0/0x218 [ 45.997942] bpf_link_release+0x3c/0x58 [ 45.998429] __fput+0x20c/0x7e8 [ 45.998833] ____fput+0x24/0x30 [ 45.999247] task_work_run+0x104/0x258 [ 45.999731] do_notify_resume+0x894/0xaf8 [ 46.000236] work_pending+0xc/0x328 [ 46.000697] SMP: stopping secondary CPUs [ 46.001226] Dumping ftrace buffer: [ 46.001663] (ftrace buffer empty) [ 46.002110] Kernel Offset: disabled [ 46.002545] CPU features: 0x00000001,23202c00 [ 46.003080] Memory Limit: none Reported-by: Abaci <abaci@linux.alibaba.com> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Reviewed-by: Dust Li <dust.li@linux.alibaba.com> --- v2: return err when dev was removed. net/core/dev.c | 4 ++++ 1 file changed, 4 insertions(+) -- 2.31.0