Re: [RFC PATCH 1/2] Drivers: hv: vmbus: add vmbus_establish_gpadl_caller_decrypted()

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but vmbus_teardown_gpadl() silently overwrites the error code from vmbus_post_msg(), causing it to falsely report success when GPADL teardown fails, leading to use-after-free memory corruption.
--

--- Patch [1]: [RFC PATCH 1/2] Drivers: hv: vmbus: add vmbus_establish_gpadl_caller_decrypted() ---
commit 93ae4b1e666b7bf013113501ce3aaa80b55f1c28
Author: Kameron Carr <[email protected]>

    Drivers: hv: vmbus: add vmbus_establish_gpadl_caller_decrypted()
    
    Refactor vmbus_establish_gpadl() to separate the encryption lifecycle
    from the rest of the GPADL establishment logic. Add a new 
    vmbus_establish_gpadl_caller_decrypted() for callers that want
    to decrypt their own buffers.

> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 6821f225248b1..0166367a4df37 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
[ ... ]
> +int vmbus_establish_gpadl_caller_decrypted(struct vmbus_channel *channel,
> +					   void *kbuffer, u32 size,
> +					   struct vmbus_gpadl *gpadl)
> +{
> +	int ret = __vmbus_establish_gpadl(channel, HV_GPADL_BUFFER, kbuffer,
> +					  size, 0U, gpadl);
> +
> +	/* Caller owns @kbuffer's encryption; teardown must not touch it. */
> +	gpadl->decrypted = false;

[Severity: High]
This isn't a bug introduced by this patch, but does setting gpadl->decrypted
to false here interact with a pre-existing issue in vmbus_teardown_gpadl()
that masks teardown failures?

In vmbus_teardown_gpadl(), if vmbus_post_msg() fails (e.g., due to host
resource starvation), the error code in the ret variable is overwritten
unconditionally before the function returns:

drivers/hv/channel.c:vmbus_teardown_gpadl() {
    ...
	ret = vmbus_post_msg(msg,
			     sizeof(struct vmbus_channel_gpadl_teardown), true);
	trace_vmbus_teardown_gpadl(msg, ret);

	if (ret)
		goto post_msg_err;
    ...
post_msg_err:
    ...
	if (gpadl->decrypted)
		ret = set_memory_encrypted((unsigned long)gpadl->buffer,
					PFN_UP(gpadl->size));
	else
		ret = 0;
    ...
	return ret;
}

Since gpadl->decrypted is set to false for these caller-managed GPADLs,
if vmbus_post_msg() fails during teardown, the else branch will set ret = 0.

Could this cause vmbus_teardown_gpadl() to falsely report success when
the teardown actually failed? If the caller then frees the buffer while
the hypervisor still holds a mapping to it, could it lead to memory
corruption across the VM boundary?

> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(vmbus_establish_gpadl_caller_decrypted);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1