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

[Tom Lendacky]

On 09/15/2016 04:57 AM, Matt Fleming wrote:
> 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?

The original idea of parsing the tables and building a list was
a large change.  This approach would be simpler by just checking if
the PA is outside the kernel usable area, and if so, removing the
encryption bit.

> 
> 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.

Yeah, for kexec I think I'll need to make sure that everything looks
like it came from the BIOS/UEFI/bootloader.  If all of the kexec
pieces are allocated with un-encrypted memory, then the boot path
should remain the same.  That's the piece I need to investigate
further.

Thanks,
Tom

> 

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

[Tom Lendacky]

On 09/15/2016 04:57 AM, Matt Fleming wrote:
> 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?

The original idea of parsing the tables and building a list was
a large change.  This approach would be simpler by just checking if
the PA is outside the kernel usable area, and if so, removing the
encryption bit.

> 
> 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.

Yeah, for kexec I think I'll need to make sure that everything looks
like it came from the BIOS/UEFI/bootloader.  If all of the kexec
pieces are allocated with un-encrypted memory, then the boot path
should remain the same.  That's the piece I need to investigate
further.

Thanks,
Tom

> 

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.

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.

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

[Tom Lendacky]

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.

Thanks,
Tom

> 
> --Andy
> 

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

[Tom Lendacky]

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.

Thanks,
Tom

> 
> --Andy
> 

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

[Andy Lutomirski]

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?

--Andy

