Re: [PATCH v7 19/36] x86/mm: Add support to access boot related data in the clear

[Matt Fleming]

On Fri, 16 Jun, at 01:53:26PM, Tom Lendacky wrote:
> Boot data (such as EFI related data) is not encrypted when the system is
> booted because UEFI/BIOS does not run with SME active. In order to access
> this data properly it needs to be mapped decrypted.
> 
> Update early_memremap() to provide an arch specific routine to modify the
> pagetable protection attributes before they are applied to the new
> mapping. This is used to remove the encryption mask for boot related data.
> 
> Update memremap() to provide an arch specific routine to determine if RAM
> remapping is allowed.  RAM remapping will cause an encrypted mapping to be
> generated. By preventing RAM remapping, ioremap_cache() will be used
> instead, which will provide a decrypted mapping of the boot related data.
> 
> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
> ---
>  arch/x86/include/asm/io.h |5 +
>  arch/x86/mm/ioremap.c |  179 
> +
>  include/linux/io.h|2 +
>  kernel/memremap.c |   20 -
>  mm/early_ioremap.c|   18 -
>  5 files changed, 217 insertions(+), 7 deletions(-)

Reviewed-by: Matt Fleming <m...@codeblueprint.co.uk>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 18/36] x86/efi: Update EFI pagetable creation to work with SME

[Matt Fleming]

On Fri, 16 Jun, at 01:53:17PM, Tom Lendacky wrote:
> When SME is active, pagetable entries created for EFI need to have the
> encryption mask set as necessary.
> 
> When the new pagetable pages are allocated they are mapped encrypted. So,
> update the efi_pgt value that will be used in cr3 to include the encryption
> mask so that the PGD table can be read successfully. The pagetable mapping
> as well as the kernel are also added to the pagetable mapping as encrypted.
> All other EFI mappings are mapped decrypted (tables, etc.).
> 
> Reviewed-by: Borislav Petkov <b...@suse.de>
> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
> ---
>  arch/x86/platform/efi/efi_64.c |   15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
 
Reviewed-by: Matt Fleming <m...@codeblueprint.co.uk>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 16/36] efi: Add an EFI table address match function

[Matt Fleming]

On Fri, 16 Jun, at 01:52:53PM, Tom Lendacky wrote:
> Add a function that will determine if a supplied physical address matches
> the address of an EFI table.
> 
> Reviewed-by: Borislav Petkov <b...@suse.de>
> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
> ---
>  drivers/firmware/efi/efi.c |   33 +
>  include/linux/efi.h|7 +++
>  2 files changed, 40 insertions(+)

Reviewed-by: Matt Fleming <m...@codeblueprint.co.uk>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 17/36] efi: Update efi_mem_type() to return an error rather than 0

[Matt Fleming]

On Fri, 16 Jun, at 01:53:06PM, Tom Lendacky wrote:
> The efi_mem_type() function currently returns a 0, which maps to
> EFI_RESERVED_TYPE, if the function is unable to find a memmap entry for
> the supplied physical address. Returning EFI_RESERVED_TYPE implies that
> a memmap entry exists, when it doesn't.  Instead of returning 0, change
> the function to return a negative error value when no memmap entry is
> found.
> 
> Reviewed-by: Borislav Petkov <b...@suse.de>
> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
> ---
>  arch/ia64/kernel/efi.c  |4 ++--
>  arch/x86/platform/efi/efi.c |6 +++---
>  include/linux/efi.h |2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Matt Fleming <m...@codeblueprint.co.uk>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 17/32] x86/mm: Add support to access boot related data in the clear

[Matt Fleming]

On Mon, 15 May, at 08:35:17PM, Borislav Petkov wrote:
> On Tue, Apr 18, 2017 at 04:19:21PM -0500, Tom Lendacky wrote:
>
> > +   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;
> 
> So those two above look like could be two global vars which are
> initialized somewhere in the EFI init path:
> 
> efi_memmap_phys and efi_systab_phys or so.
> 
> Matt ?
> 
> And then you won't need to create that paddr each time on the fly. I
> mean, it's not a lot of instructions but still...
 
We should already have the physical memmap address available in
'efi.memmap.phys_map'.

