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