[PATCH 5.10] ethtool: do not perform operations on net devices being unregistered

[PATCH 5.10] ethtool: do not perform operations on net devices being unregistered

[Antoine Tenart]

commit dde91ccfa25fd58f64c397d91b81a4b393100ffa upstream

There is a short period between a net device starts to be unregistered
and when it is actually gone. In that time frame ethtool operations
could still be performed, which might end up in unwanted or undefined
behaviours[1].

Do not allow ethtool operations after a net device starts its
unregistration. This patch targets the netlink part as the ioctl one
isn't affected: the reference to the net device is taken and the
operation is executed within an rtnl lock section and the net device
won't be found after unregister.

[1] For example adding Tx queues after unregister ends up in NULL
    pointer exceptions and UaFs, such as:

      BUG: KASAN: use-after-free in kobject_get+0x14/0x90
      Read of size 1 at addr ffff88801961248c by task ethtool/755

      CPU: 0 PID: 755 Comm: ethtool Not tainted 5.15.0-rc6+ #778
      Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-4.fc34 04/014
      Call Trace:
       dump_stack_lvl+0x57/0x72
       print_address_description.constprop.0+0x1f/0x140
       kasan_report.cold+0x7f/0x11b
       kobject_get+0x14/0x90
       kobject_add_internal+0x3d1/0x450
       kobject_init_and_add+0xba/0xf0
       netdev_queue_update_kobjects+0xcf/0x200
       netif_set_real_num_tx_queues+0xb4/0x310
       veth_set_channels+0x1c3/0x550
       ethnl_set_channels+0x524/0x610

[The patch differs from the upstream one as code was moved around by
commit 41107ac22fcf ("ethtool: move netif_device_present check from
ethnl_parse_header_dev_get to ethnl_ops_begin"). The check on the netdev
state is still done in ethnl_ops_begin as it must be done in an rtnl
section (the one which performs the op) to not race with
unregister_netdevice_many.
Also note the trace in [1] is not possible here as the channel ops for
veth were added later, but that was just one example.]

Fixes: 041b1c5d4a53 ("ethtool: helper functions for netlink interface")
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Antoine Tenart <atenart@kernel.org>
---

Hello,

This patch is intended for the stable 5.10 tree.

As reported by Greg, patch dde91ccfa25f ("ethtool: do not perform
operations on net devices being unregistered") did not apply correctly
on the 5.10 tree. The explanation of this and the approach taken here is
explained in the above commit log, between [].

I removed the Link tag and Signed-off-by from Jakub from the original
patch as this one is slightly different in its implementation.

Thanks,
Antoine

 net/ethtool/netlink.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index d8efec516d86..979dee6bb88c 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -249,6 +249,9 @@ struct ethnl_reply_data {
 
 static inline int ethnl_ops_begin(struct net_device *dev)
 {
+	if (dev && dev->reg_state == NETREG_UNREGISTERING)
+		return -ENODEV;
+
 	if (dev && dev->ethtool_ops->begin)
 		return dev->ethtool_ops->begin(dev);
 	else
-- 
2.33.1

Re: [PATCH 5.10] ethtool: do not perform operations on net devices being unregistered

[Greg KH]

On Mon, Dec 13, 2021 at 11:15:06AM +0100, Antoine Tenart wrote:
> commit dde91ccfa25fd58f64c397d91b81a4b393100ffa upstream
> 
> There is a short period between a net device starts to be unregistered
> and when it is actually gone. In that time frame ethtool operations
> could still be performed, which might end up in unwanted or undefined
> behaviours[1].
> 
> Do not allow ethtool operations after a net device starts its
> unregistration. This patch targets the netlink part as the ioctl one
> isn't affected: the reference to the net device is taken and the
> operation is executed within an rtnl lock section and the net device
> won't be found after unregister.
> 
> [1] For example adding Tx queues after unregister ends up in NULL
>     pointer exceptions and UaFs, such as:
> 
>       BUG: KASAN: use-after-free in kobject_get+0x14/0x90
>       Read of size 1 at addr ffff88801961248c by task ethtool/755
> 
>       CPU: 0 PID: 755 Comm: ethtool Not tainted 5.15.0-rc6+ #778
>       Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-4.fc34 04/014
>       Call Trace:
>        dump_stack_lvl+0x57/0x72
>        print_address_description.constprop.0+0x1f/0x140
>        kasan_report.cold+0x7f/0x11b
>        kobject_get+0x14/0x90
>        kobject_add_internal+0x3d1/0x450
>        kobject_init_and_add+0xba/0xf0
>        netdev_queue_update_kobjects+0xcf/0x200
>        netif_set_real_num_tx_queues+0xb4/0x310
>        veth_set_channels+0x1c3/0x550
>        ethnl_set_channels+0x524/0x610
> 
> [The patch differs from the upstream one as code was moved around by
> commit 41107ac22fcf ("ethtool: move netif_device_present check from
> ethnl_parse_header_dev_get to ethnl_ops_begin"). The check on the netdev
> state is still done in ethnl_ops_begin as it must be done in an rtnl
> section (the one which performs the op) to not race with
> unregister_netdevice_many.
> Also note the trace in [1] is not possible here as the channel ops for
> veth were added later, but that was just one example.]
> 
> Fixes: 041b1c5d4a53 ("ethtool: helper functions for netlink interface")
> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Antoine Tenart <atenart@kernel.org>
> ---
> 
> Hello,
> 
> This patch is intended for the stable 5.10 tree.
> 
> As reported by Greg, patch dde91ccfa25f ("ethtool: do not perform
> operations on net devices being unregistered") did not apply correctly
> on the 5.10 tree. The explanation of this and the approach taken here is
> explained in the above commit log, between [].
> 
> I removed the Link tag and Signed-off-by from Jakub from the original
> patch as this one is slightly different in its implementation.
> 
> Thanks,
> Antoine
> 
>  net/ethtool/netlink.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
> index d8efec516d86..979dee6bb88c 100644
> --- a/net/ethtool/netlink.h
> +++ b/net/ethtool/netlink.h
> @@ -249,6 +249,9 @@ struct ethnl_reply_data {
>  
>  static inline int ethnl_ops_begin(struct net_device *dev)
>  {
> +	if (dev && dev->reg_state == NETREG_UNREGISTERING)
> +		return -ENODEV;
> +
>  	if (dev && dev->ethtool_ops->begin)
>  		return dev->ethtool_ops->begin(dev);
>  	else
> -- 
> 2.33.1
> 

Now queued up, thanks.

greg k-h