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
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
