Re: [PATCH v2 1/3] Drivers: hv: vmbus: Fix error paths in vmbus_teardown_gpadl()

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

Pre-existing issues:
- [High] Missing update to `gpadl->decrypted` on the `kzalloc` error path in `vmbus_teardown_gpadl()` bypasses the memory leak mechanism, leading to guest memory corruption.
--

commit 06cfb23325c5ac07c00c74d8250a4bd0f20ac4f0
Author: Michael Kelley [off-list ref]

    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")

[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;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907214902.9046-1-mhklinux@outlook.com?part=1