This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://sashiko.dev/#/patchset/[email protected]?part=1 --- Drivers: hv: vmbus: Fix error paths in vmbus_teardown_gpadl() In vmbus_teardown_gpadl(), the return value from vmbus_post_msg() is overwritten by the logic that decides if set_memory_encrypted() should run. A failure from vmbus_post_msg() is lost and vmbus_teardown_gpadl() may incorrectly report success. Furthermore, if vmbus_post_msg() fails, the GPADL remains active on the Hyper-V side, yet in a CoCo VM the buffer will be re-encrypted anyway.
Reported-by: Sashiko <[email protected]> Closes: https://lore.kernel.org/linux-hyperv/[email protected]/ Fixes: d4dccf353db8 ("Drivers: hv: vmbus: Mark vmbus ring buffer visible to host in Isolation VM") > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c > index f4370617deacb..cc86e8505ad0b 100644 > --- a/drivers/hv/channel.c > +++ b/drivers/hv/channel.c > @@ -1059,14 +1059,14 @@ int vmbus_teardown_gpadl(struct vmbus_channel > *channel, struct vmbus_gpadl *gpad > > kfree(info); > > - if (gpadl->decrypted) > + if (!ret && gpadl->decrypted) { > ret = set_memory_encrypted((unsigned long)gpadl->buffer, > PFN_UP(gpadl->size)); > - else > - ret = 0; > - if (ret) > - pr_warn("Fail to set mem host visibility in GPADL teardown > %d.\n", ret); > + if (ret) > + pr_warn("Fail to set mem host visibility in GPADL > teardown %d.\n", ret); > + } > > + /* If error in ret, mark buffer decrypted so it is leaked */ > gpadl->decrypted = ret; [Severity: High] This is a pre-existing issue, but does this early return in vmbus_teardown_gpadl() bypass the leak mechanism? drivers/hv/channel.c:vmbus_teardown_gpadl() { ... info = kzalloc(sizeof(*info) + sizeof(struct vmbus_channel_gpadl_teardown), GFP_KERNEL); if (!info) return -ENOMEM; ... } If this kzalloc() fails, gpadl->decrypted is not updated. In a CoCo VM, __vmbus_open() establishes a GPADL for the ring buffer and initializes gpadl->decrypted to false. If a subsequent step in __vmbus_open() fails, it calls vmbus_teardown_gpadl() to roll back: drivers/hv/channel.c:__vmbus_open() { ... error_free_gpadl: vmbus_teardown_gpadl(newchannel, &newchannel->ringbuffer_gpadlhandle); error_clean_ring: ... return err; } Since __vmbus_open() ignores the teardown failure, vmbus_open() will then proceed to call vmbus_free_ring(): drivers/hv/channel.c:vmbus_free_ring() { ... /* In a CoCo VM leak the memory if it didn't get re-encrypted */ if (!channel->ringbuffer_gpadlhandle.decrypted) __free_pages(channel->ringbuffer_page, ... } Because gpadl->decrypted remains false instead of being set to -ENOMEM, vmbus_free_ring() will erroneously free the memory back to the kernel page allocator. Could this lead to guest memory corruption if the host hypervisor retains an active GPADL mapping and overwrites it? > > return ret;
