netdev - Re: [patch net-next v4 05/13] net: devlink: track netdev with devlink

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: <Y2d51izTZV1rThOc@shredder>
Date:   Sun, 6 Nov 2022 11:09:42 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        pabeni@...hat.com, edumazet@...gle.com, tariqt@...dia.com,
        moshe@...dia.com, saeedm@...dia.com, linux-rdma@...r.kernel.org
Subject: Re: [patch net-next v4 05/13] net: devlink: track netdev with
 devlink_port assigned

On Wed, Nov 02, 2022 at 05:02:03PM +0100, Jiri Pirko wrote:
> @@ -9645,10 +9649,13 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
>  
>  	ret = xa_alloc_cyclic(&devlinks, &devlink->index, devlink, xa_limit_31b,
>  			      &last_id, GFP_KERNEL);
> -	if (ret < 0) {
> -		kfree(devlink);
> -		return NULL;
> -	}
> +	if (ret < 0)
> +		goto err_xa_alloc;
> +
> +	devlink->netdevice_nb.notifier_call = devlink_netdevice_event;
> +	ret = register_netdevice_notifier_net(net, &devlink->netdevice_nb);
> +	if (ret)
> +		goto err_register_netdevice_notifier;
>  
>  	devlink->dev = dev;
>  	devlink->ops = ops;
> @@ -9675,6 +9682,12 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
>  	init_completion(&devlink->comp);
>  
>  	return devlink;
> +
> +err_register_netdevice_notifier:
> +	xa_erase(&devlinks, devlink->index);
> +err_xa_alloc:
> +	kfree(devlink);
> +	return NULL;
>  }
>  EXPORT_SYMBOL_GPL(devlink_alloc_ns);
>  
> @@ -9828,6 +9841,10 @@ void devlink_free(struct devlink *devlink)
>  	WARN_ON(!list_empty(&devlink->port_list));
>  
>  	xa_destroy(&devlink->snapshot_ids);
> +
> +	unregister_netdevice_notifier_net(devlink_net(devlink),
> +					  &devlink->netdevice_nb);
> +
>  	xa_erase(&devlinks, devlink->index);
>  
>  	kfree(devlink);

The network namespace of the devlink instance can change throughout the
lifetime of the devlink instance, but the notifier block is always
registered in the initial namespace. This leads to
unregister_netdevice_notifier_net() failing to unregister the notifier
block, which leads to use-after-free. Reproduce (with KASAN enabled):

# echo "10 0" > /sys/bus/netdevsim/new_device
# ip netns add bla
# devlink dev reload netdevsim/netdevsim10 netns bla
# echo 10 > /sys/bus/netdevsim/del_device
# ip link add dummy10 up type dummy

I see two possible solutions:

1. Use register_netdevice_notifier() instead of
register_netdevice_notifier_net().

2. Move the notifier block to the correct namespace in devlink_reload().