Re: [PATCH v3 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:
- [High] hv: vmbus: channel UAF and double removal on vmbus_add_channel_work() error
- [High] hv: vmbus: leaked CPU mask and suspend counter in vmbus_add_channel_work()
--

commit 99d3a552529c5941e934071a1f2968acea94e54e
Author: Yuqiang Sun <izaiahsun@gmail.com>

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

This commit avoids calling hv_process_channel_removal() from
vmbus_device_release() when device_register() fails by clearing the
device's channel pointer and letting vmbus_add_channel_work() clean up
the channel. It also keeps the device registered if sysfs channel
setup fails after device_register() succeeds.

> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index cb1d8d76928ff..1fa232e86fce2 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -554,15 +554,16 @@ static void vmbus_add_channel_work(struct work_struct *work)
>  	 * method.
>  	 *
>  	 * If vmbus_device_register() fails, the 'device_obj' is freed in
> -	 * vmbus_device_release() as called by device_unregister() in the
> -	 * error path of vmbus_device_register(). In the outside error
> -	 * path, there's no need to free it.
> +	 * vmbus_device_release() as called by put_device() in the error
> +	 * path of vmbus_device_register(). In the outside error path,
> +	 * there's no need to free it, but the channel must not point to it.
>  	 */
>  	ret = vmbus_device_register(newchannel->device_obj);
>  
>  	if (ret != 0) {
>  		pr_err("unable to add child device object (relid %d)\n",
>  			newchannel->offermsg.child_relid);
> +		newchannel->device_obj = NULL;
>  		goto err_deq_chan;
>  	}

[Severity: High]
As reported by Sashiko on v2 [1], can jumping to err_deq_chan in
vmbus_add_channel_work() race with a concurrent vmbus_onoffer_rescind()
and cause a use-after-free on channel or a double channel removal?

In vmbus_process_offer(), newchannel is linked onto chn_list or sc_list,
mapped via vmbus_channel_map_relid(), and offer_in_progress is decremented
before vmbus_add_channel_work() is queued. If a rescind offer arrives
while vmbus_add_channel_work() is running, vmbus_onoffer_rescind() looks
up the channel, sets channel->rescind_ref = true and channel->rescind = true,
and waits for probe_done without holding a kobject reference or an RCU
read lock:

drivers/hv/channel_mgmt.c:vmbus_onoffer_rescind() {
	...
	while (READ_ONCE(channel->probe_done) == false) {
		msleep(1);
	}

	if (channel->device_obj) {
		...
	} else if (channel->primary_channel != NULL) {
		mutex_lock(&vmbus_connection.channel_mutex);
		if (channel->state == CHANNEL_OPEN_STATE) {
			hv_process_channel_removal(channel);
		}
		...
}

When vmbus_device_register() fails (or when vmbus_device_create() or
vmbus_add_channel_kobj() fails), err_deq_chan sets
newchannel->probe_done = true, unlinks and unmaps newchannel, and calls
free_channel(newchannel) -> kobject_put() -> vmbus_chan_release() ->
kfree_rcu(channel, rcu):

drivers/hv/channel_mgmt.c:vmbus_add_channel_work() {
	...
err_deq_chan:
	mutex_lock(&vmbus_connection.channel_mutex);
	newchannel->probe_done = true;

	if (primary_channel == NULL)
		list_del(&newchannel->listentry);
	else
		list_del(&newchannel->sc_list);

	vmbus_channel_unmap_relid(newchannel);
	mutex_unlock(&vmbus_connection.channel_mutex);

	vmbus_release_relid(newchannel->offermsg.child_relid);
	free_channel(newchannel);
}

Because vmbus_onoffer_rescind() is sleeping in msleep(1), the RCU grace
period can finish before vmbus_onoffer_rescind() wakes up and dereferences
channel->probe_done, channel->device_obj, and channel->primary_channel.

In addition, on weakly ordered architectures, newchannel->device_obj = NULL
and newchannel->probe_done = true are plain stores separated only by
mutex_lock(&vmbus_connection.channel_mutex) (an acquire barrier, not a
release barrier), while vmbus_onoffer_rescind() reads
READ_ONCE(channel->probe_done) and channel->device_obj without holding
channel_mutex or using load-acquire. Could vmbus_onoffer_rescind() observe
probe_done == true while still seeing the stale non-NULL channel->device_obj
pointer to the freed hv_device?

Finally, for a sub-channel failing in vmbus_add_channel_kobj(),
newchannel->state was already set to CHANNEL_OPEN_STATE at the start of
vmbus_add_channel_work(). When vmbus_onoffer_rescind() wakes up and enters
the channel->primary_channel != NULL branch, wouldn't it see
channel->state == CHANNEL_OPEN_STATE and call
hv_process_channel_removal(channel) a second time after err_deq_chan has
already unlinked, unmapped, and freed it?

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

[ ... ]

> @@ -979,9 +994,15 @@ static void vmbus_device_release(struct device *device)
>  
>  	hv_debug_rm_dev_dir(hv_dev);
>  
> -	mutex_lock(&vmbus_connection.channel_mutex);
> -	hv_process_channel_removal(channel);
> -	mutex_unlock(&vmbus_connection.channel_mutex);
> +	/*
> +	 * No channel if vmbus_device_register() failed: the channel was not
> +	 * rescinded, and vmbus_add_channel_work() cleans it up.
> +	 */
> +	if (channel) {
> +		mutex_lock(&vmbus_connection.channel_mutex);
> +		hv_process_channel_removal(channel);
> +		mutex_unlock(&vmbus_connection.channel_mutex);
> +	}
>  	kfree(hv_dev);
>  }

[Severity: High]
As reported by Sashiko on v2 [1], when vmbus_device_release() skips
hv_process_channel_removal() and leaves cleanup to err_deq_chan in
vmbus_add_channel_work(), does err_deq_chan leak the allocated CPU bit in
hv_numa_map and the nr_chan_close_on_suspend counter?

Before queuing vmbus_add_channel_work(), vmbus_process_offer() allocates
target_cpu and increments nr_chan_close_on_suspend:

drivers/hv/channel_mgmt.c:vmbus_process_offer() {
	...
	init_vp_index(newchannel);

	/* Remember the channels that should be cleaned up upon suspend. */
	if (is_hvsock_channel(newchannel) || is_sub_channel(newchannel))
		atomic_inc(&vmbus_connection.nr_chan_close_on_suspend);
	...
}

Unlike hv_process_channel_removal(), err_deq_chan in
vmbus_add_channel_work() does not call
hv_clear_allocated_cpu(newchannel->target_cpu) when
hv_is_perf_channel(newchannel) is true.

It also unmaps the channel's relid via vmbus_channel_unmap_relid(newchannel)
without decrementing vmbus_connection.nr_chan_close_on_suspend (via
check_ready_for_suspend_event()) when is_hvsock_channel(newchannel) or
is_sub_channel(newchannel) is true. Because the relid is unmapped, even a
later rescind from the host will see relid2channel() == NULL in
vmbus_onoffer_rescind() and return early. Wouldn't that leave
nr_chan_close_on_suspend permanently elevated and cause a subsequent
vmbus_bus_suspend() to hang indefinitely in
wait_for_completion(&vmbus_connection.ready_for_suspend_event)?

[1] https://lore.kernel.org/r/20261103080347.90310-3-izaiahsun@gmail.com

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