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 <[email protected]> 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/[email protected]?part=8
