Shared (decrypted) pages should never return to the page allocator, or
future usage of the pages may allow for the contents to be exposed to the
host. They may also cause the guest to crash if the page is used in way
disallowed by HW (i.e. for executable code or as a page table).
Normally set_memory() call failures are rare. But on TDX
set_memory_XXcrypted() involves calls to the untrusted VMM, and an attacker
could fail these calls such that:
1. set_memory_encrypted() returns an error and leaves the pages fully
shared.
2. set_memory_decrypted() returns an error, but the pages are actually
full converted to shared.
This means that patterns like the below can cause problems:
void *addr = alloc();
int fail = set_memory_decrypted(addr, 1);
if (fail)
free_pages(addr, 0);
And:
void *addr = alloc();
int fail = set_memory_decrypted(addr, 1);
if (fail) {
set_memory_encrypted(addr, 1);
free_pages(addr, 0);
}
Unfortunately these patterns appear in the kernel. And what the
set_memory() callers should do in this situation is not clear either. They
shouldn’t use them as shared because something clearly went wrong, but
they also need to fully reset the pages to private to free them. But, the
kernel needs the VMMs help to do this and the VMM is already being
uncooperative around the needed operations. So this isn't guaranteed to
succeed and the caller is kind of stuck with unusable pages.
The only choice is to panic or leak the pages. The kernel tries not to
panic if at all possible, so just leak the pages at the call sites.
Separately there is a patch[0] to warn if the guest detects strange VMM
behavior around this. It is stalled, so in the mean time I’m proceeding
with fixing the callers to leak the pages. No additional warnings are
added, because the plan is to warn in a single place in x86 set_memory()
code.
This series fixes the cases in the hyperv code.
IMPORTANT NOTE:
I don't have a setup to test tdx hyperv changes. These changes are compile
tested only. Previously Michael Kelley suggested some folks at MS might be
able to help with this.
[0] https://lore.kernel.org/lkml/[email protected]/
Rick Edgecombe (4):
hv: Leak pages if set_memory_encrypted() fails
hv: Track decrypted status in vmbus_gpadl
hv_nstvsc: Don't free decrypted memory
uio_hv_generic: Don't free decrypted memory
drivers/hv/channel.c | 11 ++++++++---
drivers/hv/connection.c | 11 +++++++----
drivers/net/hyperv/netvsc.c | 7 +++++--
drivers/uio/uio_hv_generic.c | 12 ++++++++----
include/linux/hyperv.h | 1 +
5 files changed, 29 insertions(+), 13 deletions(-)
--
2.34.1
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.
Hyperv could free decrypted/shared pages if set_memory_encrypted() fails.
Leak the pages if this happens.
Only compile tested.
Cc: "K. Y. Srinivasan" <[email protected]>
Cc: Haiyang Zhang <[email protected]>
Cc: Wei Liu <[email protected]>
Cc: Dexuan Cui <[email protected]>
Cc: [email protected]
Signed-off-by: Rick Edgecombe <[email protected]>
---
drivers/hv/connection.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 3cabeeabb1ca..e39493421bbb 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -315,6 +315,7 @@ int vmbus_connect(void)
void vmbus_disconnect(void)
{
+ int ret;
/*
* First send the unload request to the host.
*/
@@ -337,11 +338,13 @@ void vmbus_disconnect(void)
vmbus_connection.int_page = NULL;
}
- set_memory_encrypted((unsigned long)vmbus_connection.monitor_pages[0], 1);
- set_memory_encrypted((unsigned long)vmbus_connection.monitor_pages[1], 1);
+ ret = set_memory_encrypted((unsigned long)vmbus_connection.monitor_pages[0], 1);
+ ret |= set_memory_encrypted((unsigned long)vmbus_connection.monitor_pages[1], 1);
- hv_free_hyperv_page(vmbus_connection.monitor_pages[0]);
- hv_free_hyperv_page(vmbus_connection.monitor_pages[1]);
+ if (!ret) {
+ hv_free_hyperv_page(vmbus_connection.monitor_pages[0]);
+ hv_free_hyperv_page(vmbus_connection.monitor_pages[1]);
+ }
vmbus_connection.monitor_pages[0] = NULL;
vmbus_connection.monitor_pages[1] = NULL;
}
--
2.34.1
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.
uio_hv_generic could free decrypted/shared pages if
set_memory_decrypted() fails.
Check the decrypted field in the gpadl before freeing in order to not
leak the memory.
Only compile tested.
Cc: "K. Y. Srinivasan" <[email protected]>
Cc: Haiyang Zhang <[email protected]>
Cc: Wei Liu <[email protected]>
Cc: Dexuan Cui <[email protected]>
Cc: [email protected]
Signed-off-by: Rick Edgecombe <[email protected]>
---
drivers/uio/uio_hv_generic.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
index 20d9762331bd..6be3462b109f 100644
--- a/drivers/uio/uio_hv_generic.c
+++ b/drivers/uio/uio_hv_generic.c
@@ -181,12 +181,14 @@ hv_uio_cleanup(struct hv_device *dev, struct hv_uio_private_data *pdata)
{
if (pdata->send_gpadl.gpadl_handle) {
vmbus_teardown_gpadl(dev->channel, &pdata->send_gpadl);
- vfree(pdata->send_buf);
+ if (!pdata->send_gpadl.decrypted)
+ vfree(pdata->send_buf);
}
if (pdata->recv_gpadl.gpadl_handle) {
vmbus_teardown_gpadl(dev->channel, &pdata->recv_gpadl);
- vfree(pdata->recv_buf);
+ if (!pdata->recv_gpadl.decrypted)
+ vfree(pdata->recv_buf);
}
}
@@ -295,7 +297,8 @@ hv_uio_probe(struct hv_device *dev,
ret = vmbus_establish_gpadl(channel, pdata->recv_buf,
RECV_BUFFER_SIZE, &pdata->recv_gpadl);
if (ret) {
- vfree(pdata->recv_buf);
+ if (!pdata->recv_gpadl.decrypted)
+ vfree(pdata->recv_buf);
goto fail_close;
}
@@ -317,7 +320,8 @@ hv_uio_probe(struct hv_device *dev,
ret = vmbus_establish_gpadl(channel, pdata->send_buf,
SEND_BUFFER_SIZE, &pdata->send_gpadl);
if (ret) {
- vfree(pdata->send_buf);
+ if (!pdata->send_gpadl.decrypted)
+ vfree(pdata->send_buf);
goto fail_close;
}
--
2.34.1
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.
hv_nstvsc could free decrypted/shared pages if set_memory_decrypted()
fails. Check the decrypted field in the gpadl before freeing in order to
not leak the memory.
Only compile tested.
Cc: "K. Y. Srinivasan" <[email protected]>
Cc: Haiyang Zhang <[email protected]>
Cc: Wei Liu <[email protected]>
Cc: Dexuan Cui <[email protected]>
Cc: [email protected]
Signed-off-by: Rick Edgecombe <[email protected]>
---
drivers/net/hyperv/netvsc.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index a6fcbda64ecc..2b6ec979a62f 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -154,8 +154,11 @@ static void free_netvsc_device(struct rcu_head *head)
int i;
kfree(nvdev->extension);
- vfree(nvdev->recv_buf);
- vfree(nvdev->send_buf);
+
+ if (!nvdev->recv_buf_gpadl_handle.decrypted)
+ vfree(nvdev->recv_buf);
+ if (!nvdev->send_buf_gpadl_handle.decrypted)
+ vfree(nvdev->send_buf);
bitmap_free(nvdev->send_section_map);
for (i = 0; i < VRSS_CHANNEL_MAX; i++) {
--
2.34.1
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" <[email protected]>
Cc: Haiyang Zhang <[email protected]>
Cc: Wei Liu <[email protected]>
Cc: Dexuan Cui <[email protected]>
Cc: [email protected]
Signed-off-by: Rick Edgecombe <[email protected]>
---
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;
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 {
--
2.34.1
On Wed, Feb 21, 2024 at 06:10:03PM -0800, Rick Edgecombe wrote:
> 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.
>
> Hyperv could free decrypted/shared pages if set_memory_encrypted() fails.
"Hyper-V" throughout.
> Leak the pages if this happens.
>
> Only compile tested.
>
> Cc: "K. Y. Srinivasan" <[email protected]>
> Cc: Haiyang Zhang <[email protected]>
> Cc: Wei Liu <[email protected]>
> Cc: Dexuan Cui <[email protected]>
> Cc: [email protected]
> Signed-off-by: Rick Edgecombe <[email protected]>
> ---
> drivers/hv/connection.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> index 3cabeeabb1ca..e39493421bbb 100644
> --- a/drivers/hv/connection.c
> +++ b/drivers/hv/connection.c
> @@ -315,6 +315,7 @@ int vmbus_connect(void)
>
> void vmbus_disconnect(void)
> {
> + int ret;
> /*
> * First send the unload request to the host.
> */
> @@ -337,11 +338,13 @@ void vmbus_disconnect(void)
> vmbus_connection.int_page = NULL;
> }
>
> - set_memory_encrypted((unsigned long)vmbus_connection.monitor_pages[0], 1);
> - set_memory_encrypted((unsigned long)vmbus_connection.monitor_pages[1], 1);
> + ret = set_memory_encrypted((unsigned long)vmbus_connection.monitor_pages[0], 1);
> + ret |= set_memory_encrypted((unsigned long)vmbus_connection.monitor_pages[1], 1);
>
> - hv_free_hyperv_page(vmbus_connection.monitor_pages[0]);
> - hv_free_hyperv_page(vmbus_connection.monitor_pages[1]);
> + if (!ret) {
> + hv_free_hyperv_page(vmbus_connection.monitor_pages[0]);
> + hv_free_hyperv_page(vmbus_connection.monitor_pages[1]);
> + }
This silently leaks the pages if set_memory_encrypted() fails. I think
there should print some warning or error messages here.
Thanks,
Wei.
> vmbus_connection.monitor_pages[0] = NULL;
> vmbus_connection.monitor_pages[1] = NULL;
> }
> --
> 2.34.1
>
From: Rick Edgecombe <[email protected]> Sent: Wednesday, February 21, 2024 6:10 PM
>
Historically, the preferred Subject prefix for changes to connection.c has
been "Drivers: hv: vmbus:", not just "hv:". Sometimes that preference
isn't followed, but most of the time it is.
> On TDX it is possible for the untrusted host to cause
I'd argue that this is for CoCo VMs in general, not just TDX. I don't know
all the failure modes for SEV-SNP, but the code paths you are changing
are run in both TDX and SEV-SNP CoCo VMs.
> 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.
>
> Hyperv could free decrypted/shared pages if set_memory_encrypted() fails.
It's not Hyper-V doing the freeing. Maybe say "VMBus code could
free ...."
> Leak the pages if this happens.
>
> Only compile tested.
>
> Cc: "K. Y. Srinivasan" <[email protected]>
> Cc: Haiyang Zhang <[email protected]>
> Cc: Wei Liu <[email protected]>
> Cc: Dexuan Cui <[email protected]>
> Cc: [email protected]
> Signed-off-by: Rick Edgecombe <[email protected]>
> ---
> drivers/hv/connection.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> index 3cabeeabb1ca..e39493421bbb 100644
> --- a/drivers/hv/connection.c
> +++ b/drivers/hv/connection.c
> @@ -315,6 +315,7 @@ int vmbus_connect(void)
>
> void vmbus_disconnect(void)
> {
> + int ret;
> /*
> * First send the unload request to the host.
> */
> @@ -337,11 +338,13 @@ void vmbus_disconnect(void)
> vmbus_connection.int_page = NULL;
> }
>
> - set_memory_encrypted((unsigned long)vmbus_connection.monitor_pages[0], 1);
> - set_memory_encrypted((unsigned long)vmbus_connection.monitor_pages[1], 1);
> + ret = set_memory_encrypted((unsigned long)vmbus_connection.monitor_pages[0], 1);
> + ret |= set_memory_encrypted((unsigned long)vmbus_connection.monitor_pages[1], 1);
>
> - hv_free_hyperv_page(vmbus_connection.monitor_pages[0]);
> - hv_free_hyperv_page(vmbus_connection.monitor_pages[1]);
> + if (!ret) {
> + hv_free_hyperv_page(vmbus_connection.monitor_pages[0]);
> + hv_free_hyperv_page(vmbus_connection.monitor_pages[1]);
> + }
Of course, this will leak the memory for both pages if only one of the
set_memory_encrypted() calls fails, but I'm OK with that. It doesn't
seem worth the additional complexity to treat each page separately.
> vmbus_connection.monitor_pages[0] = NULL;
> vmbus_connection.monitor_pages[1] = NULL;
> }
> --
> 2.34.1
From: Rick Edgecombe <[email protected]> Sent: Wednesday, February 21, 2024 6:10 PM
>
> Shared (decrypted) pages should never return to the page allocator, or
> future usage of the pages may allow for the contents to be exposed to the
> host. They may also cause the guest to crash if the page is used in way
> disallowed by HW (i.e. for executable code or as a page table).
>
> Normally set_memory() call failures are rare. But on TDX
> set_memory_XXcrypted() involves calls to the untrusted VMM, and an
> attacker
> could fail these calls such that:
> 1. set_memory_encrypted() returns an error and leaves the pages fully
> shared.
> 2. set_memory_decrypted() returns an error, but the pages are actually
> full converted to shared.
>
> This means that patterns like the below can cause problems:
> void *addr = alloc();
> int fail = set_memory_decrypted(addr, 1);
> if (fail)
> free_pages(addr, 0);
>
> And:
> void *addr = alloc();
> int fail = set_memory_decrypted(addr, 1);
> if (fail) {
> set_memory_encrypted(addr, 1);
> free_pages(addr, 0);
> }
>
> Unfortunately these patterns appear in the kernel. And what the
> set_memory() callers should do in this situation is not clear either. They
> shouldn't use them as shared because something clearly went wrong, but
> they also need to fully reset the pages to private to free them. But, the
> kernel needs the VMMs help to do this and the VMM is already being
> uncooperative around the needed operations. So this isn't guaranteed to
> succeed and the caller is kind of stuck with unusable pages.
>
> The only choice is to panic or leak the pages. The kernel tries not to
> panic if at all possible, so just leak the pages at the call sites.
> Separately there is a patch[0] to warn if the guest detects strange VMM
> behavior around this. It is stalled, so in the mean time I'm proceeding
> with fixing the callers to leak the pages. No additional warnings are
> added, because the plan is to warn in a single place in x86 set_memory()
> code.
>
> This series fixes the cases in the hyperv code.
>
> IMPORTANT NOTE:
> I don't have a setup to test tdx hyperv changes. These changes are compile
> tested only. Previously Michael Kelley suggested some folks at MS might be
> able to help with this.
Thanks for doing these changes. Overall they look pretty good,
modulo a few comments. The "decrypted" flag in the vmbus_gpadl
structure is a good way to keep track of the encryption status of
the associated memory.
The memory passed to the gpadl (Guest Physical Address Descriptor
List) functions may allocated and freed directly by the driver, as in
the netvsc and UIO cases. You've handled that case. But memory
may also be allocated by vmbus_alloc_ring() and freed by
vmbus_free_ring(). Your patch set needs an additional change
to check the "decrypted" flag in vmbus_free_ring().
In reviewing the code, I also see some unrelated memory freeing
issues in error paths. They are outside the scope of your changes.
I'll make a note of these for future fixing.
For testing, I'll do two things:
1) Verify that the non-error paths still work correctly with the
changes. That should be relatively straightforward as the
changes are pretty much confined to the error paths.
2) Hack set_memory_encrypted() to always fail. I hope Linux
still boots in that case, but just leaks some memory. Then if
I unbind a Hyper-V synthetic device, that should exercise the
path where set_memory_encrypted() is called. Failures
should be handled cleanly, albeit while leaking the memory.
I should be able to test in a normal VM, a TDX VM, and an
SEV-SNP VM.
I have a few more detailed comments in the individual
patches of this series.
Michael
>
> [0] https://lore.kernel.org/lkml/[email protected]/
>
> Rick Edgecombe (4):
> hv: Leak pages if set_memory_encrypted() fails
> hv: Track decrypted status in vmbus_gpadl
> hv_nstvsc: Don't free decrypted memory
> uio_hv_generic: Don't free decrypted memory
>
> drivers/hv/channel.c | 11 ++++++++---
> drivers/hv/connection.c | 11 +++++++----
> drivers/net/hyperv/netvsc.c | 7 +++++--
> drivers/uio/uio_hv_generic.c | 12 ++++++++----
> include/linux/hyperv.h | 1 +
> 5 files changed, 29 insertions(+), 13 deletions(-)
>
> --
> 2.34.1
From: Rick Edgecombe <[email protected]> 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" <[email protected]>
> Cc: Haiyang Zhang <[email protected]>
> Cc: Wei Liu <[email protected]>
> Cc: Dexuan Cui <[email protected]>
> Cc: [email protected]
> Signed-off-by: Rick Edgecombe <[email protected]>
> ---
> 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
From: Rick Edgecombe <[email protected]> Sent: Wednesday, February 21, 2024 6:10 PM
>
"Subject:" prefix should be hv_netvsc:
> On TDX it is possible for the untrusted host to cause
Same comment 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.
>
> hv_nstvsc could free decrypted/shared pages if set_memory_decrypted()
s/hv_nstvsc/hv_netvsc/
> fails. Check the decrypted field in the gpadl before freeing in order to
> not leak the memory.
>
> Only compile tested.
>
> Cc: "K. Y. Srinivasan" <[email protected]>
> Cc: Haiyang Zhang <[email protected]>
> Cc: Wei Liu <[email protected]>
> Cc: Dexuan Cui <[email protected]>
> Cc: [email protected]
> Signed-off-by: Rick Edgecombe <[email protected]>
> ---
> drivers/net/hyperv/netvsc.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index a6fcbda64ecc..2b6ec979a62f 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -154,8 +154,11 @@ static void free_netvsc_device(struct rcu_head
> *head)
> int i;
>
> kfree(nvdev->extension);
> - vfree(nvdev->recv_buf);
> - vfree(nvdev->send_buf);
> +
> + if (!nvdev->recv_buf_gpadl_handle.decrypted)
> + vfree(nvdev->recv_buf);
> + if (!nvdev->send_buf_gpadl_handle.decrypted)
> + vfree(nvdev->send_buf);
> bitmap_free(nvdev->send_section_map);
>
> for (i = 0; i < VRSS_CHANNEL_MAX; i++) {
> --
> 2.34.1
From: Rick Edgecombe <[email protected]> Sent: Wednesday, February 21, 2024 6:10 PM
>
> On TDX it is possible for the untrusted host to cause
Same comment 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.
>
> uio_hv_generic could free decrypted/shared pages if
> set_memory_decrypted() fails.
>
> Check the decrypted field in the gpadl before freeing in order to not
> leak the memory.
>
> Only compile tested.
>
> Cc: "K. Y. Srinivasan" <[email protected]>
> Cc: Haiyang Zhang <[email protected]>
> Cc: Wei Liu <[email protected]>
> Cc: Dexuan Cui <[email protected]>
> Cc: [email protected]
> Signed-off-by: Rick Edgecombe <[email protected]>
> ---
> drivers/uio/uio_hv_generic.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
> index 20d9762331bd..6be3462b109f 100644
> --- a/drivers/uio/uio_hv_generic.c
> +++ b/drivers/uio/uio_hv_generic.c
> @@ -181,12 +181,14 @@ hv_uio_cleanup(struct hv_device *dev, struct
> hv_uio_private_data *pdata)
> {
> if (pdata->send_gpadl.gpadl_handle) {
> vmbus_teardown_gpadl(dev->channel, &pdata->send_gpadl);
> - vfree(pdata->send_buf);
> + if (!pdata->send_gpadl.decrypted)
> + vfree(pdata->send_buf);
> }
>
> if (pdata->recv_gpadl.gpadl_handle) {
> vmbus_teardown_gpadl(dev->channel, &pdata->recv_gpadl);
> - vfree(pdata->recv_buf);
> + if (!pdata->recv_gpadl.decrypted)
> + vfree(pdata->recv_buf);
> }
> }
>
> @@ -295,7 +297,8 @@ hv_uio_probe(struct hv_device *dev,
> ret = vmbus_establish_gpadl(channel, pdata->recv_buf,
> RECV_BUFFER_SIZE, &pdata->recv_gpadl);
> if (ret) {
> - vfree(pdata->recv_buf);
> + if (!pdata->recv_gpadl.decrypted)
> + vfree(pdata->recv_buf);
> goto fail_close;
> }
>
> @@ -317,7 +320,8 @@ hv_uio_probe(struct hv_device *dev,
> ret = vmbus_establish_gpadl(channel, pdata->send_buf,
> SEND_BUFFER_SIZE, &pdata->send_gpadl);
> if (ret) {
> - vfree(pdata->send_buf);
> + if (!pdata->send_gpadl.decrypted)
> + vfree(pdata->send_buf);
> goto fail_close;
> }
>
> --
> 2.34.1
On Fri, 2024-03-01 at 09:26 +0000, Wei Liu wrote:
> > Hyperv could free decrypted/shared pages if set_memory_encrypted()
> > fails.
>
> "Hyper-V" throughout.
Ok.
>
> > Leak the pages if this happens.
> >
> > Only compile tested.
> >
> > Cc: "K. Y. Srinivasan" <[email protected]>
> > Cc: Haiyang Zhang <[email protected]>
> > Cc: Wei Liu <[email protected]>
> > Cc: Dexuan Cui <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Rick Edgecombe <[email protected]>
> > ---
> > drivers/hv/connection.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> > index 3cabeeabb1ca..e39493421bbb 100644
> > --- a/drivers/hv/connection.c
> > +++ b/drivers/hv/connection.c
> > @@ -315,6 +315,7 @@ int vmbus_connect(void)
> >
> > void vmbus_disconnect(void)
> > {
> > + int ret;
> > /*
> > * First send the unload request to the host.
> > */
> > @@ -337,11 +338,13 @@ void vmbus_disconnect(void)
> > vmbus_connection.int_page = NULL;
> > }
> >
> > - set_memory_encrypted((unsigned
> > long)vmbus_connection.monitor_pages[0], 1);
> > - set_memory_encrypted((unsigned
> > long)vmbus_connection.monitor_pages[1], 1);
> > + ret = set_memory_encrypted((unsigned
> > long)vmbus_connection.monitor_pages[0], 1);
> > + ret |= set_memory_encrypted((unsigned
> > long)vmbus_connection.monitor_pages[1], 1);
> >
> > - hv_free_hyperv_page(vmbus_connection.monitor_pages[0]);
> > - hv_free_hyperv_page(vmbus_connection.monitor_pages[1]);
> > + if (!ret) {
> > + hv_free_hyperv_page(vmbus_connection.monitor_pages[
> > 0]);
> > + hv_free_hyperv_page(vmbus_connection.monitor_pages[
> > 1]);
> > + }
>
> This silently leaks the pages if set_memory_encrypted() fails. I
> think
> there should print some warning or error messages here.
Another patch will warn in CPA for the particular case:
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/mm
Do we want a warning in the caller too? It is more robust to other
types of failures in the future I guess.
On Fri, 2024-03-01 at 19:00 +0000, Michael Kelley wrote:
> From: Rick Edgecombe <[email protected]> Sent: Wednesday,
> February 21, 2024 6:10 PM
> >
>
> Historically, the preferred Subject prefix for changes to
> connection.c has
> been "Drivers: hv: vmbus:", not just "hv:". Sometimes that
> preference
> isn't followed, but most of the time it is.
Ok, I can update it.
>
> > On TDX it is possible for the untrusted host to cause
>
> I'd argue that this is for CoCo VMs in general, not just TDX. I
> don't know
> all the failure modes for SEV-SNP, but the code paths you are
> changing
> are run in both TDX and SEV-SNP CoCo VMs.
On SEV-SNP the host can cause the call to fail too was my
understanding. But in Linux, that side panics and never gets to the
point of being able to free the shared memory. So it's not TDX
architecture specific, it's just how Linux handles it on the different
sids. For TDX the suggestion was to avoid panicing because it is
possible to handle in SW, as Linux usually tries it's best to do.
>
> > 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.
> >
> > Hyperv could free decrypted/shared pages if set_memory_encrypted()
> > fails.
>
> It's not Hyper-V doing the freeing. Maybe say "VMBus code could
> free ...."
Ok.
From: Edgecombe, Rick P <[email protected]> Sent: Friday, March 1, 2024 11:13 AM
> >
> > > On TDX it is possible for the untrusted host to cause
> >
> > I'd argue that this is for CoCo VMs in general, not just TDX. I don't know
> > all the failure modes for SEV-SNP, but the code paths you are changing
> > are run in both TDX and SEV-SNP CoCo VMs.
>
> On SEV-SNP the host can cause the call to fail too was my
> understanding. But in Linux, that side panics and never gets to the
> point of being able to free the shared memory. So it's not TDX
> architecture specific, it's just how Linux handles it on the different
> sids. For TDX the suggestion was to avoid panicing because it is
> possible to handle in SW, as Linux usually tries it's best to do.
>
The Hyper-V case can actually be a third path when a paravisor
is being used. In that case, for both TDX and SEV-SNP, the
hypervisor callbacks in __set_memory_enc_pgtable() go
to Hyper-V specific functions that talk to the paravisor. Those
callbacks never panic. After a failure, either at the paravisor
level or in the paravisor talking to the hypervisor/VMM, the
decrypted/encrypted state of the memory isn't known. So
leaking the memory is still the right thing to do, and your
patch set is good. But in the Hyper-V with paravisor case,
the leaking is applicable more broadly than just TDX.
The text in the commit message isn't something that I'll
go to the mat over. But I wanted to offer the slightly broader
perspective.
Michael
On Fri, 2024-03-01 at 20:21 +0000, Michael Kelley wrote:
>
> The Hyper-V case can actually be a third path when a paravisor
> is being used. In that case, for both TDX and SEV-SNP, the
> hypervisor callbacks in __set_memory_enc_pgtable() go
> to Hyper-V specific functions that talk to the paravisor. Those
> callbacks never panic. After a failure, either at the paravisor
> level or in the paravisor talking to the hypervisor/VMM, the
> decrypted/encrypted state of the memory isn't known. So
> leaking the memory is still the right thing to do, and your
> patch set is good. But in the Hyper-V with paravisor case,
> the leaking is applicable more broadly than just TDX.
>
> The text in the commit message isn't something that I'll
> go to the mat over. But I wanted to offer the slightly broader
> perspective.
Oh, interesting. I think I missed it because it only has a special
enc_status_change_finish(). But yea. It does sound like the text you
suggested is more accurate. I'll update it.
From: Michael Kelley <[email protected]> Sent: Friday, March 1, 2024 11:00 AM
> >
> > IMPORTANT NOTE:
> > I don't have a setup to test tdx hyperv changes. These changes are compile
> > tested only. Previously Michael Kelley suggested some folks at MS might be
> > able to help with this.
>
> Thanks for doing these changes. Overall they look pretty good,
> modulo a few comments. The "decrypted" flag in the vmbus_gpadl
> structure is a good way to keep track of the encryption status of
> the associated memory.
>
> The memory passed to the gpadl (Guest Physical Address Descriptor
> List) functions may allocated and freed directly by the driver, as in
> the netvsc and UIO cases. You've handled that case. But memory
> may also be allocated by vmbus_alloc_ring() and freed by
> vmbus_free_ring(). Your patch set needs an additional change
> to check the "decrypted" flag in vmbus_free_ring().
>
> In reviewing the code, I also see some unrelated memory freeing
> issues in error paths. They are outside the scope of your changes.
> I'll make a note of these for future fixing.
>
> For testing, I'll do two things:
>
> 1) Verify that the non-error paths still work correctly with the
> changes. That should be relatively straightforward as the
> changes are pretty much confined to the error paths.
>
> 2) Hack set_memory_encrypted() to always fail. I hope Linux
> still boots in that case, but just leaks some memory. Then if
> I unbind a Hyper-V synthetic device, that should exercise the
> path where set_memory_encrypted() is called. Failures
> should be handled cleanly, albeit while leaking the memory.
>
> I should be able to test in a normal VM, a TDX VM, and an
> SEV-SNP VM.
>
Rick --
Using your patches plus the changes in my comments, I've
done most of the testing described above. The normal
paths work, and when I hack set_memory_encrypted()
to fail, the error paths correctly did not free the memory.
I checked both the ring buffer memory and the additional
vmalloc memory allocated by the netvsc driver and the uio
driver. The memory status can be checked after-the-fact
via /proc/vmmallocinfo and /proc/buddyinfo since these
are mostly large allocations. As expected, the drivers
output their own error messages after the failures to
teardown the GPADLs.
I did not test the vmbus_disconnect() path since that
effectively kills the VM.
I tested in a normal VM, and in an SEV-SNP VM. I didn't
specifically test in a TDX VM, but given that Hyper-V CoCo
guests run with a paravisor, the guest sees the same thing
either way.
Michael
On Thu, 2024-03-07 at 17:11 +0000, Michael Kelley wrote:
> Using your patches plus the changes in my comments, I've
> done most of the testing described above. The normal
> paths work, and when I hack set_memory_encrypted()
> to fail, the error paths correctly did not free the memory.
> I checked both the ring buffer memory and the additional
> vmalloc memory allocated by the netvsc driver and the uio
> driver. The memory status can be checked after-the-fact
> via /proc/vmmallocinfo and /proc/buddyinfo since these
> are mostly large allocations. As expected, the drivers
> output their own error messages after the failures to
> teardown the GPADLs.
>
> I did not test the vmbus_disconnect() path since that
> effectively kills the VM.
>
> I tested in a normal VM, and in an SEV-SNP VM. I didn't
> specifically test in a TDX VM, but given that Hyper-V CoCo
> guests run with a paravisor, the guest sees the same thing
> either way.
Thanks Michael! How would you feel about reposting the patches with
your changes added? I think you have a very good handle on the part of
the problem I understand, and additionally much more familiarity with
these drivers.
From: Edgecombe, Rick P <[email protected]> Sent: Thursday, March 7, 2024 11:12 AM
>
> On Thu, 2024-03-07 at 17:11 +0000, Michael Kelley wrote:
> > Using your patches plus the changes in my comments, I've
> > done most of the testing described above. The normal
> > paths work, and when I hack set_memory_encrypted()
> > to fail, the error paths correctly did not free the memory.
> > I checked both the ring buffer memory and the additional
> > vmalloc memory allocated by the netvsc driver and the uio
> > driver. The memory status can be checked after-the-fact
> > via /proc/vmmallocinfo and /proc/buddyinfo since these
> > are mostly large allocations. As expected, the drivers
> > output their own error messages after the failures to
> > teardown the GPADLs.
> >
> > I did not test the vmbus_disconnect() path since that
> > effectively kills the VM.
> >
> > I tested in a normal VM, and in an SEV-SNP VM. I didn't
> > specifically test in a TDX VM, but given that Hyper-V CoCo
> > guests run with a paravisor, the guest sees the same thing
> > either way.
>
> Thanks Michael! How would you feel about reposting the patches with
> your changes added? I think you have a very good handle on the part of
> the problem I understand, and additionally much more familiarity with
> these drivers.
Yes, I can submit a new version.
Michael