[PATCH v1 0/2] x86: Secure Memory Encryption (SME) fixes 2017-07-26

[PATCH v1 0/2] x86: Secure Memory Encryption (SME) fixes 2017-07-26

[Tom Lendacky]

This patch series addresses some issues found during further testing of
Secure Memory Encryption (SME).

The following fixes are included in this update series:

- Fix a cache-related memory corruption when kexec is invoked in
  successive instances
- Remove the encryption mask from the protection properties returned
  by arch_apei_get_mem_attribute() when SME is active

---

This patch series is based off of the master branch of tip:
  https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git master

  Commit 8333bcad393c ("Merge branch 'x86/asm'")

Cc: <kexec@lists.infradead.org>

Tom Lendacky (2):
  x86/mm, kexec: Fix memory corruption with SME on successive kexecs
  acpi, x86: Remove encryption mask from ACPI page protection type

 arch/x86/include/asm/acpi.h          | 7 ++++++-
 arch/x86/kernel/relocate_kernel_64.S | 7 +++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH v1 1/2] x86/mm, kexec: Fix memory corruption with SME on successive kexecs

[Tom Lendacky]

After issuing successive kexecs it was found that the SHA hash failed
verification when booting the kexec'd kernel.  When SME is enabled, the
change from using pages that were marked encrypted to now being marked as
not encrypted (through new identify mapped page tables) results in memory
corruption if there are any cache entries for the previously encrypted
pages. This is because separate cache entries can exist for the same
physical location but tagged both with and without the encryption bit.

To prevent this, issue a wbinvd before copying the pages from the source
location to the destination location to clear any possible cache entry
conflicts.

Cc: <kexec@lists.infradead.org>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 arch/x86/kernel/relocate_kernel_64.S | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
index 98111b3..c11d8bc 100644
--- a/arch/x86/kernel/relocate_kernel_64.S
+++ b/arch/x86/kernel/relocate_kernel_64.S
@@ -132,6 +132,13 @@ identity_mapped:
 	/* Flush the TLB (needed?) */
 	movq	%r9, %cr3
 
+	/*
+	 * If SME is/was active, there could be old encrypted cache line
+	 * entries that will conflict with the now unencrypted memory
+	 * used by kexec. Flush the caches before copying the kernel.
+	 */
+	wbinvd
+
 	movq	%rcx, %r11
 	call	swap_pages
 
-- 
1.9.1

Re: [PATCH v1 1/2] x86/mm, kexec: Fix memory corruption with SME on successive kexecs

[Ingo Molnar]

* Tom Lendacky <thomas.lendacky@amd.com> wrote:

> After issuing successive kexecs it was found that the SHA hash failed
> verification when booting the kexec'd kernel.  When SME is enabled, the
> change from using pages that were marked encrypted to now being marked as
> not encrypted (through new identify mapped page tables) results in memory
> corruption if there are any cache entries for the previously encrypted
> pages. This is because separate cache entries can exist for the same
> physical location but tagged both with and without the encryption bit.
> 
> To prevent this, issue a wbinvd before copying the pages from the source
> location to the destination location to clear any possible cache entry
> conflicts.
> 
> Cc: <kexec@lists.infradead.org>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  arch/x86/kernel/relocate_kernel_64.S | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
> index 98111b3..c11d8bc 100644
> --- a/arch/x86/kernel/relocate_kernel_64.S
> +++ b/arch/x86/kernel/relocate_kernel_64.S
> @@ -132,6 +132,13 @@ identity_mapped:
>  	/* Flush the TLB (needed?) */
>  	movq	%r9, %cr3
>  
> +	/*
> +	 * If SME is/was active, there could be old encrypted cache line
> +	 * entries that will conflict with the now unencrypted memory
> +	 * used by kexec. Flush the caches before copying the kernel.
> +	 */
> +	wbinvd

WBINVD is very expensive IIRC - several milliseconds.

So if we change the page table from encrypted to unencrypted we need to do a full 
cache flush sounds pretty broken to me - how can then this be done via an API such 
as mmap() without executing WBINVD?