And the physical address of the system table should be in
'efi_phys.systab'. See efi_init().
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v4 13/28] efi: Update efi_mem_type() to return defined EFI mem types

[Matt Fleming]

On Thu, 23 Feb, at 11:27:55AM, Tom Lendacky wrote:
> 
> I can do that, I'll change the return type to an int. For the
> !efi_enabled I can return -ENOTSUPP and for when an entry isn't
> found I can return -EINVAL.  Sound good?
 
Sounds good to me!
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v4 13/28] efi: Update efi_mem_type() to return defined EFI mem types

[Matt Fleming]

On Thu, 16 Feb, at 09:44:57AM, Tom Lendacky wrote:
> Update the efi_mem_type() to return EFI_RESERVED_TYPE instead of a
> hardcoded 0.
> 
> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/platform/efi/efi.c |4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index a15cf81..6407103 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -1037,7 +1037,7 @@ u32 efi_mem_type(unsigned long phys_addr)
>   efi_memory_desc_t *md;
>  
>   if (!efi_enabled(EFI_MEMMAP))
> - return 0;
> + return EFI_RESERVED_TYPE;
>  
>   for_each_efi_memory_desc(md) {
>   if ((md->phys_addr <= phys_addr) &&
> @@ -1045,7 +1045,7 @@ u32 efi_mem_type(unsigned long phys_addr)
> (md->num_pages << EFI_PAGE_SHIFT
>   return md->type;
>   }
> - return 0;
> + return EFI_RESERVED_TYPE;
>  }

I see what you're getting at here, but arguably the return value in
these cases never should have been zero to begin with (your change
just makes that more obvious).

Returning EFI_RESERVED_TYPE implies an EFI memmap entry exists for
this address, which is misleading because it doesn't in the hunks
you've modified above.

Instead, could you look at returning a negative error value in the
usual way we do in the Linux kernel, and update the function prototype
to match? I don't think any callers actually require the return type
to be u32.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Matt Fleming]

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.

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

Re: [RFC PATCH v2 11/20] mm: Access BOOT related data in the clear

[Matt Fleming]

On Wed, 14 Sep, at 09:20:44AM, Tom Lendacky wrote:
> On 09/12/2016 11:55 AM, Andy Lutomirski wrote:
> > On Aug 22, 2016 6:53 PM, "Tom Lendacky"  wrote:
> >>
> >> BOOT data (such as EFI related data) is not encyrpted when the system is
> >> booted and needs to be accessed as non-encrypted.  Add support to the
> >> early_memremap API to identify the type of data being accessed so that
> >> the proper encryption attribute can be applied.  Currently, two types
> >> of data are defined, KERNEL_DATA and BOOT_DATA.
> > 
> > What happens when you memremap boot services data outside of early
> > boot?  Matt just added code that does this.
> > 
> > IMO this API is not so great.  It scatters a specialized consideration
> > all over the place.  Could early_memremap not look up the PA to figure
> > out what to do?
> 
> Yes, I could see if the PA falls outside of the kernel usable area and,
> if so, remove the memory encryption attribute from the mapping (for both
> early_memremap and memremap).
> 
> Let me look into that, I would prefer something along that line over
> this change.

So, the last time we talked about using the address to figure out
whether to encrypt/decrypt you said,

 "I looked into this and this would be a large change also to parse
  tables and build lists."

Has something changed that makes this approach easier?

And again, you need to be careful with the EFI kexec code paths, since
you've got a mixture of boot and kernel data being passed. In
particular the EFI memory map is allocated by the firmware on first
boot (BOOT_DATA) but by the kernel on kexec (KERNEL_DATA).

That's one of the reasons I suggested requiring the caller to decide
on BOOT_DATA vs KERNEL_DATA - when you start looking at kexec the
distinction isn't easily made.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 10/18] x86/efi: Access EFI related tables in the clear

[Matt Fleming]

On Thu, 16 Jun, at 09:38:31AM, Tom Lendacky wrote:
> 
> Ok, I think this was happening before the commit to build our own
> EFI page table structures:
> 
> commit 67a9108ed ("x86/efi: Build our own page table structures")
> 
> Before this commit the boot services ended up mapped into the kernel
> page table entries as un-encrypted during efi_map_regions() and I needed
> to change those entries back to encrypted. With your change above,
> this appears to no longer be needed.

