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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzYuBYE1np+YwpwZT5_K5Zzed1NTPz57zbgn+0V5W1=nZg@mail.gmail.com>
Date:   Fri, 9 Jul 2021 14:56:26 -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 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. 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@...ux.alibaba.com>
> > Signed-off-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
> > Reviewed-by: Dust Li <dust.li@...ux.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);
> >  }