Re: [PATCH v2 08/14] hv: vmbus: retain backing until ownership and references clear

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] workqueue: UAF of workqueue in vmbus_buffer_reclaimer_shutdown() racing with timer
- [Medium] mm: permanent memory leak of arrays and successful pages on partial encryption failure
--

commit 6530feb3fbe0165655c6797c004705143b2a41a7
Author: Emerson Busson <emersonbusson@gmail.com>
Subject: hv: vmbus: retain backing until ownership and references clear

This commit changes the VMBus channel code to keep buffer allocations and
GPADL states retained until ownership, page-state, and mapping-reference
conditions are clear. It adds a background workqueue for delayed reclaim
when mappings are released or teardown is acknowledged.

> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
[ ... ]
> +static void vmbus_buffer_reclaim_work(struct work_struct *work)
> +{
[ ... ]
> +	if (ret) {
> +		mutex_lock(&vmbus_buffer_owners_lock);
> +		owner->permanent_leak = true;
> +		owner->work_active = false;
> +		owner->reclaiming = false;
> +		vmbus_buffer_trace_owner(owner, "retained");
> +		mutex_unlock(&vmbus_buffer_owners_lock);
> +		pr_warn_ratelimited("VMBus buffer reclaim retained pages after encryption failure: %d\n",
> +				    ret);
> +		return;

[Severity: Medium]
Will this early return permanently leak previously processed pages?

In vmbus_buffer_reclaim_work(), if set_memory_encrypted() fails on a chunk,
it breaks the loop and returns early.

Bypassing the cleanup paths permanently leaks all other successfully
processed pages in the buffer, as well as the owner->chunks and
owner->pages tracking arrays allocated via kvmalloc_array, and the owner
struct itself.

> +	}
> +
> +	vmbus_buffer_trace_owner(owner, "reclaimed");
[ ... ]
> +/* Caller holds the owner lock or exclusive custody of this owner. */
> +void vmbus_buffer_owner_drain(struct vmbus_buffer_retained *owner,
> +			      struct workqueue_struct *wq)
> +{
> +	if (!cancel_delayed_work(&owner->reclaim_work))
> +		return;

[Severity: High]
Does this non-sync cancel risk a use-after-free when racing with the timer
callback?

If the delayed work's timer has just expired and its softirq handler
(delayed_work_timer_fn) is concurrently running on another CPU,
cancel_delayed_work() returns 0 without waiting.

> +
> +	owner->work_active = false;
> +	if (wq && vmbus_buffer_owner_can_reclaim(owner)) {
> +		owner->work_active = true;
> +		mod_delayed_work(wq, &owner->reclaim_work, 0);
> +	}
> +}
> +
> +void vmbus_buffer_reclaimer_shutdown(void)
> +{
> +	struct workqueue_struct *wq;
> +	struct vmbus_buffer_retained *owner;
[ ... ]
> +	list_for_each_entry(owner, &vmbus_buffer_owners, list)
> +		vmbus_buffer_owner_drain(owner, wq);
> +	mutex_unlock(&vmbus_buffer_owners_lock);
> +
> +	if (wq)
> +		destroy_workqueue(wq);

[Severity: High]
Can destroy_workqueue() complete while the timer handler is still running?

vmbus_buffer_reclaimer_shutdown() uses cancel_delayed_work() to stop
reclaim works. If the delayed work's timer just expired, it returns
immediately.

The shutdown function then drops the mutex and calls destroy_workqueue(wq).

destroy_workqueue() flushes existing work but does not synchronize with
concurrent timer handlers that have not yet called queue_work().

destroy_workqueue() finishes and frees the workqueue structure.

The timer handler then proceeds to call queue_work() on the freed workqueue,
causing a use-after-free on module unload.

> +}
[ ... ]
> +int vmbus_alloc_buffer_owned(struct vmbus_channel *channel, u32 size,
> +			     bool confidential, struct vmbus_buffer *buffer)
> +{
[ ... ]
> +	while (remaining) {
> +		struct page *page;
> +		gfp_t gfp;
> +		int nid;
> +
> +		order = vmbus_buffer_order(remaining, order);
> +
> +		gfp = GFP_KERNEL | __GFP_ZERO;
> +
> +		nid = cpu_to_node(channel->target_cpu);
> +		page = vmbus_alloc_pages_with_fallback(nid, gfp, &order,
> +						       vmbus_alloc_pages_node, NULL);
> +		if (!page)
> +			goto err;
> +
> +		buffer->chunks[buffer->chunk_cnt++] = page;
> +		for (i = 0; i < (1U << order); i++)
> +			buffer->pages[page_idx++] = page + i;
> +		buffer->page_cnt = page_idx;
> +
> +		ret = set_memory_decrypted((unsigned long)page_address(page),
> +					   1U << order);
> +		if (ret) {
> +			/*
> +			 * set_memory_decrypted() failed; the page state is
> +			 * unknown so it must be leaked rather than freed.
> +			 */
> +			owner->encryption_unknown = true;
> +			goto err;

[Severity: Medium]
Could this goto leak the successfully processed pages?

In vmbus_alloc_buffer_owned(), if set_memory_decrypted() fails on a chunk,
it sets owner->encryption_unknown = true and jumps to err.

This marks the entire owner as non-reclaimable, leaking all previously
populated chunks and tracking arrays.

> +		}
> +		owner->needs_encrypt = true;
> +
> +		remaining -= 1U << order;
> +	}

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