2024-04-19 20:14:13

by Kirill A. Shutemov

[permalink] [raw]
Subject: [PATCHv2] x86/tdx: Preserve shared bit on mprotect()

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 <[email protected]>
Fixes: 41394e33f3a0 ("x86/tdx: Extend the confidential computing API to support TDX guests")
Reported-and-tested-by: Chris Oo <[email protected]>
Reviewed-by: Rick Edgecombe <[email protected]>
Reviewed-by: Kuppuswamy Sathyanarayanan <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Dexuan Cui <[email protected]>
---

v2:
- Fix build for !CONFIG_ARCH_HAS_CC_PLATFORM

---
arch/x86/include/asm/coco.h | 1 +
arch/x86/include/asm/pgtable_types.h | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
index c086699b0d0c..ac8cd4447d48 100644
--- a/arch/x86/include/asm/coco.h
+++ b/arch/x86/include/asm/coco.h
@@ -25,6 +25,7 @@ u64 cc_mkdec(u64 val);
void cc_random_init(void);
#else
#define cc_vendor (CC_VENDOR_NONE)
+#define cc_mask 0

static inline u64 cc_mkenc(u64 val)
{
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 8857d811fb5d..2f321137736c 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



2024-04-22 19:46:49

by Tom Lendacky

[permalink] [raw]
Subject: Re: [PATCHv2] x86/tdx: Preserve shared bit on mprotect()

On 4/19/24 15:13, 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 <[email protected]>
> Fixes: 41394e33f3a0 ("x86/tdx: Extend the confidential computing API to support TDX guests")
> Reported-and-tested-by: Chris Oo <[email protected]>
> Reviewed-by: Rick Edgecombe <[email protected]>
> Reviewed-by: Kuppuswamy Sathyanarayanan <[email protected]>
> Cc: Tom Lendacky <[email protected]>

Appears to be no functional change on the AMD side.

Reviewed-by: Tom Lendacky <[email protected]>

> Cc: Dexuan Cui <[email protected]>
> ---
>
> v2:
> - Fix build for !CONFIG_ARCH_HAS_CC_PLATFORM
>
> ---
> arch/x86/include/asm/coco.h | 1 +
> arch/x86/include/asm/pgtable_types.h | 3 ++-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
> index c086699b0d0c..ac8cd4447d48 100644
> --- a/arch/x86/include/asm/coco.h
> +++ b/arch/x86/include/asm/coco.h
> @@ -25,6 +25,7 @@ u64 cc_mkdec(u64 val);
> void cc_random_init(void);
> #else
> #define cc_vendor (CC_VENDOR_NONE)
> +#define cc_mask 0
>
> static inline u64 cc_mkenc(u64 val)
> {
> diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
> index 8857d811fb5d..2f321137736c 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)

2024-04-22 20:18:37

by Dave Hansen

[permalink] [raw]
Subject: Re: [PATCHv2] x86/tdx: Preserve shared bit on mprotect()

On 4/22/24 12:46, Tom Lendacky wrote:
> Appears to be no functional change on the AMD side.
>
> Reviewed-by: Tom Lendacky <[email protected]>

Thanks a bunch for that! I was just noodling over this one and that was
one of my worries.

2024-04-23 14:32:53

by Kirill A. Shutemov

[permalink] [raw]
Subject: Re: [PATCHv2] x86/tdx: Preserve shared bit on mprotect()

On Mon, Apr 22, 2024 at 01:18:26PM -0700, Dave Hansen wrote:
> On 4/22/24 12:46, Tom Lendacky wrote:
> > Appears to be no functional change on the AMD side.
> >
> > Reviewed-by: Tom Lendacky <[email protected]>
>
> Thanks a bunch for that! I was just noodling over this one and that was
> one of my worries.

Please hold. 0-day found more build issues. Apparently 'cc_mask' is used in
couple of drivers for unrelated reasons and "#define cc_mask 0" breaks them.

I will send v3.

--
Kiryl Shutsemau / Kirill A. Shutemov