On 3/11/24 9:15 AM, mhkelley58@xxxxxxxxx wrote: > From: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx> > > In CoCo VMs it is possible for the untrusted host to cause > set_memory_encrypted() or set_memory_decrypted() to fail such that an > error is returned and the resulting memory is shared. Callers need to > take care to handle these errors to avoid returning decrypted (shared) > memory to the page allocator, which could lead to functional or security > issues. > > In order to make sure callers of vmbus_establish_gpadl() and > vmbus_teardown_gpadl() don't return decrypted/shared pages to > allocators, add a field in struct vmbus_gpadl to keep track of the > decryption status of the buffers. This will allow the callers to > know if they should free or leak the pages. > > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx> > Signed-off-by: Michael Kelley <mhklinux@xxxxxxxxxxx> > --- > drivers/hv/channel.c | 25 +++++++++++++++++++++---- > include/linux/hyperv.h | 1 + > 2 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c > index 56f7e06c673e..bb5abdcda18f 100644 > --- a/drivers/hv/channel.c > +++ b/drivers/hv/channel.c > @@ -472,9 +472,18 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel, > (atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1); > > ret = create_gpadl_header(type, kbuffer, size, send_offset, &msginfo); > - if (ret) > + if (ret) { > + gpadl->decrypted = false; Why not set it by default at the beginning of the function? > return ret; > + } > > + /* > + * Set the "decrypted" flag to true for the set_memory_decrypted() > + * success case. In the failure case, the encryption state of the > + * memory is unknown. Leave "decrypted" as true to ensure the > + * memory will be leaked instead of going back on the free list. > + */ > + gpadl->decrypted = true; > ret = set_memory_decrypted((unsigned long)kbuffer, > PFN_UP(size)); > if (ret) { > @@ -563,9 +572,15 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel, > > kfree(msginfo); > > - if (ret) > - set_memory_encrypted((unsigned long)kbuffer, > - PFN_UP(size)); > + if (ret) { > + /* > + * If set_memory_encrypted() fails, the decrypted flag is > + * left as true so the memory is leaked instead of being > + * put back on the free list. > + */ > + if (!set_memory_encrypted((unsigned long)kbuffer, PFN_UP(size))) > + gpadl->decrypted = false; > + } > > return ret; > } > @@ -886,6 +901,8 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, struct vmbus_gpadl *gpad > if (ret) > pr_warn("Fail to set mem host visibility in GPADL teardown %d.\n", ret); Will this be called only if vmbus_establish_gpad() is successful? If not, you might want to skip set_memory_encrypted() call for decrypted = false case. > > + gpadl->decrypted = ret; > + IMO, you can set it to false by default. Any way with non zero return, user know about the decryption failure. > return ret; > } > EXPORT_SYMBOL_GPL(vmbus_teardown_gpadl); > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h > index 2b00faf98017..5bac136c268c 100644 > --- a/include/linux/hyperv.h > +++ b/include/linux/hyperv.h > @@ -812,6 +812,7 @@ struct vmbus_gpadl { > u32 gpadl_handle; > u32 size; > void *buffer; > + bool decrypted; > }; > > struct vmbus_channel { -- Sathyanarayanan Kuppuswamy Linux Kernel Developer