netdev - Re: [PATCH net v2] xdp, net: fix use-after-free in bpf_xdp_link

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAEf4BzZLa75jN-+GOpCM7Ma75iHkn_cfer8Vtirpx+nGKeFtXA@mail.gmail.com>
Date:   Fri, 9 Jul 2021 18:23:18 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
        Networking <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        KP Singh <kpsingh@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Antoine Tenart <atenart@...nel.org>,
        Alexander Lobakin <alobakin@...me>,
        Wei Wang <weiwan@...gle.com>, Taehee Yoo <ap420073@...il.com>,
        bpf <bpf@...r.kernel.org>, Abaci <abaci@...ux.alibaba.com>,
        Dust Li <dust.li@...ux.alibaba.com>
Subject: Re: [PATCH net v2] xdp, net: fix use-after-free in bpf_xdp_link_release

On Fri, Jul 9, 2021 at 5:20 PM Jakub Kicinski <kuba@...nel.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@...nel.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.