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

2017-05-31 Thread Borislav Petkov 
On Tue, May 30, 2017 at 11:46:52AM -0500, Tom Lendacky wrote:
> Check if you have CONFIG_DEBUG_SECTION_MISMATCH=y

$ grep MISM .config
CONFIG_DEBUG_SECTION_MISMATCH=y
CONFIG_SECTION_MISMATCH_WARN_ONLY=y

Still no joy.

Can you give me your .config?

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

2017-05-30 Thread Tom Lendacky 

On 5/26/2017 11:35 AM, Borislav Petkov wrote:

On Fri, May 26, 2017 at 11:22:36AM -0500, Tom Lendacky wrote:

In addition to the same issue as efi.memmap.phys_map, efi_phys has
the __initdata attribute so it will be released/freed which will cause
problems in checks performed afterwards.


Sounds to me like we should drop the __initdata attr and prepare them
much earlier for use by the SME code.


Probably something we can look at for a follow-on patch.

Thanks,
Tom





___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

2017-05-30 Thread Tom Lendacky 

On 5/21/2017 2:16 AM, Borislav Petkov wrote:

On Fri, May 19, 2017 at 03:50:32PM -0500, Tom Lendacky wrote:

The "worker" function would be doing the loop through the setup data,
but since the setup data is mapped inside the loop I can't do the __init
calling the non-init function and still hope to consolidate the code.
Maybe I'm missing something here...


Hmm, I see what you mean. But the below change ontop doesn't fire any
warnings here. Maybe your .config has something set which I don't...


Check if you have CONFIG_DEBUG_SECTION_MISMATCH=y

Thanks,
Tom



---
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 55317ba3b6dc..199c983192ae 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -515,71 +515,50 @@ static bool memremap_is_efi_data(resource_size_t 
phys_addr,
   * Examine the physical address to determine if it is boot data by checking
   * it against the boot params setup_data chain.
   */
-static bool memremap_is_setup_data(resource_size_t phys_addr,
-  unsigned long size)
+static bool
+__memremap_is_setup_data(resource_size_t phys_addr, unsigned long size, bool 
early)
  {
struct setup_data *data;
u64 paddr, paddr_next;
+   u32 len;
  
  	paddr = boot_params.hdr.setup_data;

while (paddr) {
-   bool is_setup_data = false;
  
  		if (phys_addr == paddr)

return true;
  
-		data = memremap(paddr, sizeof(*data),

-   MEMREMAP_WB | MEMREMAP_DEC);
+   if (early)
+   data = early_memremap_decrypted(paddr, sizeof(*data));
+   else
+   data = memremap(paddr, sizeof(*data), MEMREMAP_WB | 
MEMREMAP_DEC);
  
  		paddr_next = data->next;

+   len = data->len;
  
-		if ((phys_addr > paddr) && (phys_addr < (paddr + data->len)))

-   is_setup_data = true;
+   if (early)
+   early_memunmap(data, sizeof(*data));
+   else
+   memunmap(data);
  
-		memunmap(data);
  
-		if (is_setup_data)

+   if ((phys_addr > paddr) && (phys_addr < (paddr + data->len)))
return true;
  
  		paddr = paddr_next;

}
-
return false;
  }
  
-/*

- * Examine the physical address to determine if it is boot data by checking
- * it against the boot params setup_data chain (early boot version).
- */
  static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
unsigned long size)
  {
-   struct setup_data *data;
-   u64 paddr, paddr_next;
-
-   paddr = boot_params.hdr.setup_data;
-   while (paddr) {
-   bool is_setup_data = false;
-
-   if (phys_addr == paddr)
-   return true;
-
-   data = early_memremap_decrypted(paddr, sizeof(*data));
-
-   paddr_next = data->next;
-
-   if ((phys_addr > paddr) && (phys_addr < (paddr + data->len)))
-   is_setup_data = true;
-
-   early_memunmap(data, sizeof(*data));
-
-   if (is_setup_data)
-   return true;
-
-   paddr = paddr_next;
-   }
+   return __memremap_is_setup_data(phys_addr, size, true);
+}
  
-	return false;

