Re: [RFC PATCH v3 10/20] Add support to access boot related data in the clear

[Tom Lendacky]

On 12/7/2016 7:19 AM, Matt Fleming wrote:
> On Wed, 09 Nov, at 06:36:31PM, Tom Lendacky wrote:
>> Boot data (such as EFI related data) is not encrypted when the system is
>> booted and needs to be accessed unencrypted.  Add support to apply the
>> proper attributes to the EFI page tables and to the early_memremap and
>> memremap APIs to identify the type of data being accessed so that the
>> proper encryption attribute can be applied.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
>> ---
>>  arch/x86/include/asm/e820.h    |    1 
>>  arch/x86/kernel/e820.c         |   16 +++++++
>>  arch/x86/mm/ioremap.c          |   89 ++++++++++++++++++++++++++++++++++++++++
>>  arch/x86/platform/efi/efi_64.c |   12 ++++-
>>  drivers/firmware/efi/efi.c     |   33 +++++++++++++++
>>  include/linux/efi.h            |    2 +
>>  kernel/memremap.c              |    8 +++-
>>  mm/early_ioremap.c             |   18 +++++++-
>>  8 files changed, 172 insertions(+), 7 deletions(-)
>  
> FWIW, I think this version is an improvement over all the previous
> ones.
> 
> [...]
> 
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index ff542cd..ee347c2 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -20,6 +20,9 @@
>>  #include <asm/tlbflush.h>
>>  #include <asm/pgalloc.h>
>>  #include <asm/pat.h>
>> +#include <asm/e820.h>
>> +#include <asm/setup.h>
>> +#include <linux/efi.h>
>>  
>>  #include "physaddr.h"
>>  
>> @@ -418,6 +421,92 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr)
>>  	iounmap((void __iomem *)((unsigned long)addr & PAGE_MASK));
>>  }
>>  
>> +static bool memremap_setup_data(resource_size_t phys_addr,
>> +				unsigned long size)
>> +{
>> +	u64 paddr;
>> +
>> +	if (phys_addr == boot_params.hdr.setup_data)
>> +		return true;
>> +
> 
> Why is the setup_data linked list not traversed when checking for
> matching addresses? Am I reading this incorrectly? I don't see how
> this can work.

Yeah, I caught that too after I sent this out. I think the best way to
handle this would be to create a list/array of setup data addresses in
the parse_setup_data() routine and then check the address against that
list in this routine.

> 
>> +	paddr = boot_params.efi_info.efi_memmap_hi;
>> +	paddr <<= 32;
>> +	paddr |= boot_params.efi_info.efi_memmap;
>> +	if (phys_addr == paddr)
>> +		return true;
>> +
>> +	paddr = boot_params.efi_info.efi_systab_hi;
>> +	paddr <<= 32;
>> +	paddr |= boot_params.efi_info.efi_systab;
>> +	if (phys_addr == paddr)
>> +		return true;
>> +
>> +	if (efi_table_address_match(phys_addr))
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +static bool memremap_apply_encryption(resource_size_t phys_addr,
>> +				      unsigned long size)
>> +{
>> +	/* SME is not active, just return true */
>> +	if (!sme_me_mask)
>> +		return true;
>> +
>> +	/* Check if the address is part of the setup data */
>> +	if (memremap_setup_data(phys_addr, size))
>> +		return false;
>> +
>> +	/* Check if the address is part of EFI boot/runtime data */
>> +	switch (efi_mem_type(phys_addr)) {
>> +	case EFI_BOOT_SERVICES_DATA:
>> +	case EFI_RUNTIME_SERVICES_DATA:
>> +		return false;
>> +	}
> 
> EFI_LOADER_DATA is notable by its absence.
> 
> We use that memory type for allocations inside of the EFI boot stub
> that are than used while the kernel is running. One use that comes to
> mind is for initrd files, see handle_cmdline_files().
> 
> Oh I see you handle that in PATCH 9, never mind.
> 
>> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
>> index 58b0f80..3f89179 100644
>> --- a/arch/x86/platform/efi/efi_64.c
>> +++ b/arch/x86/platform/efi/efi_64.c
>> @@ -221,7 +221,13 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
>>  	if (efi_enabled(EFI_OLD_MEMMAP))
>>  		return 0;
>>  
>> -	efi_scratch.efi_pgt = (pgd_t *)__pa(efi_pgd);
>> +	/*
>> +	 * Since the PGD is encrypted, set the encryption mask so that when
>> +	 * this value is loaded into cr3 the PGD will be decrypted during
>> +	 * the pagetable walk.
>> +	 */
>> +	efi_scratch.efi_pgt = (pgd_t *)__sme_pa(efi_pgd);
>> +
>>  	pgd = efi_pgd;
>>  
>>  	/*
> 
> Do all callers of __pa() in arch/x86 need fixing up like this?

No, currently this is only be needed when we're dealing with values that
will be used in the cr3 register.

Thanks,
Tom

>