| Message ID | 20240222021006.2279329-3-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Handle set_memory_XXcrypted() errors in hyperv | expand |
From: Rick Edgecombe <rick.p.edgecombe@intel.com> Sent: Wednesday, February 21, 2024 6:10 PM > See comment in Patch 1 about the "Subject" prefix. > On TDX it is possible for the untrusted host to cause See comment in Patch 1 about TDX vs. CoCo VM. > 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 caller's of vmbus_establish_gpadl() and s/caller's/callers/ > 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 buffer's. This will allow the callers to s/buffer's/buffers/ > know if they should free or leak the pages. > > Only compile tested. > > Cc: "K. Y. Srinivasan" <kys@microsoft.com> > Cc: Haiyang Zhang <haiyangz@microsoft.com> > Cc: Wei Liu <wei.liu@kernel.org> > Cc: Dexuan Cui <decui@microsoft.com> > Cc: linux-hyperv@vger.kernel.org > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> > --- > drivers/hv/channel.c | 11 ++++++++--- > include/linux/hyperv.h | 1 + > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c > index 56f7e06c673e..fe5d2f505a39 100644 > --- a/drivers/hv/channel.c > +++ b/drivers/hv/channel.c > @@ -478,6 +478,7 @@ static int __vmbus_establish_gpadl(struct > vmbus_channel *channel, > ret = set_memory_decrypted((unsigned long)kbuffer, > PFN_UP(size)); > if (ret) { > + gpadl->decrypted = false; There's an earlier error return in this function where gpadl->decrypted isn't set at all. I think it works because that flag is in the channel structure, which is zero'd when allocated, so the flag defaults to false. But it would probably be better to explicitly set gpadl->decrypted to false upon entry to the function so there's no implicit and potentially ambiguous behavior. > dev_warn(&channel->device_obj->device, > "Failed to set host visibility for new GPADL %d.\n", > ret); > @@ -550,6 +551,7 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel, > gpadl->gpadl_handle = gpadlmsg->gpadl; > gpadl->buffer = kbuffer; > gpadl->size = size; > + gpadl->decrypted = true; This needs to be done immediately after the call to set_memory_decrypted() is known to succeed, so that the "goto cleanup" cases get to the "cleanup:" label with correct information about the encrypted state of the memory. > > > cleanup: > @@ -563,9 +565,10 @@ 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((unsigned long)kbuffer, PFN_UP(size))) Doesn't the above need to be "if (!set_memory_encrypted(...))" ? Then if set_memory_encrypted() fails, gpadl->decrypted will be "true". > + gpadl->decrypted = false; > + } > > return ret; > } > @@ -886,6 +889,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); > > + gpadl->decrypted = ret; > + > 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 { > -- > 2.34.1
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index 56f7e06c673e..fe5d2f505a39 100644 --- a/drivers/hv/channel.c +++ b/drivers/hv/channel.c @@ -478,6 +478,7 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel, ret = set_memory_decrypted((unsigned long)kbuffer, PFN_UP(size)); if (ret) { + gpadl->decrypted = false; dev_warn(&channel->device_obj->device, "Failed to set host visibility for new GPADL %d.\n", ret); @@ -550,6 +551,7 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel, gpadl->gpadl_handle = gpadlmsg->gpadl; gpadl->buffer = kbuffer; gpadl->size = size; + gpadl->decrypted = true; cleanup: @@ -563,9 +565,10 @@ 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((unsigned long)kbuffer, PFN_UP(size))) + gpadl->decrypted = false; + } return ret; } @@ -886,6 +889,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); + gpadl->decrypted = ret; + 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 {
On TDX 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 caller's 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 buffer's. This will allow the callers to know if they should free or leak the pages. Only compile tested. Cc: "K. Y. Srinivasan" <kys@microsoft.com> Cc: Haiyang Zhang <haiyangz@microsoft.com> Cc: Wei Liu <wei.liu@kernel.org> Cc: Dexuan Cui <decui@microsoft.com> Cc: linux-hyperv@vger.kernel.org Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com> --- drivers/hv/channel.c | 11 ++++++++--- include/linux/hyperv.h | 1 + 2 files changed, 9 insertions(+), 3 deletions(-)