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] [day] [month] [year] [list]

Message-ID: <Y2i2hTt2txFoo2aR@nanopsycho>
Date:   Mon, 7 Nov 2022 08:40:53 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Ido Schimmel <idosch@...sch.org>
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

Sun, Nov 06, 2022 at 10:09:42AM CET, idosch@...sch.org wrote:
>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().

Yep, this was my intension, slipped my mind. Thanks! Will send a
follow-up.