From: Li RongQing <lirongqing@baidu.com>

If set_memory_decrypted() fails, and memory maybe have a mix of pagetable
entries, that could be a problem.

As Tom explained:
  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.

Fixes: fce96cf04430 ("virt: Add SEV-SNP guest driver")
Suggested-by: Tom Lendacky <thomas.lendacky@amd.com>
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 b699771..ce843a9 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -645,8 +645,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 it, ret=%d\n", ret);
 		return NULL;
 	}
 
-- 
2.9.4

On Mon, Jan 13, 2025 at 04:06:18PM +0800, lirongqing wrote:
> From: Li RongQing <lirongqing@baidu.com>
> 
> If set_memory_decrypted() fails, and memory maybe have a mix of pagetable
> entries, that could be a problem.
> 
> As Tom explained:
>   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.

Who fails? How do they fail? What exactly is this fixing here? Something
hypothetical or a real issue?

If latter, do explain in detail.

This commit message is raising more questions than it answers...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette