Subject: RE: [RFC PATCH v4 15/28] Add support to access persistent memory in the clear



> -----Original Message-----
> From: [email protected] [mailto:linux-kernel-
> [email protected]] On Behalf Of Tom Lendacky
> Sent: Thursday, February 16, 2017 9:45 AM
> Subject: [RFC PATCH v4 15/28] Add support to access persistent memory in
> the clear
>
> Persistent memory is expected to persist across reboots. The encryption
> key used by SME will change across reboots which will result in corrupted
> persistent memory. Persistent memory is handed out by block devices
> through memory remapping functions, so be sure not to map this memory as
> encrypted.

The system might be able to save and restore the correct encryption key for a
region of persistent memory, in which case it does need to be mapped as
encrypted.

This might deserve a new EFI_MEMORY_ENCRYPTED attribute bit so the
system firmware can communicate that information to the OS (in the
UEFI memory map and the ACPI NFIT SPA Range structures). It wouldn't
likely ever be added to the E820h table - ACPI 6.1 already obsoleted the
Extended Attribute for AddressRangeNonVolatile.

>
> Signed-off-by: Tom Lendacky <[email protected]>
> ---
> arch/x86/mm/ioremap.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index b0ff6bc..c6cb921 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -498,6 +498,8 @@ static bool
> memremap_should_map_encrypted(resource_size_t phys_addr,
> case E820_TYPE_ACPI:
> case E820_TYPE_NVS:
> case E820_TYPE_UNUSABLE:
> + case E820_TYPE_PMEM:
> + case E820_TYPE_PRAM:
> return false;
> default:
> break;

E820_TYPE_RESERVED is also used to report persistent memory in
some systems (patch 16 adds that for other reasons).

You might want to intercept the persistent memory types in the
efi_mem_type(phys_addr) switch statement earlier in the function
as well. https://lkml.org/lkml/2017/3/13/357 recently mentioned that
"in qemu hotpluggable memory isn't put into E820," with the latest
information only in the UEFI memory map.

Persistent memory can be reported there as:
* EfiReservedMemoryType type with the EFI_MEMORY_NV attribute
* EfiPersistentMemory type with the EFI_MEMORY_NV attribute

Even the UEFI memory map is not authoritative, though. To really
determine what is in these regions requires parsing the ACPI NFIT
SPA Ranges structures. Parts of the E820 or UEFI regions could be
reported as volatile there and should thus be encrypted.

---
Robert Elliott, HPE Persistent Memory





2017-03-23 21:03:15

by Tom Lendacky

[permalink] [raw]
Subject: Re: [RFC PATCH v4 15/28] Add support to access persistent memory in the clear

On 3/17/2017 5:58 PM, Elliott, Robert (Persistent Memory) wrote:
>
>
>> -----Original Message-----
>> From: [email protected] [mailto:linux-kernel-
>> [email protected]] On Behalf Of Tom Lendacky
>> Sent: Thursday, February 16, 2017 9:45 AM
>> Subject: [RFC PATCH v4 15/28] Add support to access persistent memory in
>> the clear
>>
>> Persistent memory is expected to persist across reboots. The encryption
>> key used by SME will change across reboots which will result in corrupted
>> persistent memory. Persistent memory is handed out by block devices
>> through memory remapping functions, so be sure not to map this memory as
>> encrypted.
>
> The system might be able to save and restore the correct encryption key for a
> region of persistent memory, in which case it does need to be mapped as
> encrypted.

If the OS could get some indication that BIOS/UEFI has saved and
restored the encryption key, then it could be mapped encrypted.

>
> This might deserve a new EFI_MEMORY_ENCRYPTED attribute bit so the
> system firmware can communicate that information to the OS (in the
> UEFI memory map and the ACPI NFIT SPA Range structures). It wouldn't
> likely ever be added to the E820h table - ACPI 6.1 already obsoleted the
> Extended Attribute for AddressRangeNonVolatile.

An attribute bit in some form would be a nice way to inform the OS that
the persistent memory can be mapped encrypted.

>
>>
>> Signed-off-by: Tom Lendacky <[email protected]>
>> ---
>> arch/x86/mm/ioremap.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index b0ff6bc..c6cb921 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -498,6 +498,8 @@ static bool
>> memremap_should_map_encrypted(resource_size_t phys_addr,
>> case E820_TYPE_ACPI:
>> case E820_TYPE_NVS:
>> case E820_TYPE_UNUSABLE:
>> + case E820_TYPE_PMEM:
>> + case E820_TYPE_PRAM:
>> return false;
>> default:
>> break;
>
> E820_TYPE_RESERVED is also used to report persistent memory in
> some systems (patch 16 adds that for other reasons).
>
> You might want to intercept the persistent memory types in the
> efi_mem_type(phys_addr) switch statement earlier in the function
> as well. https://lkml.org/lkml/2017/3/13/357 recently mentioned that
> "in qemu hotpluggable memory isn't put into E820," with the latest
> information only in the UEFI memory map.
>
> Persistent memory can be reported there as:
> * EfiReservedMemoryType type with the EFI_MEMORY_NV attribute
> * EfiPersistentMemory type with the EFI_MEMORY_NV attribute
>
> Even the UEFI memory map is not authoritative, though. To really
> determine what is in these regions requires parsing the ACPI NFIT
> SPA Ranges structures. Parts of the E820 or UEFI regions could be
> reported as volatile there and should thus be encrypted.

Thanks for the details on this. I'll take a closer look at this and
update the checks appropriately.

Thanks,
Tom

>
> ---
> Robert Elliott, HPE Persistent Memory
>
>
>