Great news! Things are as they should be ;)
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 10/18] x86/efi: Access EFI related tables in the clear

[Matt Fleming]

On Thu, 09 Jun, at 01:33:30PM, Tom Lendacky wrote:
> 
> I was trying to play it safe here, but as you say, the firmware should
> be using our page tables so we can get rid of this call. The problem
> will actually be if we transition to a 32-bit efi. The encryption bit
> will be lost in cr3 and so the pgd table will have to be un-encrypted.
> The entries in the pgd can have the encryption bit set so I would only
> need to worry about the pgd itself. I'll have to update the
> efi_alloc_page_tables routine.
 
Interesting, I hadn't expected 32-bit EFI to be an option for
platforms with the SME technology. I'd assumed we could just ignore
that.

Are you saying that the encryption bit isn't supported in 32-bit
compatibility mode? We don't do a "full" switch to 32-bit protected
mode when in mixed mode, just load a 32-bit code segment descriptor.
The page tables are not modified at all.

> The encryption bit in the cr3 register will indicate if the pgd table
> is encrypted or not. Based on my comment above about the pgd having
> to be un-encrypted in case we have to transition to 32-bit efi, this
> can be removed.
 
I'm not (yet) sure that the pgd needs to be unencrypted for 32-bit EFI
when running a 64-bit kernel. In the AMD Programmer's Manual, Section
7.10.3 Operating Modes seems to indicate that running encrypted should
work fine.

> I'll look into this a bit more. From looking at it I don't want the
> _PAGE_ENC bit set for the memmap unless it gets re-allocated (which
> I missed in these patches). Let me see what I can do with this.
 
I don't understand your comment about re-allocating the memmap.

The kernel builds its own EFI memory map at runtime, initially based
on the memory map provided by the firmware. We always allocate a new
memory map.

In efi_setup_page_tables() we're building our own page tables, which
should be encrypted, and mapping EFI regions described by the memmap
into those page tables.

So unless we're mapping an MMIO region (in which case _PAGE_PCD is set
in @flags for kernel_map_pages_in_pgd()) I would expect _PAGE_ENC to
be set.

> I'll look further into this, but I saw that this area of virtual memory
> was mapped un-encrypted and after freeing the boot services the
> mappings were somehow reused as un-encrypted for DMA which assumes
> (unless using swiotlb) encrypted. This resulted in DMA data being
> transferred in as encrypted and then accessed un-encrypted.

That the mappings were re-used isn't a surprise.

efi_free_boot_services() lifts the reservation that was put in place
during efi_reserve_boot_services() and releases the pages to the
kernel's memory allocators.

What is surprising is that they were marked unencrypted at all.
There's nothing special about these pages as far as the __va() region
is concerned.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 10/18] x86/efi: Access EFI related tables in the clear

[Matt Fleming]

On Mon, 13 Jun, at 01:03:22PM, Matt Fleming wrote:
> 
> Would we need a new function? Couldn't we just have a new
> FIXMAP_PAGE_* constant? e.g. would something like this work?
> 
> ---
> 
> enum memremap_owner {
>   KERNEL_DATA = 0,
>   BOOT_DATA,
> };
> 
> void __init *
> early_memremap(resource_size_t phys_addr, unsigned long size,
>  enum memremap_owner owner)
> {
>   pgprot_t prot;
> 
>   switch (owner) {
>   case BOOT_DATA:
>   prot = FIXMAP_PAGE_BOOT;
>   break;
>   case KERNEL_DATA:   /* FALLTHROUGH */
>   default:
>   prot = FIXMAP_PAGE_NORMAL;
>   
>   }
> 
>   return (__force void *)__early_ioremap(phys_addr, size, prot);
> }

Although it occurs to me that if there's a trivial 1:1 mapping between
memremap_owner and FIXMAP_PAGE_* we might as well just add a new
early_memremap_boot() that uses the correct FIXMAP_PAGE_* constant,
akin to early_memremap_ro().
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 10/18] x86/efi: Access EFI related tables in the clear

[Matt Fleming]

