netdev - Re: [PATCH net] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <07f2df6c-d7e5-9781-dae4-b0c2411c946c@linux.ibm.com>
Date:   Mon, 6 Dec 2021 11:46:35 +0200
From:   Julian Wiedmann <jwi@...ux.ibm.com>
To:     Antoine Tenart <atenart@...nel.org>, davem@...emloft.net,
        kuba@...nel.org
Cc:     alexander.duyck@...il.com, mkubecek@...e.cz, netdev@...r.kernel.org
Subject: Re: [PATCH net] ethtool: do not perform operations on net devices
 being unregistered

On 03.12.21 12:13, Antoine Tenart wrote:
> 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
> 
> Fixes: 041b1c5d4a53 ("ethtool: helper functions for netlink interface")
> Suggested-by: Jakub Kicinski <kuba@...nel.org>
> Signed-off-by: Antoine Tenart <atenart@...nel.org>
> ---
> 
> Following the discussions in those threads:
> - https://lore.kernel.org/all/20211129154520.295823-1-atenart@kernel.org/T/
> - https://lore.kernel.org/all/20211122162007.303623-1-atenart@kernel.org/T/
> 
>  net/ethtool/netlink.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
> index 38b44c0291b1..96f4180aabd2 100644
> --- a/net/ethtool/netlink.c
> +++ b/net/ethtool/netlink.c
> @@ -40,7 +40,8 @@ int ethnl_ops_begin(struct net_device *dev)
>  	if (dev->dev.parent)
>  		pm_runtime_get_sync(dev->dev.parent);
>  
> -	if (!netif_device_present(dev)) {
> +	if (!netif_device_present(dev) ||
> +	    dev->reg_state == NETREG_UNREGISTERING) {
>  		ret = -ENODEV;
>  		goto err;
>  	}
> 

Wondering if other places would also benefit from a netif_device_detach()
in the unregistration sequence ...