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 [off-list ref]

    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.

[ ... ]

+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/20260721195633.1438361-1-kameroncarr@linux.microsoft.com?part=1