Thanks,

	Ingo

Re: [PATCH v1 1/2] x86/mm, kexec: Fix memory corruption with SME on successive kexecs

[Tom Lendacky]

On 7/27/2017 2:17 AM, Ingo Molnar wrote:
> 
> * Tom Lendacky <thomas.lendacky@amd.com> wrote:
> 
>> After issuing successive kexecs it was found that the SHA hash failed
>> verification when booting the kexec'd kernel.  When SME is enabled, the
>> change from using pages that were marked encrypted to now being marked as
>> not encrypted (through new identify mapped page tables) results in memory
>> corruption if there are any cache entries for the previously encrypted
>> pages. This is because separate cache entries can exist for the same
>> physical location but tagged both with and without the encryption bit.
>>
>> To prevent this, issue a wbinvd before copying the pages from the source
>> location to the destination location to clear any possible cache entry
>> conflicts.
>>
>> Cc: <kexec@lists.infradead.org>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>   arch/x86/kernel/relocate_kernel_64.S | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/x86/kernel/relocate_kernel_64.S b/arch/x86/kernel/relocate_kernel_64.S
>> index 98111b3..c11d8bc 100644
>> --- a/arch/x86/kernel/relocate_kernel_64.S
>> +++ b/arch/x86/kernel/relocate_kernel_64.S
>> @@ -132,6 +132,13 @@ identity_mapped:
>>   	/* Flush the TLB (needed?) */
>>   	movq	%r9, %cr3
>>   
>> +	/*
>> +	 * If SME is/was active, there could be old encrypted cache line
>> +	 * entries that will conflict with the now unencrypted memory
>> +	 * used by kexec. Flush the caches before copying the kernel.
>> +	 */
>> +	wbinvd
> 
> WBINVD is very expensive IIRC - several milliseconds.
> 
> So if we change the page table from encrypted to unencrypted we need to do a full
> cache flush sounds pretty broken to me - how can then this be done via an API such
> as mmap() without executing WBINVD?

The hardware doesn't enforce coherency between encrypted and unencrypted
mappings of the same physical page[1].  There are APIs that will perform
a targeted cache flush when changing the encryption bit associated with
a page table entry (set_memory_encrypted()/set_memory_decrypted()) and
don't require a full cache flush. But in the case of kexec, there is a
wholesale change of the page tables from what was active to the new
identity mapped tables without any way to know what was previously
mapped and whether it was previously mapped as encrypted or unencrypted.
In this case I don't think an API such as mmap() will help.  For SME, we
will need to be sure the cache is flushed to avoid any coherency issues.

I can #ifdef the wbinvd based on whether AMD_MEM_ENCRYPT is configured
or not so that the wbinvd is avoided if not configured.

Thanks,
Tom

[1] http://support.amd.com/TechDocs/24593.pdf (Section 7.10.6)

> 
> Thanks,
> 
> 	Ingo
> 

Re: [PATCH v1 1/2] x86/mm, kexec: Fix memory corruption with SME on successive kexecs

[Linus Torvalds]

On Thu, Jul 27, 2017 at 7:15 AM, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> I can #ifdef the wbinvd based on whether AMD_MEM_ENCRYPT is configured
> or not so that the wbinvd is avoided if not configured.

I suspect an ifdef will be useless, since things like distro kernels
tend to enable everything.

So it should probably be disabled dynamically, and only done if the
AMD memory encryption thing has actually been active.

[ There have also been various actual errata with wbinvd, so there
tends to be a non-performance reason to try to avoid it unless
strictly required ]

               Linus

Re: [PATCH v1 1/2] x86/mm, kexec: Fix memory corruption with SME on successive kexecs

[Tom Lendacky]

