Re: [PATCH v2 10/14] hv: vmbus: pin buffer pages across UIO mmap to close the reclaim race

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hv: vmbus_buffer_pin_pages UAF race on subchannel ringbuffers
--

commit 69d0c6246e4ab3d36e972718b31a04ff08d2eee9
Author: Emerson Busson <emersonbusson@gmail.com>
Subject: hv: vmbus: pin buffer pages across UIO mmap to close the reclaim race

This commit pins buffer pages across UIO mmap to prevent a race condition 
with the reclaim worker. It adds vmbus_buffer_pin_pages() and 
vmbus_buffer_unpin_pages() to safely take references on the pages. The sysfs 
ring mmap wrapper is adjusted to handle the new pin lifecycle.

> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 7eb9ea814ef45..2793ca1b7f320 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
[ ... ]
> @@ -1145,6 +1145,98 @@ void vmbus_release_buffer(struct vmbus_buffer *buffer)
>  }
>  EXPORT_SYMBOL_GPL(vmbus_release_buffer);
>  
> +/**
> + * vmbus_buffer_pin_pages - snapshot a buffer's pages and hold references
[ ... ]
> +int vmbus_buffer_pin_pages(struct vmbus_buffer *buffer,
> +			   struct vmbus_buffer_pin *pin)
> +{
> +	struct vmbus_buffer_retained *owner;
> +	u32 i;
> +
> +	/*
> +	 * The owner and page array are published and cleared under
> +	 * vmbus_buffer_owners_lock (see vmbus_release_buffer()). Reading
> +	 * either outside that lock lets a concurrent release free the array
> +	 * while this loop still walks it.
> +	 */
> +	mutex_lock(&vmbus_buffer_owners_lock);

[Severity: High]
Does this code still have a use-after-free race for subchannel ringbuffers?

In vmbus_release_buffer(), if buffer->owner is NULL, the buffer is freed 
and zeroed without acquiring vmbus_buffer_owners_lock. Subchannel 
ringbuffers do not have an owner, so their release bypasses this lock.

If a concurrent mmap() is called on a subchannel's sysfs ring file during 
device removal, this function will acquire the lock and start walking 
buffer->pages while vmbus_release_buffer() concurrently zeroes and frees 
it without the lock, causing a crash.

> +	owner = buffer->owner;
> +	if (!buffer->pages || !buffer->page_cnt) {
> +		mutex_unlock(&vmbus_buffer_owners_lock);
> +		return -ENODEV;
> +	}
[ ... ]
> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
> index b40e80e19c6c6..91cb25d019be9 100644
> --- a/drivers/uio/uio_hv_generic.c
> +++ b/drivers/uio/uio_hv_generic.c
[ ... ]
> @@ -372,12 +637,20 @@ static void
>  hv_uio_remove(struct hv_device *dev)
>  {
>  	struct hv_uio_private_data *pdata = hv_get_drvdata(dev);
> +	int ret;
>  
>  	if (!pdata)
>  		return;
>  
>  	hv_remove_ring_sysfs(dev->channel);

[Severity: High]
Does this code leave subchannel sysfs files active and mapped?

hv_remove_ring_sysfs() is only called on the primary channel, which leaves 
any subchannel ring sysfs files active. 

> +	/* Keep event notification alive until channel callbacks are stopped. */
> +	get_device(&pdata->info.uio_dev->dev);
>  	uio_unregister_device(&pdata->info);
> +	/* unregister prevents the eventual fd close from calling .release. */
> +	ret = hv_uio_disconnect_if_open(dev->channel, vmbus_disconnect_ring);

vmbus_disconnect_ring() iterates over the subchannels and frees their 
ringbuffers directly. Because the subchannel sysfs ring files are still 
active, userspace can call mmap() concurrently on them. This triggers the 
lockless vmbus_release_buffer() vs vmbus_buffer_pin_pages() race described 
above.

Could hv_remove_ring_sysfs() be invoked on all subchannels before their 
ringbuffers are freed?

> +	if (ret)
> +		dev_err(&dev->device, "channel disconnect failed: %d\n", ret);
> +	put_device(&pdata->info.uio_dev->dev);
>  	hv_uio_cleanup(dev, pdata);
>  
>  	vmbus_free_ring(dev->channel);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007190752.336426-1-emersonbusson@gmail.com?part=10