linux-kernel - Re: [RFC v1 25/26] x86/tdx: Make DMA pages shared

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: <cc8e2e73-9d08-894c-47f9-ec82b0642789@intel.com>
Date:   Thu, 1 Apr 2021 14:01:15 -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>
Cc:     Andi Kleen <ak@...ux.intel.com>,
        Kirill Shutemov <kirill.shutemov@...ux.intel.com>,
        Kuppuswamy Sathyanarayanan <knsathya@...nel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        Raj Ashok <ashok.raj@...el.com>,
        Sean Christopherson <seanjc@...gle.com>,
        linux-kernel@...r.kernel.org,
        Kai Huang <kai.huang@...ux.intel.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>
Subject: Re: [RFC v1 25/26] x86/tdx: Make DMA pages shared

> +int tdx_map_gpa(phys_addr_t gpa, int numpages, bool private)
> +{
> +	int ret, i;
> +
> +	ret = __tdx_map_gpa(gpa, numpages, private);
> +	if (ret || !private)
> +		return ret;
> +
> +	for (i = 0; i < numpages; i++)
> +		tdx_accept_page(gpa + i*PAGE_SIZE);
> +
> +	return 0;
> +}

Please do something like this:

enum tdx_max_type {
	TDX_MAP_PRIVATE,
	TDX_MAP_SHARED
}

Then, your calls will look like:

	tdx_map_gpa(gpa, nr, TDX_MAP_SHARED);

instead of:

	tdx_map_gpa(gpa, nr, false);

>  static __cpuidle void tdx_halt(void)
>  {
>  	register long r10 asm("r10") = TDVMCALL_STANDARD;
> diff --git a/arch/x86/mm/mem_encrypt_common.c b/arch/x86/mm/mem_encrypt_common.c
> index 964e04152417..b6d93b0c5dcf 100644
> --- a/arch/x86/mm/mem_encrypt_common.c
> +++ b/arch/x86/mm/mem_encrypt_common.c
> @@ -15,9 +15,9 @@
>  bool force_dma_unencrypted(struct device *dev)
>  {
>  	/*
> -	 * For SEV, all DMA must be to unencrypted/shared addresses.
> +	 * For SEV and TDX, all DMA must be to unencrypted/shared addresses.
>  	 */
> -	if (sev_active())
> +	if (sev_active() || is_tdx_guest())
>  		return true;
>  
>  	/*
> diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
> index 16f878c26667..6f23a9816ef0 100644
> --- a/arch/x86/mm/pat/set_memory.c
> +++ b/arch/x86/mm/pat/set_memory.c
> @@ -27,6 +27,7 @@
>  #include <asm/proto.h>
>  #include <asm/memtype.h>
>  #include <asm/set_memory.h>
> +#include <asm/tdx.h>
>  
>  #include "../mm_internal.h"
>  
> @@ -1977,8 +1978,8 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
>  	struct cpa_data cpa;
>  	int ret;
>  
> -	/* Nothing to do if memory encryption is not active */
> -	if (!mem_encrypt_active())
> +	/* Nothing to do if memory encryption and TDX are not active */
> +	if (!mem_encrypt_active() && !is_tdx_guest())
>  		return 0;

So, this is starting to look like the "enc" naming is wrong, or at least
a little misleading.   Should we be talking about "protection" or
"guards" or something?

>  	/* Should not be working on unaligned addresses */
> @@ -1988,8 +1989,14 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
>  	memset(&cpa, 0, sizeof(cpa));
>  	cpa.vaddr = &addr;
>  	cpa.numpages = numpages;
> -	cpa.mask_set = enc ? __pgprot(_PAGE_ENC) : __pgprot(0);
> -	cpa.mask_clr = enc ? __pgprot(0) : __pgprot(_PAGE_ENC);
> +	if (is_tdx_guest()) {
> +		cpa.mask_set = __pgprot(enc ? 0 : tdx_shared_mask());
> +		cpa.mask_clr = __pgprot(enc ? tdx_shared_mask() : 0);
> +	} else {
> +		cpa.mask_set = __pgprot(enc ? _PAGE_ENC : 0);
> +		cpa.mask_clr = __pgprot(enc ? 0 : _PAGE_ENC);
> +	}

OK, this is too hideous to live.  It sucks that the TDX and SEV/SME bits
are opposite polarity, but oh well.

To me, this gets a lot clearer, and opens up room for commenting if you
do something like:

	if (is_tdx_guest()) {
		mem_enc_bits   = 0;
		mem_plain_bits = tdx_shared_mask();
	} else {
		mem_enc_bits   = _PAGE_ENC;
		mem_plain_bits = 0
	}

	if (enc) {
		cpa.mask_set = mem_enc_bits;
		cpa.mask_clr = mem_plain_bits;  // clear "plain" bits
	} else {
		
		cpa.mask_set = mem_plain_bits;
		cpa.mask_clr = mem_enc_bits;	// clear encryption bits
	}

>  	cpa.pgd = init_mm.pgd;
>  
>  	/* Must avoid aliasing mappings in the highmem code */
> @@ -1999,7 +2006,8 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
>  	/*
>  	 * Before changing the encryption attribute, we need to flush caches.
>  	 */
> -	cpa_flush(&cpa, !this_cpu_has(X86_FEATURE_SME_COHERENT));
> +	if (!enc || !is_tdx_guest())
> +		cpa_flush(&cpa, !this_cpu_has(X86_FEATURE_SME_COHERENT));

That "!enc" looks wrong to me.  Caches would need to be flushed whenever
encryption attributes *change*, not just when they are set.

Also, cpa_flush() flushes caches *AND* the TLB.  How does TDX manage to
not need TLB flushes?

>  	ret = __change_page_attr_set_clr(&cpa, 1);
>  
> @@ -2012,6 +2020,11 @@ static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
>  	 */
>  	cpa_flush(&cpa, 0);
>  
> +	if (!ret && is_tdx_guest()) {
> +		ret = tdx_map_gpa(__pa(addr), numpages, enc);
> +		// XXX: need to undo on error?
> +	}

Time to fix this stuff up if you want folks to take this series more
seriously.