On 7/27/2017 12:34 PM, Linus Torvalds wrote:
> On Thu, Jul 27, 2017 at 7:15 AM, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> I can #ifdef the wbinvd based on whether AMD_MEM_ENCRYPT is configured
>> or not so that the wbinvd is avoided if not configured.
> 
> I suspect an ifdef will be useless, since things like distro kernels
> tend to enable everything.
> 
> So it should probably be disabled dynamically, and only done if the
> AMD memory encryption thing has actually been active.
> 
> [ There have also been various actual errata with wbinvd, so there
> tends to be a non-performance reason to try to avoid it unless
> strictly required ]

Ok, I'll make the wbinvd a run time decision based on whether SME is
active at the time.

Thanks,
Tom

> 
>                 Linus
> 

[PATCH v1 2/2] acpi, x86: Remove encryption mask from ACPI page protection type

[Tom Lendacky]

The function arch_apei_get_mem_attributes() is used to set the page
protection type for ACPI physical addresses. When SME is active, the
associated protection type needs to not have the encryption mask set
since the ACPI tables live in un-encrypted memory. Modify the
arch_apei_get_mem_attributes() function to remove the encryption mask
when SME is active by returning the PAGE_KERNEL_IO protection type.

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 arch/x86/include/asm/acpi.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
index 562286f..89df39d 100644
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h
@@ -34,6 +34,7 @@
 
 #ifdef CONFIG_ACPI_APEI
 # include <asm/pgtable_types.h>
+# include <linux/mem_encrypt.h>
 #endif
 
 #ifdef CONFIG_ACPI
@@ -164,8 +165,12 @@ static inline pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
 	 * anything other than PAGE_KERNEL (some arm64 platforms
 	 * require the equivalent of PAGE_KERNEL_NOCACHE), return that
 	 * until we know differently.
+	 *
+	 * When SME is active, the ACPI information will not reside in
+	 * in memory in an encrypted state so return a protection attribute
+	 * that does not have the encryption bit set.
 	 */
-	 return PAGE_KERNEL;
+	 return sme_active() ? PAGE_KERNEL_IO : PAGE_KERNEL;
 }
 #endif
 
-- 
1.9.1

Re: [PATCH v1 2/2] acpi, x86: Remove encryption mask from ACPI page protection type

[Ingo Molnar]

* Tom Lendacky <thomas.lendacky@amd.com> wrote:

> The function arch_apei_get_mem_attributes() is used to set the page
> protection type for ACPI physical addresses. When SME is active, the
> associated protection type needs to not have the encryption mask set
> since the ACPI tables live in un-encrypted memory. Modify the
> arch_apei_get_mem_attributes() function to remove the encryption mask
> when SME is active by returning the PAGE_KERNEL_IO protection type.
> 
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  arch/x86/include/asm/acpi.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> index 562286f..89df39d 100644
> --- a/arch/x86/include/asm/acpi.h
> +++ b/arch/x86/include/asm/acpi.h
> @@ -34,6 +34,7 @@
>  
>  #ifdef CONFIG_ACPI_APEI
>  # include <asm/pgtable_types.h>
> +# include <linux/mem_encrypt.h>
>  #endif
>  
>  #ifdef CONFIG_ACPI
> @@ -164,8 +165,12 @@ static inline pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
>  	 * anything other than PAGE_KERNEL (some arm64 platforms
>  	 * require the equivalent of PAGE_KERNEL_NOCACHE), return that
>  	 * until we know differently.
> +	 *
> +	 * When SME is active, the ACPI information will not reside in
> +	 * in memory in an encrypted state so return a protection attribute
> +	 * that does not have the encryption bit set.
>  	 */
> -	 return PAGE_KERNEL;
> +	 return sme_active() ? PAGE_KERNEL_IO : PAGE_KERNEL;

'in in memory'?

Also, this seems a bit ad-hoc to me. What are the rules for what is encrypted and 
what is not?

I presume the main rule is that everything that was written before the kernel 
activates SME, and which the kernel will read later on, is unencrypted - 
everything else is encrypted. Is that correct?

How about things like kexec creating a separate mptable for the kexec kernel - 
would that code have to create unencrypted data? See 
e820__memblock_alloc_reserved() and related code.

Thanks,

	Ingo

Re: [PATCH v1 2/2] acpi, x86: Remove encryption mask from ACPI page protection type

