Re: [PATCH 2/2] Drivers: hv: vmbus: Do not remove the channel when device registration fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Drivers: hv: vmbus: channel UAF after device_del() in vmbus_device_register()
- [High] Drivers: hv: vmbus: NULL channel dereference in vmbus_match() on reg failure
--

commit 1b8392611b72249068d7b640dddf153a43c0780f
Author: Yuqiang Sun <izaiahsun@gmail.com>

Drivers: hv: vmbus: Do not remove the channel when device registration fails

This commit clears child_device_obj->channel before dropping the device on
the error paths of vmbus_device_register(), and updates
vmbus_device_release() to only call hv_process_channel_removal() if the
channel pointer is non-NULL, leaving channel cleanup on registration failure
to vmbus_add_channel_work().

> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index fc0f16f512669..e8fb0c4c4f5ea 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c

[ ... ]

> @@ -2189,6 +2195,7 @@ int vmbus_device_register(struct hv_device *child_device_obj)
>  	ret = device_register(&child_device_obj->device);
>  	if (ret) {
>  		pr_err("Unable to register child device\n");
> +		child_device_obj->channel = NULL;
>  		put_device(&child_device_obj->device);
>  		return ret;
>  	}

[Severity: High]
Can clearing child_device_obj->channel here (and in err_dev_unregister below)
cause a NULL pointer dereference in vmbus_match() if a concurrent
driver_attach() is iterating over hv_bus?

Once device_register() -> device_add() -> bus_add_device() adds
child_device_obj->device.p->knode_bus to hv_bus's klist_devices, a concurrent
driver_attach() (for example from __vmbus_driver_register() or userspace
writing to new_id_store() -> vmbus_add_dynid()) can iterate over
klist_devices via bus_for_each_dev() while holding an active klist node
reference on child_device_obj->device.

If device_add() subsequently fails at its DPMError label (or if
vmbus_device_register() fails later and calls device_del() at
err_dev_unregister), bus_remove_device() calls klist_del(&dev->p->knode_bus).
Unlike klist_remove(), klist_del() does not wait for active klist_iter
readers in bus_for_each_dev() to finish before vmbus_device_register() sets
child_device_obj->channel = NULL.

On the concurrent bus_for_each_dev() thread, __driver_attach() calls
driver_match_device() -> vmbus_match() before acquiring device_lock(dev) or
checking dev->p->dead:

drivers/hv/vmbus_drv.c:vmbus_match() {
	...
	/* The hv_sock driver handles all hv_sock offers. */
	if (is_hvsock_channel(hv_dev->channel))
		return drv->hvsock;
	...
}

Unlike the sysfs attribute callbacks in drivers/hv/vmbus_drv.c which check
for !hv_dev->channel, vmbus_match() unconditionally passes hv_dev->channel to
is_hvsock_channel(), dereferencing hv_dev->channel->offermsg when
hv_dev->channel is NULL.

> @@ -2228,7 +2235,10 @@ int vmbus_device_register(struct hv_device *child_device_obj)
>  	kset_unregister(child_device_obj->channels_kset);
>  
>  err_dev_unregister:
> -	device_unregister(&child_device_obj->device);
> +	/* The driver's remove, if one was bound, still needs the channel */
> +	device_del(&child_device_obj->device);
> +	child_device_obj->channel = NULL;
> +	put_device(&child_device_obj->device);
>  	return ret;
>  }

[Severity: Critical]
When vmbus_device_register() fails after device_register() has already
probed and bound a driver, does device_del() followed by freeing newchannel
in vmbus_add_channel_work()'s err_deq_chan path leave dangling channel
pointers when the channel has not been rescinded?

There appear to be two cases where drv->remove() does not sever all
references created during drv->probe() unless the channel was rescinded:

1. For hv_sock channels, hvs_probe() -> hvs_open_connection() in
net/vmw_vsock/hyperv_transport.c sets hvs->chan = chan (or
hvs_new->chan = chan) on the vsock_sock, takes a socket reference via
sock_hold(), and inserts the socket into the vsock connected table and accept
queue. When device_del() invokes vmbus_remove() -> hvs_remove():

net/vmw_vsock/hyperv_transport.c:hvs_remove() {
	struct vmbus_channel *chan = hdev->channel;

	vmbus_close(chan);
}

hvs_remove() only closes the ring buffer because in normal operation it is
only called from hvs_destruct() -> vmbus_hvsock_device_unregister() after
hvs_close_connection() has already run on channel rescind. When
err_dev_unregister runs without a rescind, hvs_close_connection() is never
called, so the live vsock_sock retains hvs->chan pointing to newchannel
after vmbus_add_channel_work() frees it via free_channel(newchannel), leading
to a use-after-free in socket operations or in hvs_destruct() ->
vmbus_hvsock_device_unregister(chan).

2. For multichannel drivers that open sub-channels during probe,
vmbus_process_offer() links offered sub-channels onto newchannel->sc_list
with sc->primary_channel = newchannel and queues vmbus_add_channel_work()
onto handle_sub_chan_wq. When device_del() runs drv->remove() ->
vmbus_close() -> vmbus_disconnect_ring():

drivers/hv/channel.c:vmbus_disconnect_ring() {
	...
	list_for_each_entry_safe(cur_channel, tmp, &channel->sc_list, sc_list) {
		if (cur_channel->rescind)
			wait_for_completion(&cur_channel->rescind_event);

		mutex_lock(&vmbus_connection.channel_mutex);
		if (vmbus_close_internal(cur_channel) == 0) {
			vmbus_free_ring(cur_channel);

			if (cur_channel->rescind)
				hv_process_channel_removal(cur_channel);
		}
		mutex_unlock(&vmbus_connection.channel_mutex);
	}
	...
}

vmbus_disconnect_ring() only removes cur_channel if cur_channel->rescind is
true, leaving non-rescinded sub-channels on sc_list and handle_sub_chan_wq
referencing the freed primary channel.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102094719.35760-1-izaiahsun@gmail.com?part=2