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@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> Reviewed-by: Dust Li <dust.li@xxxxxxxxxxxxxxxxx>
> 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);
> }
