[v1] x86/tdx: Preserve shared bit on mprotect()

[PATCH] x86/tdx: Preserve shared bit on mprotect()

Posted by Kirill A. Shutemov 1 year, 10 months ago

The TDX guest platform takes one bit from the physical address to
indicate if the page is shared (accessible by VMM). This bit is not part
of the physical_mask and is not preserved during mprotect(). As a
result, the 'shared' bit is lost during mprotect() on shared mappings.

_COMMON_PAGE_CHG_MASK specifies which PTE bits need to be preserved
during modification. AMD includes 'sme_me_mask' in the define to
preserve the 'encrypt' bit.

To cover both Intel and AMD cases, include 'cc_mask' in
_COMMON_PAGE_CHG_MASK instead of 'sme_me_mask'.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Fixes: 41394e33f3a0 ("x86/tdx: Extend the confidential computing API to support TDX guests")
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Chris Oo <cho@microsoft.com>
Cc: Dexuan Cui <decui@microsoft.com>
---
 arch/x86/include/asm/pgtable_types.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 3f648ffdfbe5..7dd2fdfacff3 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -148,7 +148,7 @@
 #define _COMMON_PAGE_CHG_MASK	(PTE_PFN_MASK | _PAGE_PCD | _PAGE_PWT |	\
 				 _PAGE_SPECIAL | _PAGE_ACCESSED |	\
 				 _PAGE_DIRTY_BITS | _PAGE_SOFT_DIRTY |	\
-				 _PAGE_DEVMAP | _PAGE_ENC | _PAGE_UFFD_WP)
+				 _PAGE_DEVMAP | _PAGE_CC | _PAGE_UFFD_WP)
 #define _PAGE_CHG_MASK	(_COMMON_PAGE_CHG_MASK | _PAGE_PAT)
 #define _HPAGE_CHG_MASK (_COMMON_PAGE_CHG_MASK | _PAGE_PSE | _PAGE_PAT_LARGE)
 
@@ -173,6 +173,7 @@ enum page_cache_mode {
 };
 #endif
 
+#define _PAGE_CC		(_AT(pteval_t, cc_mask))
 #define _PAGE_ENC		(_AT(pteval_t, sme_me_mask))
 
 #define _PAGE_CACHE_MASK	(_PAGE_PWT | _PAGE_PCD | _PAGE_PAT)
-- 
2.43.0

Re: [PATCH] x86/tdx: Preserve shared bit on mprotect()

Posted by Kuppuswamy Sathyanarayanan 1 year, 9 months ago

On 4/12/24 12:12 PM, Kirill A. Shutemov wrote:
> The TDX guest platform takes one bit from the physical address to
> indicate if the page is shared (accessible by VMM). This bit is not part
> of the physical_mask and is not preserved during mprotect(). As a
> result, the 'shared' bit is lost during mprotect() on shared mappings.
>
> _COMMON_PAGE_CHG_MASK specifies which PTE bits need to be preserved
> during modification. AMD includes 'sme_me_mask' in the define to
> preserve the 'encrypt' bit.
>
> To cover both Intel and AMD cases, include 'cc_mask' in
> _COMMON_PAGE_CHG_MASK instead of 'sme_me_mask'.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Fixes: 41394e33f3a0 ("x86/tdx: Extend the confidential computing API to support TDX guests")
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Chris Oo <cho@microsoft.com>
> Cc: Dexuan Cui <decui@microsoft.com>
> ---

Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>  arch/x86/include/asm/pgtable_types.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
> index 3f648ffdfbe5..7dd2fdfacff3 100644
> --- a/arch/x86/include/asm/pgtable_types.h
> +++ b/arch/x86/include/asm/pgtable_types.h
> @@ -148,7 +148,7 @@
>  #define _COMMON_PAGE_CHG_MASK	(PTE_PFN_MASK | _PAGE_PCD | _PAGE_PWT |	\
>  				 _PAGE_SPECIAL | _PAGE_ACCESSED |	\
>  				 _PAGE_DIRTY_BITS | _PAGE_SOFT_DIRTY |	\
> -				 _PAGE_DEVMAP | _PAGE_ENC | _PAGE_UFFD_WP)
> +				 _PAGE_DEVMAP | _PAGE_CC | _PAGE_UFFD_WP)
>  #define _PAGE_CHG_MASK	(_COMMON_PAGE_CHG_MASK | _PAGE_PAT)
>  #define _HPAGE_CHG_MASK (_COMMON_PAGE_CHG_MASK | _PAGE_PSE | _PAGE_PAT_LARGE)
>  
> @@ -173,6 +173,7 @@ enum page_cache_mode {
>  };
>  #endif
>  
> +#define _PAGE_CC		(_AT(pteval_t, cc_mask))
>  #define _PAGE_ENC		(_AT(pteval_t, sme_me_mask))
>  
>  #define _PAGE_CACHE_MASK	(_PAGE_PWT | _PAGE_PCD | _PAGE_PAT)

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH] x86/tdx: Preserve shared bit on mprotect()

Posted by Edgecombe, Rick P 1 year, 10 months ago

On Fri, 2024-04-12 at 22:12 +0300, Kirill A. Shutemov wrote:
> The TDX guest platform takes one bit from the physical address to
> indicate if the page is shared (accessible by VMM). This bit is not part
> of the physical_mask and is not preserved during mprotect(). As a
> result, the 'shared' bit is lost during mprotect() on shared mappings.
> 
> _COMMON_PAGE_CHG_MASK specifies which PTE bits need to be preserved
> during modification. AMD includes 'sme_me_mask' in the define to
> preserve the 'encrypt' bit.
> 
> To cover both Intel and AMD cases, include 'cc_mask' in
> _COMMON_PAGE_CHG_MASK instead of 'sme_me_mask'.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Fixes: 41394e33f3a0 ("x86/tdx: Extend the confidential computing API to
> support TDX guests")
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Chris Oo <cho@microsoft.com>
> Cc: Dexuan Cui <decui@microsoft.com>

Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

So does this mean there is shared memory mapped to userspace? Or is this a
theoretical correctness thing?

Re: [PATCH] x86/tdx: Preserve shared bit on mprotect()

Posted by kirill.shutemov@linux.intel.com 1 year, 10 months ago

On Fri, Apr 12, 2024 at 08:48:56PM +0000, Edgecombe, Rick P wrote:
> On Fri, 2024-04-12 at 22:12 +0300, Kirill A. Shutemov wrote:
> > The TDX guest platform takes one bit from the physical address to
> > indicate if the page is shared (accessible by VMM). This bit is not part
> > of the physical_mask and is not preserved during mprotect(). As a
> > result, the 'shared' bit is lost during mprotect() on shared mappings.
> > 
> > _COMMON_PAGE_CHG_MASK specifies which PTE bits need to be preserved
> > during modification. AMD includes 'sme_me_mask' in the define to
> > preserve the 'encrypt' bit.
> > 
> > To cover both Intel and AMD cases, include 'cc_mask' in
> > _COMMON_PAGE_CHG_MASK instead of 'sme_me_mask'.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Fixes: 41394e33f3a0 ("x86/tdx: Extend the confidential computing API to
> > support TDX guests")
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Chris Oo <cho@microsoft.com>
> > Cc: Dexuan Cui <decui@microsoft.com>
> 
> Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> 
> So does this mean there is shared memory mapped to userspace? Or is this a
> theoretical correctness thing?

Drivers can do this. Things like VFIO, I guess.

I think I should have credited Chris for reporting and testing the problem:

Reported-and-tested-by: Chris Oo <cho@microsoft.com>

-- 
  Kiryl Shutsemau / Kirill A. Shutemov