+static bool memremap_is_setup_data(resource_size_t phys_addr, unsigned long 
size)
+{
+   return __memremap_is_setup_data(phys_addr, size, false);
  }
  
  /*




___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

2017-05-26 Thread Tom Lendacky 

On 5/18/2017 2:50 PM, Matt Fleming wrote:

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


Unfortunately memremap_is_efi_data() is called before the efi structure
gets initialized, so I can't use that value.



And the physical address of the system table should be in
'efi_phys.systab'. See efi_init().


In addition to the same issue as efi.memmap.phys_map, efi_phys has
the __initdata attribute so it will be released/freed which will cause
problems in checks performed afterwards.

Thanks,
Tom





___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

2017-05-21 Thread Borislav Petkov 
On Fri, May 19, 2017 at 03:50:32PM -0500, Tom Lendacky wrote:
> The "worker" function would be doing the loop through the setup data,
> but since the setup data is mapped inside the loop I can't do the __init
> calling the non-init function and still hope to consolidate the code.
> Maybe I'm missing something here...

Hmm, I see what you mean. But the below change ontop doesn't fire any
warnings here. Maybe your .config has something set which I don't...

---
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 55317ba3b6dc..199c983192ae 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -515,71 +515,50 @@ static bool memremap_is_efi_data(resource_size_t 
phys_addr,
  * Examine the physical address to determine if it is boot data by checking
  * it against the boot params setup_data chain.
  */
-static bool memremap_is_setup_data(resource_size_t phys_addr,
-  unsigned long size)
+static bool
+__memremap_is_setup_data(resource_size_t phys_addr, unsigned long size, bool 
early)
 {
struct setup_data *data;
u64 paddr, paddr_next;
+   u32 len;
 
paddr = boot_params.hdr.setup_data;
while (paddr) {
-   bool is_setup_data = false;
 
if (phys_addr == paddr)
return true;
 
-   data = memremap(paddr, sizeof(*data),
-   MEMREMAP_WB | MEMREMAP_DEC);
+   if (early)
+   data = early_memremap_decrypted(paddr, sizeof(*data));
+   else
+   data = memremap(paddr, sizeof(*data), MEMREMAP_WB | 
MEMREMAP_DEC);
 
paddr_next = data->next;
+   len = data->len;
 
-   if ((phys_addr > paddr) && (phys_addr < (paddr + data->len)))
-   is_setup_data = true;
+   if (early)
+   early_memunmap(data, sizeof(*data));
+   else
+   memunmap(data);
 
-   memunmap(data);
 
-   if (is_setup_data)
+   if ((phys_addr > paddr) && (phys_addr < (paddr + data->len)))
return true;
 
paddr = paddr_next;
}
-
return false;
 }
 
-/*
- * Examine the physical address to determine if it is boot data by checking
- * it against the boot params setup_data chain (early boot version).
- */
 static bool __init early_memremap_is_setup_data(resource_size_t phys_addr,
unsigned long size)
 {
-   struct setup_data *data;
-   u64 paddr, paddr_next;
-
-   paddr = boot_params.hdr.setup_data;
-   while (paddr) {
-   bool is_setup_data = false;
-
-   if (phys_addr == paddr)
-   return true;
-
-   data = early_memremap_decrypted(paddr, sizeof(*data));
-
-   paddr_next = data->next;
-
-   if ((phys_addr > paddr) && (phys_addr < (paddr + data->len)))
-   is_setup_data = true;
-
-   early_memunmap(data, sizeof(*data));
-
-   if (is_setup_data)
-   return true;
-
-   paddr = paddr_next;
-   }
+   return __memremap_is_setup_data(phys_addr, size, true);
+}
 
