linux-kernel - Re: [RFC v2 27/32] x86/tdx: Exclude Shared bit from __PHYSICAL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3ae38a0b-0676-1543-7015-39a589b2807a@intel.com>
Date:   Wed, 19 May 2021 09:14:00 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        Tony Luck <tony.luck@...el.com>
Cc:     Andi Kleen <ak@...ux.intel.com>,
        Kirill Shutemov <kirill.shutemov@...ux.intel.com>,
        Kuppuswamy Sathyanarayanan <knsathya@...nel.org>,
        Raj Ashok <ashok.raj@...el.com>,
        Sean Christopherson <seanjc@...gle.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC v2 27/32] x86/tdx: Exclude Shared bit from __PHYSICAL_MASK

On 4/26/21 11:01 AM, Kuppuswamy Sathyanarayanan wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
> 
> tdx_shared_mask() returns the mask that has to be set in a page
> table entry to make page shared with VMM.

Here's a rewrite:

Just like MKTME, TDX reassigns bits of the physical address for
metadata.  MKTME used several bits for an encryption KeyID.  TDX uses a
single bit in guests to communicate whether a physical page should be
protected by TDX as private memory (bit set to 0) or unprotected and
shared with the VMM (bit set to 1).

Add a helper, tdg_shared_mask() (bad name please fix it) to generate the
mask.  The processor enumerates its physical address width to include
the shared bit, which means it gets included in __PHYSICAL_MASK by default.

Remove the shared mask from 'physical_mask' since any bits in
tdg_shared_mask() are not used for physical addresses in page table entries.

--

BTW, do you find it confusing that the subject says: '__PHYSICAL_MASK'
and yet the code only modifies 'physical_mask'?

> Also, note that we cannot club shared mapping configuration between
> AMD SME and Intel TDX Guest platforms in common function. SME has
> to do it very early in __startup_64() as it sets the bit on all
> memory, except what is used for communication. TDX can postpone as
> we don't need any shared mapping in very early boot.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> Reviewed-by: Andi Kleen <ak@...ux.intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
> ---
>  arch/x86/Kconfig           | 1 +
>  arch/x86/include/asm/tdx.h | 6 ++++++
>  arch/x86/kernel/tdx.c      | 9 +++++++++
>  3 files changed, 16 insertions(+)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 67f99bf27729..5f92e8205de2 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -882,6 +882,7 @@ config INTEL_TDX_GUEST
>  	select PARAVIRT_XL
>  	select X86_X2APIC
>  	select SECURITY_LOCKDOWN_LSM
> +	select X86_MEM_ENCRYPT_COMMON
>  	help
>  	  Provide support for running in a trusted domain on Intel processors
>  	  equipped with Trusted Domain eXtenstions. TDX is an new Intel
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index b972c6531a53..dc80cf7f7d08 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -111,6 +111,8 @@ unsigned char tdg_inb(unsigned short port);
>  unsigned short tdg_inw(unsigned short port);
>  unsigned int tdg_inl(unsigned short port);
>  
> +extern phys_addr_t tdg_shared_mask(void);
> +
>  #else // !CONFIG_INTEL_TDX_GUEST
>  
>  static inline bool is_tdx_guest(void)
> @@ -149,6 +151,10 @@ static inline long tdx_kvm_hypercall4(unsigned int nr, unsigned long p1,
>  	return -ENODEV;
>  }
>  
> +static inline phys_addr_t tdg_shared_mask(void)
> +{
> +	return 0;
> +}
>  #endif /* CONFIG_INTEL_TDX_GUEST */
>  #endif /* __ASSEMBLY__ */
>  #endif /* _ASM_X86_TDX_H */
> diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
> index 1f1bb98e1d38..7e391cd7aa2b 100644
> --- a/arch/x86/kernel/tdx.c
> +++ b/arch/x86/kernel/tdx.c
> @@ -76,6 +76,12 @@ bool is_tdx_guest(void)
>  }
>  EXPORT_SYMBOL_GPL(is_tdx_guest);
>  
> +/* The highest bit of a guest physical address is the "sharing" bit */
> +phys_addr_t tdg_shared_mask(void)
> +{
> +	return 1ULL << (td_info.gpa_width - 1);
> +}

Why not just inline this thing?  Functions don't get any smaller than
that.  Or does it not get used anywhere else?  Or are you concerned
about exporting td_info?

>  static void tdg_get_info(void)
>  {
>  	u64 ret;
> @@ -87,6 +93,9 @@ static void tdg_get_info(void)
>  
>  	td_info.gpa_width = out.rcx & GENMASK(5, 0);
>  	td_info.attributes = out.rdx;
> +
> +	/* Exclude Shared bit from the __PHYSICAL_MASK */
> +	physical_mask &= ~tdg_shared_mask();
>  }
>  
>  static __cpuidle void tdg_halt(void)
>