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

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: <Ybnz2agUcwHE8hRH@kroah.com>
Date:   Wed, 15 Dec 2021 14:55:37 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Antoine Tenart <atenart@...nel.org>
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
        stable@...r.kernel.org
Subject: Re: [PATCH 5.10] ethtool: do not perform operations on net devices
 being unregistered

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@...nel.org>
> Signed-off-by: Antoine Tenart <atenart@...nel.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