-   return false;
+static bool memremap_is_setup_data(resource_size_t phys_addr, unsigned long 
size)
+{
+   return __memremap_is_setup_data(phys_addr, size, false);
 }
 
 /*

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

2017-05-19 Thread Tom Lendacky 

On 5/18/2017 4:02 AM, Borislav Petkov wrote:

On Wed, May 17, 2017 at 01:54:39PM -0500, Tom Lendacky wrote:

I was worried what the compiler might do when CONFIG_EFI is not set,
but it appears to take care of it. I'll double check though.


There's a efi_enabled() !CONFIG_EFI version too, so should be fine.


I may introduce a length variable to capture data->len right after
paddr_next is set and then have just a single memunmap() call before
the if check.


Yap.


I tried that, but calling an "__init" function (early_memremap()) from
a non "__init" function generated warnings. I suppose I can pass in a
function for the map and unmap but that looks worse to me (also the
unmap functions take different arguments).


No, the other way around: the __init function should call the non-init
one and you need the non-init one anyway for memremap_is_setup_data().



The "worker" function would be doing the loop through the setup data,
but since the setup data is mapped inside the loop I can't do the __init
calling the non-init function and still hope to consolidate the code.
Maybe I'm missing something here...

Thanks,
Tom


This is like the chicken and the egg scenario. In order to determine if
an address is setup data I have to explicitly map the setup data chain
as decrypted. In order to do that I have to supply a flag to explicitly
map the data decrypted otherwise I wind up back in the
memremap_is_setup_data() function again and again and again...


Oh, fun.



___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

2017-05-18 Thread Borislav Petkov 
On Wed, May 17, 2017 at 01:54:39PM -0500, Tom Lendacky wrote:
> I was worried what the compiler might do when CONFIG_EFI is not set,
> but it appears to take care of it. I'll double check though.

There's a efi_enabled() !CONFIG_EFI version too, so should be fine.

> I may introduce a length variable to capture data->len right after
> paddr_next is set and then have just a single memunmap() call before
> the if check.

Yap.

> I tried that, but calling an "__init" function (early_memremap()) from
> a non "__init" function generated warnings. I suppose I can pass in a
> function for the map and unmap but that looks worse to me (also the
> unmap functions take different arguments).

No, the other way around: the __init function should call the non-init
one and you need the non-init one anyway for memremap_is_setup_data().

> This is like the chicken and the egg scenario. In order to determine if
> an address is setup data I have to explicitly map the setup data chain
> as decrypted. In order to do that I have to supply a flag to explicitly
> map the data decrypted otherwise I wind up back in the
> memremap_is_setup_data() function again and again and again...

Oh, fun.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

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

2017-05-17 Thread Tom Lendacky 

On 5/15/2017 1:35 PM, Borislav Petkov wrote:

On Tue, Apr 18, 2017 at 04:19:21PM -0500, 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.

The early_memremap() support is updated to provide an arch specific


"Update early_memremap() to provide... "


Will do.




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.

The memremap() support is updated to provide an arch specific routine


Ditto. Passive tone always reads harder than an active tone,
"doer"-sentence.


Ditto.




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 
---
 arch/x86/include/asm/io.h |4 +
 arch/x86/mm/ioremap.c |  182 +
 include/linux/io.h|2
 kernel/memremap.c |   20 -
 mm/early_ioremap.c|   18 
 5 files changed, 219 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 7afb0e2..75f2858 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -381,4 +381,8 @@ extern int __must_check arch_phys_wc_add(unsigned long base,
 #define arch_io_reserve_memtype_wc arch_io_reserve_memtype_wc
 #endif

+extern bool arch_memremap_do_ram_remap(resource_size_t offset, size_t size,
+  unsigned long flags);
+#define arch_memremap_do_ram_remap arch_memremap_do_ram_remap
+
 #endif /* _ASM_X86_IO_H */
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 9bfcb1f..bce0604 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -13,6 +13,7 @@
 #include 
 #include 
 #include 
+#include 

 #include 
 #include 
@@ -21,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 

 #include "physaddr.h"

@@ -419,6 +421,186 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr)
iounmap((void __iomem *)((unsigned long)addr & PAGE_MASK));
 }