[Tom Lendacky]

On 7/27/2017 2:39 AM, Ingo Molnar wrote:
> 
> * Tom Lendacky <thomas.lendacky@amd.com> wrote:
> 
>> The function arch_apei_get_mem_attributes() is used to set the page
>> protection type for ACPI physical addresses. When SME is active, the
>> associated protection type needs to not have the encryption mask set
>> since the ACPI tables live in un-encrypted memory. Modify the
>> arch_apei_get_mem_attributes() function to remove the encryption mask
>> when SME is active by returning the PAGE_KERNEL_IO protection type.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>   arch/x86/include/asm/acpi.h | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
>> index 562286f..89df39d 100644
>> --- a/arch/x86/include/asm/acpi.h
>> +++ b/arch/x86/include/asm/acpi.h
>> @@ -34,6 +34,7 @@
>>   
>>   #ifdef CONFIG_ACPI_APEI
>>   # include <asm/pgtable_types.h>
>> +# include <linux/mem_encrypt.h>
>>   #endif
>>   
>>   #ifdef CONFIG_ACPI
>> @@ -164,8 +165,12 @@ static inline pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
>>   	 * anything other than PAGE_KERNEL (some arm64 platforms
>>   	 * require the equivalent of PAGE_KERNEL_NOCACHE), return that
>>   	 * until we know differently.
>> +	 *
>> +	 * When SME is active, the ACPI information will not reside in
>> +	 * in memory in an encrypted state so return a protection attribute
>> +	 * that does not have the encryption bit set.
>>   	 */
>> -	 return PAGE_KERNEL;
>> +	 return sme_active() ? PAGE_KERNEL_IO : PAGE_KERNEL;
> 
> 'in in memory'?

Should just be one 'in', will fix.

> 
> Also, this seems a bit ad-hoc to me. What are the rules for what is encrypted and
> what is not?
> 
> I presume the main rule is that everything that was written before the kernel
> activates SME, and which the kernel will read later on, is unencrypted -
> everything else is encrypted. Is that correct?

Yes, that is the basic rule.  Anything from BIOS/UEFI is unencrypted
and much of everything else after the kernel encrypts itself will be
encrypted.  Kernel related hings that aren't be encrypted are, for
example, the SWIOTLB bounce buffers, the trampoline area for bringing
up APs, etc.

> 
> How about things like kexec creating a separate mptable for the kexec kernel -
> would that code have to create unencrypted data? See
> e820__memblock_alloc_reserved() and related code.

Yes, that would need to be unencrypted since it will be considered to
be something supplied by BIOS/UEFI (even though kexec builds it). I'm
not familiar with where kexec creates a new mptable. I see the refs to
it in e820.c, but I'm missing something. Can you point me to that?

Thanks,
Tom

> 
> Thanks,
> 
> 	Ingo
> 

Re: [PATCH v1 2/2] acpi, x86: Remove encryption mask from ACPI page protection type

[Borislav Petkov]

On Wed, Jul 26, 2017 at 01:04:34PM -0500, Tom Lendacky wrote:
> The function arch_apei_get_mem_attributes() is used to set the page
> protection type for ACPI physical addresses. When SME is active, the
> associated protection type needs to not have the encryption mask set
> since the ACPI tables live in un-encrypted memory. Modify the
> arch_apei_get_mem_attributes() function to remove the encryption mask
> when SME is active by returning the PAGE_KERNEL_IO protection type.
> 
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  arch/x86/include/asm/acpi.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> index 562286f..89df39d 100644
> --- a/arch/x86/include/asm/acpi.h
> +++ b/arch/x86/include/asm/acpi.h
> @@ -34,6 +34,7 @@
>  
>  #ifdef CONFIG_ACPI_APEI
>  # include <asm/pgtable_types.h>
> +# include <linux/mem_encrypt.h>
>  #endif
>  
>  #ifdef CONFIG_ACPI
> @@ -164,8 +165,12 @@ static inline pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
>  	 * anything other than PAGE_KERNEL (some arm64 platforms
>  	 * require the equivalent of PAGE_KERNEL_NOCACHE), return that
>  	 * until we know differently.
> +	 *
> +	 * When SME is active, the ACPI information will not reside in
> +	 * in memory in an encrypted state so return a protection attribute
> +	 * that does not have the encryption bit set.
>  	 */
> -	 return PAGE_KERNEL;
> +	 return sme_active() ? PAGE_KERNEL_IO : PAGE_KERNEL;

