Re: [PATCH] hv: hv_balloon: validate unballoon range count

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] free_balloon_pages() blindly trusts untrusted host-provided PFNs, causing arbitrary memory free.
- [High] Teardown race in balloon_remove() leads to Use-After-Free.
- [High] Data race when updating worker arguments from VMBus softirq callbacks.
- [High] Concurrent lockless modification of dm->num_pages_ballooned.
- [High] Pending workqueues are leaked on the balloon_probe() error path.
--

--- Patch [1]: [PATCH] hv: hv_balloon: validate unballoon range count ---
commit cffe2d5855cc693f743d4cc5594c147a6fff1bdb
Author: Michael Bommarito <michael.bommarito@gmail.com>

hv: hv_balloon: validate unballoon range count

The Hyper-V dynamic memory host supplies DM_UNBALLOON_REQUEST messages
with a header size and a range_count field. balloon_down() trusts
range_count and walks req->range_array without checking that the received
message contains that many ranges. A malformed host or backend message can
therefore make the guest read past the received VMBus packet while freeing
balloon ranges.

> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> index 42ce27be344d5..0ebfc2524d630 100644
> --- a/drivers/hv/hv_balloon.c
> +++ b/drivers/hv/hv_balloon.c

[ ... ]

> @@ -1346,6 +1361,12 @@ static void balloon_down(struct hv_dynmem_device *dm,
>  	int i;
>  	unsigned int prev_pages_ballooned = dm->num_pages_ballooned;

[Severity: High]
This is a pre-existing issue, but is it safe to modify
dm->num_pages_ballooned without locking or atomic operations?

If a DM_UNBALLOON_REQUEST arrives while a DM_BALLOON_REQUEST is still being
processed, free_balloon_pages() executes synchronously in the VMBus tasklet
context and decrements the counter:

drivers/hv/hv_balloon.c:free_balloon_pages() {
	...
		dm->num_pages_ballooned--;
	...
}

At the same time, alloc_balloon_pages() executes asynchronously in the
workqueue and increments it:

drivers/hv/hv_balloon.c:alloc_balloon_pages() {
	...
		dm->num_pages_ballooned += alloc_unit;
	...
}

Could lost updates corrupt the guest's memory pressure statistics, leading
to incorrect accounting when reported back to the host via
get_pages_committed()?

>  
> +	if (!unballoon_request_valid(req, msg_size)) {
> +		pr_warn_ratelimited("Invalid unballoon request: size %u, header size %u, range count %u\n",
> +				    msg_size, req->hdr.size, req->range_count);
> +		return;
> +	}
> +
>  	for (i = 0; i < range_count; i++) {
>  		free_balloon_pages(dm, &range_array[i]);
>  		complete(&dm_device.config_event);

[Severity: Critical]
This is a pre-existing issue, but does free_balloon_pages() blindly trust
the untrusted host-provided PFNs?

If a malicious host sends a DM_UNBALLOON_REQUEST specifying arbitrary PFNs
(e.g., kernel text or page tables), the guest iterates over these PFNs and
unconditionally frees them without validating if the pages were actually
ballooned out:

drivers/hv/hv_balloon.c:free_balloon_pages() {
	...
	for (i = 0; i < num_pages; i++) {
		pg = pfn_to_page(i + start_frame);
		__ClearPageOffline(pg);
		__free_page(pg);
	...
}

Can this lead to an immediate guest crash or arbitrary memory corruption via
freeing active kernel or user pages to the buddy allocator?

[ ... ]

> @@ -1527,7 +1548,8 @@ static void balloon_onchannelcallback(void *context)
>  
>  			dm->state = DM_BALLOON_DOWN;
>  			balloon_down(dm,
> -				     (struct dm_unballoon_request *)recv_buffer);
> +				     (struct dm_unballoon_request *)recv_buffer,
> +				     recvlen);
>  			break;
>  
>  		case DM_MEM_HOT_ADD_REQUEST:

[Severity: High]
This is a pre-existing issue, but are worker arguments safely updated during
VMBus softirq callbacks?

The VMBus interrupt handler balloon_onchannelcallback() directly overwrites
shared worker arguments without any locking or synchronization:

drivers/hv/hv_balloon.c:balloon_onchannelcallback() {
	...
		case DM_BALLOON_REQUEST:
			...
			dm_device.balloon_wrk.num_pages = bal_msg->num_pages;
			schedule_work(&dm_device.balloon_wrk.wrk);
			break;
	...
		case DM_MEM_HOT_ADD_REQUEST:
			...
				dm->ha_wrk.ha_page_range = *ha_pg_range;
				dm->ha_wrk.ha_region_range.page_range = 0;
	...
}

If an untrusted host floods the guest with back-to-back request messages,
can worker threads operate on torn, mixed, or corrupted parameters, leading
to incorrect guest memory operations?


[Severity: High]
This isn't a bug introduced by this patch, but does balloon_remove() have a
teardown race that violates subsystem rules?

The function calls cancel_work_sync() before vmbus_close():

drivers/hv/hv_balloon.c:balloon_remove() {
	...
	cancel_work_sync(&dm->balloon_wrk.wrk);
	cancel_work_sync(&dm->ha_wrk.wrk);
	...
	if (dm_device.state != DM_INIT_ERROR) {
		disable_page_reporting();
		vmbus_close(dev->channel);
	...
}

Because the VMBus channel remains open and active, an incoming interrupt can
trigger balloon_onchannelcallback() and independently reschedule the work
item after it was already cancelled.

Could this lead to a use-after-free of a work struct and device state,
causing a kernel crash upon module unload or device unbind?


[Severity: High]
This is a pre-existing issue, but does the probe_error path leak pending
workqueues?

In balloon_probe(), balloon_connect_vsp() opens the channel, allowing the
host to immediately send messages that schedule balloon_wrk or ha_wrk. If
the subsequent kthread_run() step fails, the code jumps to probe_error,
closes the channel, and returns an error without cancelling the work items:

drivers/hv/hv_balloon.c:balloon_probe() {
	...
probe_error:
	dm_device.state = DM_INIT_ERROR;
	dm_device.thread  = NULL;
	disable_page_reporting();
	vmbus_close(dev->channel);
connect_error:
	...
}

Can this result in asynchronous execution of a leaked work item on
uninitialized or freed device state, causing a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710022914.3740453-1-michael.bommarito@gmail.com?part=1