+/*
+ * Examine the physical address to determine if it is an area of memory
+ * that should be mapped decrypted.  If the memory is not part of the
+ * kernel usable area it was accessed and created decrypted, so these
+ * areas should be mapped decrypted.
+ */
+static bool memremap_should_map_decrypted(resource_size_t phys_addr,
+ unsigned long size)
+{
+   /* Check if the address is outside kernel usable area */
+   switch (e820__get_entry_type(phys_addr, phys_addr + size - 1)) {
+   case E820_TYPE_RESERVED:
+   case E820_TYPE_ACPI:
+   case E820_TYPE_NVS:
+   case E820_TYPE_UNUSABLE:
+   return true;
+   default:
+   break;
+   }
+
+   return false;
+}
+
+/*
+ * Examine the physical address to determine if it is EFI data. Check
+ * it against the boot params structure and EFI tables and memory types.
+ */
+static bool memremap_is_efi_data(resource_size_t phys_addr,
+unsigned long size)
+{
+   u64 paddr;
+
+   /* Check if the address is part of EFI boot/runtime data */
+   if (efi_enabled(EFI_BOOT)) {


Save indentation level:

if (!efi_enabled(EFI_BOOT))
return false;



I was worried what the compiler might do when CONFIG_EFI is not set,
but it appears to take care of it. I'll double check though.




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


+   if (phys_addr == paddr)
+   return true;
+
+   if (efi_table_address_match(phys_addr))
+   return true;
+
+   switch (efi_mem_type(phys_addr)) {
+   case EFI_BOOT_SERVICES_DATA:
+   case EFI_RUNTIME_SERVICES_DATA:
+   return true;
+   default:
+   break;
+   }
+   }
+
+   return false;
+}
+
+/*
+ * Examine the physical address to determine if it is boot data by checking
+ * it against the boot params setup_data

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

2017-05-15 Thread Borislav Petkov 
On Tue, Apr 18, 2017 at 04:19:21PM -0500, 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.
> 
> The early_memremap() support is updated to provide an arch specific

"Update early_memremap() to provide... "

> 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.
> 
> The memremap() support is updated to provide an arch specific routine

Ditto. Passive tone always reads harder than an active tone,
"doer"-sentence.

> 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 
> ---
>  arch/x86/include/asm/io.h |4 +
>  arch/x86/mm/ioremap.c |  182 
> +
>  include/linux/io.h|2 
>  kernel/memremap.c |   20 -
>  mm/early_ioremap.c|   18 
>  5 files changed, 219 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> index 7afb0e2..75f2858 100644
> --- a/arch/x86/include/asm/io.h
> +++ b/arch/x86/include/asm/io.h
> @@ -381,4 +381,8 @@ extern int __must_check arch_phys_wc_add(unsigned long 
> base,
>  #define arch_io_reserve_memtype_wc arch_io_reserve_memtype_wc
>  #endif
>  
> +extern bool arch_memremap_do_ram_remap(resource_size_t offset, size_t size,
> +unsigned long flags);
> +#define arch_memremap_do_ram_remap arch_memremap_do_ram_remap
> +
>  #endif /* _ASM_X86_IO_H */
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 9bfcb1f..bce0604 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -13,6 +13,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -21,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "physaddr.h"
>  
> @@ -419,6 +421,186 @@ void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr)
>   iounmap((void __iomem *)((unsigned long)addr & PAGE_MASK));
>  }
>  
> +/*
> + * Examine the physical address to determine if it is an area of memory
> + * that should be mapped decrypted.  If the memory is not part of the
> + * kernel usable area it was accessed and created decrypted, so these
> + * areas should be mapped decrypted.
> + */
> +static bool memremap_should_map_decrypted(resource_size_t phys_addr,
> +   unsigned long size)
> +{
> + /* Check if the address is outside kernel usable area */
> + switch (e820__get_entry_type(phys_addr, phys_addr + size - 1)) {
> + case E820_TYPE_RESERVED:
> + case E820_TYPE_ACPI:
> + case E820_TYPE_NVS:
> + case E820_TYPE_UNUSABLE:
> + return true;
> + default:
> + break;
> + }
> +
> + return false;
> +}
> +
> +/*
> + * Examine the physical address to determine if it is EFI data. Check
> + * it against the boot params structure and EFI tables and memory types.
> + */
> +static bool memremap_is_efi_data(resource_size_t phys_addr,
> +  unsigned long size)
> +{
> + u64 paddr;
> +
> + /* Check if the address is part of EFI boot/runtime data */
> + if (efi_enabled(EFI_BOOT)) {

Save indentation level:

if (!efi_enabled(EFI_BOOT))
return false;


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

> + if (phys_addr == paddr)
> + return true;
> +
> + if (efi_table_address_match(phys_addr))
> + return true;
> +
> + switch (efi_mem_type(phys_addr)) {
> + case EFI_BOOT_SERVICES_DATA:
> + case EFI_RUNTIME_SERVICES_DATA:
> + return true;
> + default:
> + break;
> + }
> + }
> +
> + return false;
> +}
> +
> +/*
> + * Examine the physical address to determine if it is boot data by checking
> + * it against the boot params setup_data chain.
> + */
> +static bool