From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Lendacky Subject: Re: [RFC PATCH v3 10/20] Add support to access boot related data in the clear Date: Fri, 9 Dec 2016 08:26:40 -0600 Message-ID: <8aebb166-12ae-64aa-bf1a-3f46fe8b52dd@amd.com> References: <20161110003426.3280.2999.stgit@tlendack-t1.amdoffice.net> <20161110003631.3280.73292.stgit@tlendack-t1.amdoffice.net> <20161207131903.GU20785@codeblueprint.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20161207131903.GU20785@codeblueprint.co.uk> Sender: owner-linux-mm@kvack.org To: Matt Fleming Cc: linux-arch@vger.kernel.org, linux-efi@vger.kernel.org, kvm@vger.kernel.org, linux-doc@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org, kasan-dev@googlegroups.com, linux-mm@kvack.org, iommu@lists.linux-foundation.org, Rik van Riel , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Arnd Bergmann , Jonathan Corbet , Joerg Roedel , Konrad Rzeszutek Wilk , Paolo Bonzini , Larry Woodman , Ingo Molnar , Borislav Petkov , Andy Lutomirski , "H. Peter Anvin" , Andrey Ryabinin , Alexander Potapenko , Thomas Gleixner , Dmitry Vyukov , Ard Biesheuvel List-Id: linux-efi@vger.kernel.org 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 >> --- >> 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 >> #include >> #include >> +#include >> +#include >> +#include >> >> #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, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org