Sun, Nov 06, 2022 at 10:09:42AM CET, idosch@xxxxxxxxxx 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.