[leaving the rest here for Matt's benefit]

>  unsigned long size,
> +   enum memremap_owner owner,
> +   pgprot_t prot)
> +{
> +   return prot;
> +}
> +
>  void __init early_ioremap_reset(void)
>  {
> early_ioremap_shutdown();
> @@ -213,16 +221,23 @@ early_ioremap(resource_size_t phys_addr, unsigned long 
> size)
>
>  /* Remap memory */
>  void __init *
> -early_memremap(resource_size_t phys_addr, unsigned long size)
> +early_memremap(resource_size_t phys_addr, unsigned long size,
> +  enum memremap_owner owner)
>  {
> -   return (__force void *)__early_ioremap(phys_addr, size,
> -  FIXMAP_PAGE_NORMAL);
> +   pgprot_t prot = early_memremap_pgprot_adjust(phys_addr, size, owner,
> +FIXMAP_PAGE_NORMAL);
> +
> +   return (__force void *)__early_ioremap(phys_addr, size, prot);
>  }
>  #ifdef FIXMAP_PAGE_RO
>  void __init *
> -early_memremap_ro(resource_size_t phys_addr, unsigned long size)
> +early_memremap_ro(resource_size_t phys_addr, unsigned long size,
> + enum memremap_owner owner)
>  {
> -   return (__force void *)__early_ioremap(phys_addr, size, 
> FIXMAP_PAGE_RO);
> +   pgprot_t prot = early_memremap_pgprot_adjust(phys_addr, size, owner,
> +FIXMAP_PAGE_RO);
> +
> +   return (__force void *)__early_ioremap(phys_addr, size, prot);
>  }
>  #endif
>
> @@ -236,7 +251,8 @@ early_memremap_prot(resource_size_t phys_addr, unsigned 
> long size,
>
>  #define MAX_MAP_CHUNK  (NR_FIX_BTMAPS << PAGE_SHIFT)
>
> -void __init copy_from_early_mem(void *dest, phys_addr_t src, unsigned long 
> size)
> +void __init copy_from_early_mem(void *dest, phys_addr_t src, unsigned long 
> size,
> +   enum memremap_owner owner)
>  {
> unsigned long slop, clen;
> char *p;
> @@ -246,7 +262,7 @@ void __init copy_from_early_mem(void *dest, phys_addr_t 
> src, unsigned long size)
> clen = size;
> if (clen > MAX_MAP_CHUNK - slop)
> clen = MAX_MAP_CHUNK - slop;
> -   p = early_memremap(src & PAGE_MASK, clen + slop);
> +   p = early_memremap(src & PAGE_MASK, clen + slop, owner);
> memcpy(dest, p + slop, clen);
> early_memunmap(p, clen + slop);
> dest += clen;
> @@ -265,12 +281,14 @@ early_ioremap(resource_size_t phys_addr, unsigned long 
> size)
>
>  /* Remap memory */
>  void __init *
> -early_memremap(resource_size_t phys_addr, unsigned long size)
> +early_memremap(resource_size_t phys_addr, unsigned long size,
> +  enum memremap_owner owner)
>  {
> return (void *)phys_addr;
>  }
>  void __init *
> -early_memremap_ro(resource_size_t phys_addr, unsigned long size)
> +early_memremap_ro(resource_size_t phys_addr, unsigned long size,
> + enum memremap_owner owner)
>  {
> return (void *)phys_addr;
>  }
>

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

[Andy Lutomirski]

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?

--Andy

[leaving the rest here for Matt's benefit]

>  unsigned long size,
> +   enum memremap_owner owner,
> +   pgprot_t prot)
> +{
> +   return prot;
> +}
> +
>  void __init early_ioremap_reset(void)
>  {
> early_ioremap_shutdown();
> @@ -213,16 +221,23 @@ early_ioremap(resource_size_t phys_addr, unsigned long 
> size)
>
>  /* Remap memory */
>  void __init *
> -early_memremap(resource_size_t phys_addr, unsigned long size)
> +early_memremap(resource_size_t phys_addr, unsigned long size,
> +  enum memremap_owner owner)
>  {
> -   return (__force void *)__early_ioremap(phys_addr, size,
> -  FIXMAP_PAGE_NORMAL);
> +   pgprot_t prot = early_memremap_pgprot_adjust(phys_addr, size, owner,
> +FIXMAP_PAGE_NORMAL);
> +
> +   return (__force void *)__early_ioremap(phys_addr, size, prot);
>  }
>  #ifdef FIXMAP_PAGE_RO
>  void __init *
> -early_memremap_ro(resource_size_t phys_addr, unsigned long size)
> +early_memremap_ro(resource_size_t phys_addr, unsigned long size,
> + enum memremap_owner owner)
>  {
> -   return (__force void *)__early_ioremap(phys_addr, size, 
> FIXMAP_PAGE_RO);
> +   pgprot_t prot = early_memremap_pgprot_adjust(phys_addr, size, owner,
> +FIXMAP_PAGE_RO);
> +
> +   return (__force void *)__early_ioremap(phys_addr, size, prot);
>  }
>  #endif
>
> @@ -236,7 +251,8 @@ early_memremap_prot(resource_size_t phys_addr, unsigned 
> long size,
>
>  #define MAX_MAP_CHUNK  (NR_FIX_BTMAPS << PAGE_SHIFT)
>
> -void __init copy_from_early_mem(void *dest, phys_addr_t src, unsigned long 
> size)
> +void __init copy_from_early_mem(void *dest, phys_addr_t src, unsigned long 
> size,
> +   enum memremap_owner owner)
>  {
> unsigned long slop, clen;
> char *p;
> @@ -246,7 +262,7 @@ void __init copy_from_early_mem(void *dest, phys_addr_t 
> src, unsigned long size)
> clen = size;
> if (clen > MAX_MAP_CHUNK - slop)
> clen = MAX_MAP_CHUNK - slop;
> -   p = early_memremap(src & PAGE_MASK, clen + slop);
> +   p = early_memremap(src & PAGE_MASK, clen + slop, owner);
> memcpy(dest, p + slop, clen);
> early_memunmap(p, clen + slop);
> dest += clen;
> @@ -265,12 +281,14 @@ early_ioremap(resource_size_t phys_addr, unsigned long 
> size)
>
>  /* Remap memory */
>  void __init *
> -early_memremap(resource_size_t phys_addr, unsigned long size)
> +early_memremap(resource_size_t phys_addr, unsigned long size,
> +  enum memremap_owner owner)
>  {
> return (void *)phys_addr;
>  }
>  void __init *
> -early_memremap_ro(resource_size_t phys_addr, unsigned long size)
> +early_memremap_ro(resource_size_t phys_addr, unsigned long size,
> + enum memremap_owner owner)
>  {
> return (void *)phys_addr;
>  }
>

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

[Borislav Petkov]

On Mon, Sep 12, 2016 at 10:14:59AM -0500, Tom Lendacky wrote:
> I did run checkpatch against everything, but was always under the
> assumption that I shouldn't change existing warnings/errors like this.
> If it's considered ok since I'm touching that line of code then I'll
> take care of those situations.

Yeah, normally we fix sensible checkpatch warnings/errors when we touch
the code so please do correct them :-)

Thanks.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

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

[Borislav Petkov]

On Mon, Sep 12, 2016 at 10:14:59AM -0500, Tom Lendacky wrote:
> I did run checkpatch against everything, but was always under the
> assumption that I shouldn't change existing warnings/errors like this.
> If it's considered ok since I'm touching that line of code then I'll
> take care of those situations.

Yeah, normally we fix sensible checkpatch warnings/errors when we touch
the code so please do correct them :-)

Thanks.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

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

[Tom Lendacky]

On 09/09/2016 11:38 AM, Borislav Petkov wrote:
> On Mon, Aug 22, 2016 at 05:37:38PM -0500, 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.
>>
>> Signed-off-by: Tom Lendacky 
>> ---
> 
> ...
> 
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index 031db21..e3bdc5a 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -419,6 +419,25 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr)
>>  iounmap((void __iomem *)((unsigned long)addr & PAGE_MASK));
>>  }
>>  
>> +/*
>> + * Architecure override of __weak function to adjust the protection 
>> attributes
>> + * used when remapping memory.
>> + */
>> +pgprot_t __init early_memremap_pgprot_adjust(resource_size_t phys_addr,
>> + unsigned long size,
>> + enum memremap_owner owner,
>> + pgprot_t prot)
>> +{
>> +/*
>> + * If memory encryption is enabled and BOOT_DATA is being mapped
>> + * then remove the encryption bit.
>> + */
>> +if (_PAGE_ENC && (owner == BOOT_DATA))
>> +prot = __pgprot(pgprot_val(prot) & ~_PAGE_ENC);
>> +
>> +return prot;
>> +}
>> +
> 
> Hmm, so AFAICT, only arch/x86/xen needs KERNEL_DATA and everything else
> is BOOT_DATA.
> 
> So instead of touching so many files and changing early_memremap(),
> why can't you remove _PAGE_ENC by default on x86 and define a specific
> early_memremap() for arch/x86/xen/ which you call there?
> 
> That would make this patch soo much smaller and the change simpler.

Yes it would.  I'll take a look into that.

> 
> ...
> 
>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>> index 5a2631a..f9286c6 100644
>> --- a/drivers/firmware/efi/efi.c
>> +++ b/drivers/firmware/efi/efi.c
>> @@ -386,7 +386,7 @@ int __init efi_mem_desc_lookup(u64 phys_addr, 
>> efi_memory_desc_t *out_md)
>>   * So just always get our own virtual map on the CPU.
>>   *
>>   */
>> -md = early_memremap(p, sizeof (*md));
>> +md = early_memremap(p, sizeof (*md), BOOT_DATA);
> 
> WARNING: space prohibited between function name and open parenthesis '('
> #432: FILE: drivers/firmware/efi/efi.c:389:
> +   md = early_memremap(p, sizeof (*md), BOOT_DATA);
> 
> Please integrate checkpatch.pl into your workflow so that you can catch
> small style nits like this. And don't take its output too seriously... :-)

I did run checkpatch against everything, but was always under the
assumption that I shouldn't change existing warnings/errors like this.
If it's considered ok since I'm touching that line of code then I'll
take care of those situations.

Thanks,
Tom

> 
>>  if (!md) {
>>  pr_err_once("early_memremap(%pa, %zu) failed.\n",
>>  , sizeof (*md));
>> @@ -501,7 +501,8 @@ int __init efi_config_parse_tables(void *config_tables, 
>> int count, int sz,
>>  if (efi.properties_table != EFI_INVALID_TABLE_ADDR) {
>>  efi_properties_table_t *tbl;
>>  
>> -tbl = early_memremap(efi.properties_table, sizeof(*tbl));
>> +tbl = early_memremap(efi.properties_table, sizeof(*tbl),
>> + BOOT_DATA);
>>  if (tbl == NULL) {
>>  pr_err("Could not map Properties table!\n");
>>  return -ENOMEM;

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

[Tom Lendacky]

On 09/09/2016 11:38 AM, Borislav Petkov wrote:
> On Mon, Aug 22, 2016 at 05:37:38PM -0500, 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.
>>
>> Signed-off-by: Tom Lendacky 
>> ---
> 
> ...
> 
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index 031db21..e3bdc5a 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -419,6 +419,25 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr)
>>  iounmap((void __iomem *)((unsigned long)addr & PAGE_MASK));
>>  }
>>  
>> +/*
>> + * Architecure override of __weak function to adjust the protection 
>> attributes
>> + * used when remapping memory.
>> + */
>> +pgprot_t __init early_memremap_pgprot_adjust(resource_size_t phys_addr,
>> + unsigned long size,
>> + enum memremap_owner owner,
>> + pgprot_t prot)
>> +{
>> +/*
>> + * If memory encryption is enabled and BOOT_DATA is being mapped
>> + * then remove the encryption bit.
>> + */
>> +if (_PAGE_ENC && (owner == BOOT_DATA))
>> +prot = __pgprot(pgprot_val(prot) & ~_PAGE_ENC);
>> +
>> +return prot;
>> +}
>> +
> 
> Hmm, so AFAICT, only arch/x86/xen needs KERNEL_DATA and everything else
> is BOOT_DATA.
> 
> So instead of touching so many files and changing early_memremap(),
> why can't you remove _PAGE_ENC by default on x86 and define a specific
> early_memremap() for arch/x86/xen/ which you call there?
> 
> That would make this patch soo much smaller and the change simpler.

Yes it would.  I'll take a look into that.

> 
> ...
> 
>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>> index 5a2631a..f9286c6 100644
>> --- a/drivers/firmware/efi/efi.c
>> +++ b/drivers/firmware/efi/efi.c
>> @@ -386,7 +386,7 @@ int __init efi_mem_desc_lookup(u64 phys_addr, 
>> efi_memory_desc_t *out_md)
>>   * So just always get our own virtual map on the CPU.
>>   *
>>   */
>> -md = early_memremap(p, sizeof (*md));
>> +md = early_memremap(p, sizeof (*md), BOOT_DATA);
> 
> WARNING: space prohibited between function name and open parenthesis '('
> #432: FILE: drivers/firmware/efi/efi.c:389:
> +   md = early_memremap(p, sizeof (*md), BOOT_DATA);
> 
> Please integrate checkpatch.pl into your workflow so that you can catch
> small style nits like this. And don't take its output too seriously... :-)

I did run checkpatch against everything, but was always under the
assumption that I shouldn't change existing warnings/errors like this.
If it's considered ok since I'm touching that line of code then I'll
take care of those situations.

Thanks,
Tom

> 
>>  if (!md) {
>>  pr_err_once("early_memremap(%pa, %zu) failed.\n",
>>  , sizeof (*md));
>> @@ -501,7 +501,8 @@ int __init efi_config_parse_tables(void *config_tables, 
>> int count, int sz,
>>  if (efi.properties_table != EFI_INVALID_TABLE_ADDR) {
>>  efi_properties_table_t *tbl;
>>  
>> -tbl = early_memremap(efi.properties_table, sizeof(*tbl));
>> +tbl = early_memremap(efi.properties_table, sizeof(*tbl),
>> + BOOT_DATA);
>>  if (tbl == NULL) {
>>  pr_err("Could not map Properties table!\n");
>>  return -ENOMEM;

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

[Borislav Petkov]

On Mon, Aug 22, 2016 at 05:37:38PM -0500, 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.
> 
> Signed-off-by: Tom Lendacky 
> ---

...

> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 031db21..e3bdc5a 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -419,6 +419,25 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr)
>   iounmap((void __iomem *)((unsigned long)addr & PAGE_MASK));
>  }
>  
> +/*
> + * Architecure override of __weak function to adjust the protection 
> attributes
> + * used when remapping memory.
> + */
> +pgprot_t __init early_memremap_pgprot_adjust(resource_size_t phys_addr,
> +  unsigned long size,
> +  enum memremap_owner owner,
> +  pgprot_t prot)
> +{
> + /*
> +  * If memory encryption is enabled and BOOT_DATA is being mapped
> +  * then remove the encryption bit.
> +  */
> + if (_PAGE_ENC && (owner == BOOT_DATA))
> + prot = __pgprot(pgprot_val(prot) & ~_PAGE_ENC);
> +
> + return prot;
> +}
> +

Hmm, so AFAICT, only arch/x86/xen needs KERNEL_DATA and everything else
is BOOT_DATA.

So instead of touching so many files and changing early_memremap(),
why can't you remove _PAGE_ENC by default on x86 and define a specific
early_memremap() for arch/x86/xen/ which you call there?

That would make this patch soo much smaller and the change simpler.

...

> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 5a2631a..f9286c6 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -386,7 +386,7 @@ int __init efi_mem_desc_lookup(u64 phys_addr, 
> efi_memory_desc_t *out_md)
>* So just always get our own virtual map on the CPU.
>*
>*/
> - md = early_memremap(p, sizeof (*md));
> + md = early_memremap(p, sizeof (*md), BOOT_DATA);

WARNING: space prohibited between function name and open parenthesis '('
#432: FILE: drivers/firmware/efi/efi.c:389:
+   md = early_memremap(p, sizeof (*md), BOOT_DATA);

Please integrate checkpatch.pl into your workflow so that you can catch
small style nits like this. And don't take its output too seriously... :-)

>   if (!md) {
>   pr_err_once("early_memremap(%pa, %zu) failed.\n",
>   , sizeof (*md));
> @@ -501,7 +501,8 @@ int __init efi_config_parse_tables(void *config_tables, 
> int count, int sz,
>   if (efi.properties_table != EFI_INVALID_TABLE_ADDR) {
>   efi_properties_table_t *tbl;
>  
> - tbl = early_memremap(efi.properties_table, sizeof(*tbl));
> + tbl = early_memremap(efi.properties_table, sizeof(*tbl),
> +  BOOT_DATA);
>   if (tbl == NULL) {
>   pr_err("Could not map Properties table!\n");
>   return -ENOMEM;
-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

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

[Borislav Petkov]

On Mon, Aug 22, 2016 at 05:37:38PM -0500, 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.
> 
> Signed-off-by: Tom Lendacky 
> ---

...

> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 031db21..e3bdc5a 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -419,6 +419,25 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr)
>   iounmap((void __iomem *)((unsigned long)addr & PAGE_MASK));
>  }
>  
> +/*
> + * Architecure override of __weak function to adjust the protection 
> attributes
> + * used when remapping memory.
> + */
> +pgprot_t __init early_memremap_pgprot_adjust(resource_size_t phys_addr,
> +  unsigned long size,
> +  enum memremap_owner owner,
> +  pgprot_t prot)
> +{
> + /*
> +  * If memory encryption is enabled and BOOT_DATA is being mapped
> +  * then remove the encryption bit.
> +  */
> + if (_PAGE_ENC && (owner == BOOT_DATA))
> + prot = __pgprot(pgprot_val(prot) & ~_PAGE_ENC);
> +
> + return prot;
> +}
> +

Hmm, so AFAICT, only arch/x86/xen needs KERNEL_DATA and everything else
is BOOT_DATA.

So instead of touching so many files and changing early_memremap(),
why can't you remove _PAGE_ENC by default on x86 and define a specific
early_memremap() for arch/x86/xen/ which you call there?

That would make this patch soo much smaller and the change simpler.

...

> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 5a2631a..f9286c6 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -386,7 +386,7 @@ int __init efi_mem_desc_lookup(u64 phys_addr, 
> efi_memory_desc_t *out_md)
>* So just always get our own virtual map on the CPU.
>*
>*/
> - md = early_memremap(p, sizeof (*md));
> + md = early_memremap(p, sizeof (*md), BOOT_DATA);

WARNING: space prohibited between function name and open parenthesis '('
#432: FILE: drivers/firmware/efi/efi.c:389:
+   md = early_memremap(p, sizeof (*md), BOOT_DATA);

Please integrate checkpatch.pl into your workflow so that you can catch
small style nits like this. And don't take its output too seriously... :-)

>   if (!md) {
>   pr_err_once("early_memremap(%pa, %zu) failed.\n",
>   , sizeof (*md));
> @@ -501,7 +501,8 @@ int __init efi_config_parse_tables(void *config_tables, 
> int count, int sz,
>   if (efi.properties_table != EFI_INVALID_TABLE_ADDR) {
>   efi_properties_table_t *tbl;
>  
> - tbl = early_memremap(efi.properties_table, sizeof(*tbl));
> + tbl = early_memremap(efi.properties_table, sizeof(*tbl),
> +  BOOT_DATA);
>   if (tbl == NULL) {
>   pr_err("Could not map Properties table!\n");
>   return -ENOMEM;
-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.