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: <20210709124340.44bafef1@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Fri, 9 Jul 2021 12:43:40 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Cc:     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@...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,  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@...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);
>  }