[PATCH] virt/coco/sev-guest: Don't free decrypted memory

[PATCH] virt/coco/sev-guest: Don't free decrypted memory

[Li RongQing]

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 <lirongqing@baidu.com>
---
 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

Re: [PATCH] virt/coco/sev-guest: Don't free decrypted memory

[Tom Lendacky]

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 <lirongqing@baidu.com>
> ---
>   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;
>   	}
>   

RE: [外部邮件] Re: [PATCH] virt/coco/sev-guest: Don't free decrypted memory

[Li,Rongqing]

> 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.


your explanation is very clear ; I will rewrite this changelog

thank you very much,

-Li