Why isn't there a PAGE_KERNEL_NOENC define which you can simply return
instead of testing?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v1 2/2] acpi, x86: Remove encryption mask from ACPI page protection type

[Tom Lendacky]

On 7/27/2017 9:41 AM, Borislav Petkov wrote:
> On Wed, Jul 26, 2017 at 01:04:34PM -0500, Tom Lendacky wrote:
>> The function arch_apei_get_mem_attributes() is used to set the page
>> protection type for ACPI physical addresses. When SME is active, the
>> associated protection type needs to not have the encryption mask set
>> since the ACPI tables live in un-encrypted memory. Modify the
>> arch_apei_get_mem_attributes() function to remove the encryption mask
>> when SME is active by returning the PAGE_KERNEL_IO protection type.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>   arch/x86/include/asm/acpi.h | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
>> index 562286f..89df39d 100644
>> --- a/arch/x86/include/asm/acpi.h
>> +++ b/arch/x86/include/asm/acpi.h
>> @@ -34,6 +34,7 @@
>>   
>>   #ifdef CONFIG_ACPI_APEI
>>   # include <asm/pgtable_types.h>
>> +# include <linux/mem_encrypt.h>
>>   #endif
>>   
>>   #ifdef CONFIG_ACPI
>> @@ -164,8 +165,12 @@ static inline pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
>>   	 * anything other than PAGE_KERNEL (some arm64 platforms
>>   	 * require the equivalent of PAGE_KERNEL_NOCACHE), return that
>>   	 * until we know differently.
>> +	 *
>> +	 * When SME is active, the ACPI information will not reside in
>> +	 * in memory in an encrypted state so return a protection attribute
>> +	 * that does not have the encryption bit set.
>>   	 */
>> -	 return PAGE_KERNEL;
>> +	 return sme_active() ? PAGE_KERNEL_IO : PAGE_KERNEL;
> 
> Why isn't there a PAGE_KERNEL_NOENC define which you can simply return
> instead of testing?

Sounds like something I should add to pgtable_types.h (which has a
#define for PAGE_KERNEL_EXEC_NOENC, but not PAGE_KERNEL_NOENC). I'll
create that #define.

As for the sme_active() check I was getting ahead of myself since
under SEV the encryption mask is needed.  I'll change it to just
return PAGE_KERNEL_NOENC and then worry about the SEV change in
the SEV patches.

Thanks,
Tom

> 

Re: [PATCH v1 2/2] acpi, x86: Remove encryption mask from ACPI page protection type

[Ingo Molnar]

* Tom Lendacky <thomas.lendacky@amd.com> wrote:

> > > +	 * in memory in an encrypted state so return a protection attribute
> > > +	 * that does not have the encryption bit set.
> > >   	 */
> > > -	 return PAGE_KERNEL;
> > > +	 return sme_active() ? PAGE_KERNEL_IO : PAGE_KERNEL;
> > 
> > Why isn't there a PAGE_KERNEL_NOENC define which you can simply return
> > instead of testing?
> 
> Sounds like something I should add to pgtable_types.h (which has a
> #define for PAGE_KERNEL_EXEC_NOENC, but not PAGE_KERNEL_NOENC). I'll
> create that #define.
> 
> As for the sme_active() check I was getting ahead of myself since
> under SEV the encryption mask is needed.  I'll change it to just
> return PAGE_KERNEL_NOENC and then worry about the SEV change in
> the SEV patches.

Ok, that works for me too, as we at least don't sprinke the code with repeated 
sme_active() toggles.

Thanks,

	Ingo