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
