Re: [PATCH v2 1/3] Drivers: hv: vmbus: Fix error paths in vmbus_teardown_gpadl()
From: Simon Horman <horms@kernel.org>
Date: 2026-09-09 16:08:04
Also in:
linux-hyperv, lkml
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/20260907214902.9046-2-mhklinux@outlook.com?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 <sashiko-bot@kernel.org> Closes: https://lore.kernel.org/linux-hyperv/20260904160724.82AB51F00A3D@smtp.kernel.org/ (local) Fixes: d4dccf353db8 ("Drivers: hv: vmbus: Mark vmbus ring buffer visible to host in Isolation VM")
quoted hunk ↗ jump to hunk
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;