On Tue, 26 Apr, at 05:57:40PM, Tom Lendacky wrote:
> The EFI tables are not encrypted and need to be accessed as such. Be sure
> to memmap them without the encryption attribute set. For EFI support that
> lives outside of the arch/x86 tree, create a routine that uses the __weak
> attribute so that it can be overridden by an architecture specific routine.
> 
> When freeing boot services related memory, since it has been mapped as
> un-encrypted, be sure to change the mapping to encrypted for future use.
> 
> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/include/asm/cacheflush.h  |3 +
>  arch/x86/include/asm/mem_encrypt.h |   22 +++
>  arch/x86/kernel/setup.c|6 +--
>  arch/x86/mm/mem_encrypt.c  |   56 +++
>  arch/x86/mm/pageattr.c |   75 
> 
>  arch/x86/platform/efi/efi.c|   26 +++-
>  arch/x86/platform/efi/efi_64.c |9 +++-
>  arch/x86/platform/efi/quirks.c |   12 +-
>  drivers/firmware/efi/efi.c |   18 +++--
>  drivers/firmware/efi/esrt.c|   12 +++---
>  include/linux/efi.h|3 +
>  11 files changed, 212 insertions(+), 30 deletions(-)

[...]

> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 994a7df8..871b213 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -53,6 +53,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define EFI_DEBUG
>  
> @@ -261,12 +262,12 @@ static int __init efi_systab_init(void *phys)
>   u64 tmp = 0;
>  
>   if (efi_setup) {
> - data = early_memremap(efi_setup, sizeof(*data));
> + data = sme_early_memremap(efi_setup, sizeof(*data));
>   if (!data)
>   return -ENOMEM;
>   }

Beware, this data comes from a previous kernel that kexec'd this
kernel. Unless you've updated bzImage64_load() to allocate an
unencrypted region 'efi_setup' will in fact be encrypted.

> @@ -690,6 +691,7 @@ static void *realloc_pages(void *old_memmap, int 
> old_shift)
>   ret = (void *)__get_free_pages(GFP_KERNEL, old_shift + 1);
>   if (!ret)
>   goto out;
> + sme_set_mem_dec(ret, PAGE_SIZE << (old_shift + 1));
>  
>   /*
>* A first-time allocation doesn't have anything to copy.

I'm not sure why it's necessary to mark this region as unencrypted,
because at this point the kernel controls the platform and when we
call into the firmware it should be using our page tables. I wouldn't
expect the firmware to mess with the SYSCFG MSR either.

Have you come across a situation where the above was required?

> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index 49e4dd4..834a992 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -223,7 +223,7 @@ 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);
> + efi_scratch.efi_pgt = (pgd_t *)__sme_pa(efi_pgd);
>   pgd = efi_pgd;
>  
>   /*

Huh? Why does __pa() now OR in sme_mas_mask? I thought SME only
required the page table structures to be modified, not the end
address?

> @@ -262,7 +262,8 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, 
> unsigned num_pages)
>   pfn = md->phys_addr >> PAGE_SHIFT;
>   npages = md->num_pages;
>  
> - if (kernel_map_pages_in_pgd(pgd, pfn, md->phys_addr, npages, 
> _PAGE_RW)) {
> + if (kernel_map_pages_in_pgd(pgd, pfn, md->phys_addr, npages,
> + _PAGE_RW | _PAGE_ENC)) {
>   pr_err("Failed to map 1:1 memory\n");
>   return 1;
>   }

Could you push the _PAGE_ENC addition down into
kernel_map_pages_in_pgd()? Other flags are also handled that way, see
_PAGE_PRESENT.

> @@ -272,6 +273,7 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, 
> unsigned num_pages)
>   if (!page)
>   panic("Unable to allocate EFI runtime stack < 4GB\n");
>  
> + sme_set_mem_dec(page_address(page), PAGE_SIZE);
>   efi_scratch.phys_stack = virt_to_phys(page_address(page));
>   efi_scratch.phys_stack += PAGE_SIZE; /* stack grows down */
>  

We should not need to mark the stack as unencrypted, the firmware
should respect our SME settings, right?

> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index ab50ada..dde4fb6b 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -13,6 +13,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define EFI_MIN_RESERVE 5120
>  
> @@ -265,6 +266,13 @@ void __init efi_free_boot_services(void)
>   

Re: [RFC PATCH v1 10/18] x86/efi: Access EFI related tables in the clear

