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

[Simon Horman]

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@xxxxxxxxxxx?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@xxxxxxxxxx>
Closes: https://lore.kernel.org/linux-hyperv/20260904160724.82AB51F00A3D@xxxxxxxxxxxxxxx/
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;