| Message ID | 20210709025525.107314-1-xuanzhuo@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [net,v2] xdp, net: fix use-after-free in bpf_xdp_link_release | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | fail | Series targets non-next tree, but doesn't contain any Fixes tags |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 18 of 18 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 6 this patch: 6 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 10 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 6 this patch: 6 |
| netdev/header_inline | success | Link |
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. > 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> > 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); > }
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. I suspect doing this state check inside dev_get_by_index() would have unintended consequences, though, right? 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? 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. > > > 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> > > > 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); > > }
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:35 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > On Fri, 9 Jul 2021 14:56:26 -0700, Andrii Nakryiko <andrii.nakryiko@gmail.com> 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. I suspect doing > > this state check inside dev_get_by_index() would have unintended > > consequences, though, right? > > In the function unregister_netdevice_many(), dev will be deleted from the linked > list, so after this, dev_get_by_index() will not return dev. If it is not in > rtnl_lock, subsequent use of dev is to check reg_state. > Ah, I see, makes sense, if we do dev lookup and attachment under the same lock then we either won't get the device or at the time of attachment it will be valid. > So I think, maybe the version of v1 does not have the problem you mentioned. > After calling rtnl_lock, we get dev from dev_get_by_index(). If it succeeds, we > execute the following process, and if it fails, we return an error directly. > > > > > > 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? > > There is no such problem here, because all netlink operations are protected by > rtnl_lock. In the protection of rtnl_lock, it is completely safe to get dev and > attach link or prog. > Ok, I see, one big rtnl_lock saves netlink :) > > > > > 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. > > As mentioned above, since the entire bpf prog operation is protected by > rtnl_lock, dev_xdp_attach() does not need to check the status of dev. > > > > > 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. > > I think I understand what you mean now. Yeah, this is a problem regardless. Btw, I was also thinking to move dev_get_by_index right before dev_xdp_attach_link inside a tight rntl_lock/rtnl_unlock region after bpf_link is allocated, but that seems pretty bad if user, intentionally or not, passes wrong ifindex. We'll be allocated a bunch of unnecessary memory and deferring freeing it for no good reason. So let's go with your v1 and link->dev = NULL to cover the clean up bug. Thanks! > > Thanks. > > > > > > > > > > > > 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> > > > > > > > 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); > > > > }
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