In CoCo VMs, it is possible for the untrusted host to cause
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. so don't free decrypted memory
Signed-off-by: Li RongQing <[email protected]>
---
drivers/virt/coco/sev-guest/sev-guest.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index 654290a8e..799563a 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -730,8 +730,7 @@ static void *alloc_shared_pages(struct device *dev, size_t sz)
ret = set_memory_decrypted((unsigned long)page_address(page), npages);
if (ret) {
- dev_err(dev, "failed to mark page shared, ret=%d\n", ret);
- __free_pages(page, get_order(sz));
+ dev_err(dev, "failed to mark page shared, leak page, ret=%d\n", ret);
return NULL;
}
--
2.9.4
On 6/14/24 00:10, Li RongQing wrote:
> In CoCo VMs, it is possible for the untrusted host to cause
> set_memory_decrypted() to fail such that an error is returned
> and the resulting memory is shared. Callers need to take care
Can you explain how it would fail or where in the call path it would fail?
Are you referring to the the Page State Change being performed by the host
but it returns a failure?
As long as the encryption bit hasn't been cleared in any of the guest
pagetables for the page range, then there should not be an issue. When the
page is referenced it will generate a #NPF and the host will have to make
that page a private page in order for forward progress to be made. But,
that page will already have been PVALIDATEd previously, so the resulting
#VC for the page no longer being PVALIDATEd will allow the guest to detect
the malicious hypervisor and terminate.
If we fail during the __change_page_attr_set_clr() call and we get a mix
of pagetable entries that could be a problem, so leaking the pages would
be best in that case.
And since the failure reason isn't clear after the call, leaking the pages
is probably the safest thing.
Thanks,
Tom
> to handle these errors to avoid returning decrypted (shared)
> memory to the page allocator, which could lead to functional
> or security issues. so don't free decrypted memory
>
> Signed-off-by: Li RongQing <[email protected]>
> ---
> drivers/virt/coco/sev-guest/sev-guest.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index 654290a8e..799563a 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -730,8 +730,7 @@ static void *alloc_shared_pages(struct device *dev, size_t sz)
>
> ret = set_memory_decrypted((unsigned long)page_address(page), npages);
> if (ret) {
> - dev_err(dev, "failed to mark page shared, ret=%d\n", ret);
> - __free_pages(page, get_order(sz));
> + dev_err(dev, "failed to mark page shared, leak page, ret=%d\n", ret);
> return NULL;
> }
>