[Matt Fleming]

(Sorry for the delay)

On Thu, 26 May, at 08:45:58AM, Tom Lendacky wrote:
> 
> The patch in question is patch 6/18 where PAGE_KERNEL is changed to
> include the _PAGE_ENC attribute (the encryption mask). This now
> makes FIXMAP_PAGE_NORMAL contain the encryption mask while
> FIXMAP_PAGE_IO does not. In this way, anything mapped using the
> early_ioremap call won't be mapped encrypted.

There are semantics attached to early_ioremap() that do not apply in
this case; that you're mapping an MMIO region but for EFI we just care
about noting where the firmware (not the kernel) populated the region
with data. Similar problems exist for other early boot code such as
the devicetree stuff.

early_ioremap() is not the answer.

What you really want is just some way to distinguish kernel-owned
regions from those owned by "somebody else".

I have no problem updating early_memremap() to take a @flags argument
to make that distinction, provided that the naming is generic and not
tied to AMD's SME technology via an "sme" prefix/suffix.

And making it generic should allow it to be easily sprinkled into the
shared architecture code in drivers/firmware/efi/ without issue.

I'm going to follow up with some additional comments/questions on
PATCH 10.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 10/18] x86/efi: Access EFI related tables in the clear

[Matt Fleming]

On Tue, 24 May, at 09:54:31AM, Tom Lendacky wrote:
> 
> I looked into this and this would be a large change also to parse tables
> and build lists.  It occurred to me that this could all be taken care of
> if the early_memremap calls were changed to early_ioremap calls. Looking
> in the git log I see that they were originally early_ioremap calls but
> were changed to early_memremap calls with this commit:
> 
> commit abc93f8eb6e4 ("efi: Use early_mem*() instead of early_io*()")
> 
> Looking at the early_memremap code and the early_ioremap code they both
> call __early_ioremap so I don't see how this change makes any
> difference (especially since FIXMAP_PAGE_NORMAL and FIXMAP_PAGE_IO are
> identical in this case).
> 
> Is it safe to change these back to early_ioremap calls (at least on
> x86)?

I really don't want to begin mixing early_ioremap() calls and
early_memremap() calls for any of the EFI code if it can be avoided.

There is slow but steady progress to move more and more of the
architecture specific EFI code out into generic code. Swapping
early_memremap() for early_ioremap() would be a step backwards,
because FIXMAP_PAGE_NORMAL and FIXMAP_PAGE_IO are not identical on
ARM/arm64.

Could you point me at the patch that in this series that fixes up
early_ioremap() to work with mem encrypt/decrypt? I took another
(quick) look through but couldn't find it.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 10/18] x86/efi: Access EFI related tables in the clear

[Matt Fleming]

On Tue, 26 Apr, at 05:57:40PM, Tom Lendacky wrote:
> The EFI tables are not encrypted and need to be accessed as such. Be sure
> to memmap them without the encryption attribute set. For EFI support that
> lives outside of the arch/x86 tree, create a routine that uses the __weak
> attribute so that it can be overridden by an architecture specific routine.
> 
> When freeing boot services related memory, since it has been mapped as
> un-encrypted, be sure to change the mapping to encrypted for future use.
> 
> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/include/asm/cacheflush.h  |3 +
>  arch/x86/include/asm/mem_encrypt.h |   22 +++
>  arch/x86/kernel/setup.c|6 +--
>  arch/x86/mm/mem_encrypt.c  |   56 +++
>  arch/x86/mm/pageattr.c |   75 
> 
>  arch/x86/platform/efi/efi.c|   26 +++-
>  arch/x86/platform/efi/efi_64.c |9 +++-
>  arch/x86/platform/efi/quirks.c |   12 +-
>  drivers/firmware/efi/efi.c |   18 +++--
>  drivers/firmware/efi/esrt.c|   12 +++---
>  include/linux/efi.h|3 +
>  11 files changed, 212 insertions(+), 30 deletions(-)

The size of this change is completely unexpected. Why is there so much
churn to workaround this new feature?

Is it not possible to maintain some kind of kernel virtual address
mapping so memremap*() and friends can figure out when to twiddle the
mapping attributes and map with/without encryption?

These API changes place an undue burden on developers that don't even
care about SME.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html