Re: [PATCH v10 20/38] x86, mpparse: Use memremap to map the mpf and mpc data

2017-11-03 Thread Tom Lendacky 

On 11/3/2017 10:12 AM, Tomeu Vizoso wrote:

On 17 July 2017 at 23:10, Tom Lendacky  wrote:

The SMP MP-table is built by UEFI and placed in memory in a decrypted
state. These tables are accessed using a mix of early_memremap(),
early_memunmap(), phys_to_virt() and virt_to_phys(). Change all accesses
to use early_memremap()/early_memunmap(). This allows for proper setting
of the encryption mask so that the data can be successfully accessed when
SME is active.

Reviewed-by: Borislav Petkov 
Signed-off-by: Tom Lendacky 
---
  arch/x86/kernel/mpparse.c | 98 +--
  1 file changed, 70 insertions(+), 28 deletions(-)


Hi there,

today I played a bit with crosvm [0] and noticed that 4.14-rc7 doesn't
boot. git-bisect pointed to this patch, and reverting it indeed gets
things working again.

Anybody has an idea of why this could be?


If you send me your kernel config I'll see if I can reproduce the issue
and debug it.

Thanks,
Tom



Thanks,

Tomeu

[0] https://chromium.googlesource.com/chromiumos/platform/crosvm



diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
index fd37f39..5cbb317 100644
--- a/arch/x86/kernel/mpparse.c
+++ b/arch/x86/kernel/mpparse.c
@@ -429,7 +429,7 @@ static inline void __init construct_default_ISA_mptable(int 
mpc_default_type)
 }
  }

-static struct mpf_intel *mpf_found;
+static unsigned long mpf_base;

  static unsigned long __init get_mpc_size(unsigned long physptr)
  {
@@ -451,6 +451,7 @@ static int __init check_physptr(struct mpf_intel *mpf, 
unsigned int early)

 size = get_mpc_size(mpf->physptr);
 mpc = early_memremap(mpf->physptr, size);
+
 /*
  * Read the physical hardware table.  Anything here will
  * override the defaults.
@@ -497,12 +498,12 @@ static int __init check_physptr(struct mpf_intel *mpf, 
unsigned int early)
   */
  void __init default_get_smp_config(unsigned int early)
  {
-   struct mpf_intel *mpf = mpf_found;
+   struct mpf_intel *mpf;

 if (!smp_found_config)
 return;

-   if (!mpf)
+   if (!mpf_base)
 return;

 if (acpi_lapic && early)
@@ -515,6 +516,12 @@ void __init default_get_smp_config(unsigned int early)
 if (acpi_lapic && acpi_ioapic)
 return;

+   mpf = early_memremap(mpf_base, sizeof(*mpf));
+   if (!mpf) {
+   pr_err("MPTABLE: error mapping MP table\n");
+   return;
+   }
+
 pr_info("Intel MultiProcessor Specification v1.%d\n",
 mpf->specification);
  #if defined(CONFIG_X86_LOCAL_APIC) && defined(CONFIG_X86_32)
@@ -529,7 +536,7 @@ void __init default_get_smp_config(unsigned int early)
 /*
  * Now see if we need to read further.
  */
-   if (mpf->feature1 != 0) {
+   if (mpf->feature1) {
 if (early) {
 /*
  * local APIC has default address
@@ -542,8 +549,10 @@ void __init default_get_smp_config(unsigned int early)
 construct_default_ISA_mptable(mpf->feature1);

 } else if (mpf->physptr) {
-   if (check_physptr(mpf, early))
+   if (check_physptr(mpf, early)) {
+   early_memunmap(mpf, sizeof(*mpf));
 return;
+   }
 } else
 BUG();

@@ -552,6 +561,8 @@ void __init default_get_smp_config(unsigned int early)
 /*
  * Only use the first configuration found.
  */
+
+   early_memunmap(mpf, sizeof(*mpf));
  }

  static void __init smp_reserve_memory(struct mpf_intel *mpf)
@@ -561,15 +572,16 @@ static void __init smp_reserve_memory(struct mpf_intel 
*mpf)

  static int __init smp_scan_config(unsigned long base, unsigned long length)
  {
-   unsigned int *bp = phys_to_virt(base);
+   unsigned int *bp;
 struct mpf_intel *mpf;
-   unsigned long mem;
+   int ret = 0;

 apic_printk(APIC_VERBOSE, "Scan for SMP in [mem %#010lx-%#010lx]\n",
 base, base + length - 1);
 BUILD_BUG_ON(sizeof(*mpf) != 16);

 while (length > 0) {
+   bp = early_memremap(base, length);
 mpf = (struct mpf_intel *)bp;
 if ((*bp == SMP_MAGIC_IDENT) &&
 (mpf->length == 1) &&
@@ -579,24 +591,26 @@ static int __init smp_scan_config(unsigned long base, 
unsigned long length)
  #ifdef CONFIG_X86_LOCAL_APIC
 smp_found_config = 1;
  #endif
-   mpf_found = mpf;
+   mpf_base = base;

-   pr_info("found SMP MP-table at [mem %#010llx-%#010llx] 
mapped at [%p]\n",
-   (unsigned long long) virt_to_phys(mpf),
-   (unsigned long long) virt_to_phys(mpf) +
-

Re: [PATCH v2 0/3] Call GetEventLog before ExitBootServices

2017-11-03 Thread Jarkko Sakkinen 
On Mon, Sep 11, 2017 at 12:00:19PM +0200, Thiebaud Weksteen wrote:
> With TPM 1.2, the ACPI table ("TCPA") has two fields to recover the Event Log
> Area (LAML and LASA). These logs are useful to understand and rebuild the
> final values of PCRs.
> 
> With TPM 2.0, the ACPI table ("TPM2") does not contain these fields anymore.
> The recommended method is now to call the GetEventLog EFI protocol before
> ExitBootServices.
> 
> Implement this method within the EFI stub and create copy of the logs for the
> TPM device. This will create 
> /sys/kernel/security/tpm0/binary_bios_measurements
> for TPM 2.0 devices (similarly to the current behaviour for TPM 1.2 devices).
> 
> ---
> 
> Patchset Changelog:
> 
> Version 2:
> - Move tpm_eventlog.h to top include directory, add commit for this.
> - Use EFI_LOADER_DATA to store the configuration table
> - Whitespace and new lines fixes
> 
> 
> Thiebaud Weksteen (3):
>   tpm: move tpm_eventlog.h outside of drivers folder
>   efi: call get_event_log before ExitBootServices
>   tpm: parse TPM event logs based on EFI table
> 
>  arch/x86/boot/compressed/eboot.c   |  1 +
>  drivers/char/tpm/Makefile  |  2 +-
>  drivers/char/tpm/tpm-chip.c|  3 +-
>  drivers/char/tpm/tpm-interface.c   |  2 +-
>  drivers/char/tpm/tpm.h | 35 --
>  drivers/char/tpm/tpm1_eventlog.c   | 17 +++--
>  drivers/char/tpm/tpm2_eventlog.c   |  2 +-
>  drivers/char/tpm/tpm_acpi.c|  2 +-
>  drivers/char/tpm/tpm_efi.c | 66 ++
>  drivers/char/tpm/tpm_of.c  |  2 +-
>  drivers/firmware/efi/Makefile  |  2 +-
>  drivers/firmware/efi/efi.c |  4 ++
>  drivers/firmware/efi/libstub/Makefile  |  3 +-
>  drivers/firmware/efi/libstub/tpm.c | 81 
> ++
>  drivers/firmware/efi/tpm.c | 39 +++
>  include/linux/efi.h| 50 +
>  {drivers/char/tpm => include/linux}/tpm_eventlog.h | 32 ++---
>  17 files changed, 301 insertions(+), 42 deletions(-)
>  create mode 100644 drivers/char/tpm/tpm_efi.c
>  create mode 100644 drivers/firmware/efi/tpm.c
>  rename {drivers/char/tpm => include/linux}/tpm_eventlog.h (77%)
> 
> -- 
> 2.14.1.581.gf28d330327-goog
> 

Tested-by: Jarkko Sakkinen 
Reviewed-by: Jarkko Sakkinen 

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 20/38] x86, mpparse: Use memremap to map the mpf and mpc data

2017-11-03 Thread Tomeu Vizoso 
On 17 July 2017 at 23:10, Tom Lendacky  wrote:
> The SMP MP-table is built by UEFI and placed in memory in a decrypted
> state. These tables are accessed using a mix of early_memremap(),
> early_memunmap(), phys_to_virt() and virt_to_phys(). Change all accesses
> to use early_memremap()/early_memunmap(). This allows for proper setting
> of the encryption mask so that the data can be successfully accessed when
> SME is active.
>
> Reviewed-by: Borislav Petkov 
> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/kernel/mpparse.c | 98 
> +--
>  1 file changed, 70 insertions(+), 28 deletions(-)

Hi there,

today I played a bit with crosvm [0] and noticed that 4.14-rc7 doesn't
boot. git-bisect pointed to this patch, and reverting it indeed gets
things working again.

Anybody has an idea of why this could be?

Thanks,

Tomeu

[0] https://chromium.googlesource.com/chromiumos/platform/crosvm

>
> diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
> index fd37f39..5cbb317 100644
> --- a/arch/x86/kernel/mpparse.c
> +++ b/arch/x86/kernel/mpparse.c
> @@ -429,7 +429,7 @@ static inline void __init 
> construct_default_ISA_mptable(int mpc_default_type)
> }
>  }
>
> -static struct mpf_intel *mpf_found;
> +static unsigned long mpf_base;
>
>  static unsigned long __init get_mpc_size(unsigned long physptr)
>  {
> @@ -451,6 +451,7 @@ static int __init check_physptr(struct mpf_intel *mpf, 
> unsigned int early)
>
> size = get_mpc_size(mpf->physptr);
> mpc = early_memremap(mpf->physptr, size);
> +
> /*
>  * Read the physical hardware table.  Anything here will
>  * override the defaults.
> @@ -497,12 +498,12 @@ static int __init check_physptr(struct mpf_intel *mpf, 
> unsigned int early)
>   */
>  void __init default_get_smp_config(unsigned int early)
>  {
> -   struct mpf_intel *mpf = mpf_found;
> +   struct mpf_intel *mpf;
>
> if (!smp_found_config)
> return;
>
> -   if (!mpf)
> +   if (!mpf_base)
> return;
>
> if (acpi_lapic && early)
> @@ -515,6 +516,12 @@ void __init default_get_smp_config(unsigned int early)
> if (acpi_lapic && acpi_ioapic)
> return;
>
> +   mpf = early_memremap(mpf_base, sizeof(*mpf));
> +   if (!mpf) {
> +   pr_err("MPTABLE: error mapping MP table\n");
> +   return;
> +   }
> +
> pr_info("Intel MultiProcessor Specification v1.%d\n",
> mpf->specification);
>  #if defined(CONFIG_X86_LOCAL_APIC) && defined(CONFIG_X86_32)
> @@ -529,7 +536,7 @@ void __init default_get_smp_config(unsigned int early)
> /*
>  * Now see if we need to read further.
>  */
> -   if (mpf->feature1 != 0) {
> +   if (mpf->feature1) {
> if (early) {
> /*
>  * local APIC has default address
> @@ -542,8 +549,10 @@ void __init default_get_smp_config(unsigned int early)
> construct_default_ISA_mptable(mpf->feature1);
>
> } else if (mpf->physptr) {
> -   if (check_physptr(mpf, early))
> +   if (check_physptr(mpf, early)) {
> +   early_memunmap(mpf, sizeof(*mpf));
> return;
> +   }
> } else
> BUG();
>
> @@ -552,6 +561,8 @@ void __init default_get_smp_config(unsigned int early)
> /*
>  * Only use the first configuration found.
>  */
> +
> +   early_memunmap(mpf, sizeof(*mpf));
>  }
>
>  static void __init smp_reserve_memory(struct mpf_intel *mpf)
> @@ -561,15 +572,16 @@ static void __init smp_reserve_memory(struct mpf_intel 
> *mpf)
>
>  static int __init smp_scan_config(unsigned long base, unsigned long length)
>  {
> -   unsigned int *bp = phys_to_virt(base);
> +   unsigned int *bp;
> struct mpf_intel *mpf;
> -   unsigned long mem;
> +   int ret = 0;
>
> apic_printk(APIC_VERBOSE, "Scan for SMP in [mem %#010lx-%#010lx]\n",
> base, base + length - 1);
> BUILD_BUG_ON(sizeof(*mpf) != 16);
>
> while (length > 0) {
> +   bp = early_memremap(base, length);
> mpf = (struct mpf_intel *)bp;
> if ((*bp == SMP_MAGIC_IDENT) &&
> (mpf->length == 1) &&
> @@ -579,24 +591,26 @@ static int __init smp_scan_config(unsigned long base, 
> unsigned long length)
>  #ifdef CONFIG_X86_LOCAL_APIC
> smp_found_config = 1;
>  #endif
> -   mpf_found = mpf;
> +   mpf_base = base;
>
> -   pr_info("found SMP MP-table at [mem 
> %#010llx-%#010llx] mapped at [%p]\n",
> -   (unsigned long long) virt_to_phys(mpf),
> -   (unsigned long long)

Re: [PATCH 3/3] arm64: Add software workaround for Falkor erratum 1041

2017-11-03 Thread Robin Murphy 
On 03/11/17 03:27, Shanker Donthineni wrote:
> The ARM architecture defines the memory locations that are permitted
> to be accessed as the result of a speculative instruction fetch from
> an exception level for which all stages of translation are disabled.
> Specifically, the core is permitted to speculatively fetch from the
> 4KB region containing the current program counter and next 4KB.
> 
> When translation is changed from enabled to disabled for the running
> exception level (SCTLR_ELn[M] changed from a value of 1 to 0), the
> Falkor core may errantly speculatively access memory locations outside
> of the 4KB region permitted by the architecture. The errant memory
> access may lead to one of the following unexpected behaviors.
> 
> 1) A System Error Interrupt (SEI) being raised by the Falkor core due
>to the errant memory access attempting to access a region of memory
>that is protected by a slave-side memory protection unit.
> 2) Unpredictable device behavior due to a speculative read from device
>memory. This behavior may only occur if the instruction cache is
>disabled prior to or coincident with translation being changed from
>enabled to disabled.
> 
> To avoid the errant behavior, software must execute an ISB immediately
> prior to executing the MSR that will change SCTLR_ELn[M] from 1 to 0.
> 
> Signed-off-by: Shanker Donthineni 
> ---
>  Documentation/arm64/silicon-errata.txt |  1 +
>  arch/arm64/Kconfig | 10 ++
>  arch/arm64/include/asm/assembler.h | 17 +
>  arch/arm64/include/asm/cpucaps.h   |  3 ++-
>  arch/arm64/kernel/cpu_errata.c | 16 
>  arch/arm64/kernel/efi-entry.S  |  4 ++--
>  arch/arm64/kernel/head.S   |  4 ++--
>  7 files changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/arm64/silicon-errata.txt 
> b/Documentation/arm64/silicon-errata.txt
> index 66e8ce1..704770c0 100644
> --- a/Documentation/arm64/silicon-errata.txt
> +++ b/Documentation/arm64/silicon-errata.txt
> @@ -74,3 +74,4 @@ stable kernels.
>  | Qualcomm Tech. | Falkor v1   | E1003   | 
> QCOM_FALKOR_ERRATUM_1003|
>  | Qualcomm Tech. | Falkor v1   | E1009   | 
> QCOM_FALKOR_ERRATUM_1009|
>  | Qualcomm Tech. | QDF2400 ITS | E0065   | 
> QCOM_QDF2400_ERRATUM_0065   |
> +| Qualcomm Tech. | Falkor v{1,2}   | E1041   | 
> QCOM_FALKOR_ERRATUM_1041|
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 0df64a6..7e933fb 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -539,6 +539,16 @@ config QCOM_QDF2400_ERRATUM_0065
>  
> If unsure, say Y.
>  
> +config QCOM_FALKOR_ERRATUM_1041
> + bool "Falkor E1041: Speculative instruction fetches might cause errant 
> memory access"
> + default y
> + help
> +   Falkor CPU may speculatively fetch instructions from an improper
> +   memory location when MMU translation is changed from SCTLR_ELn[M]=1
> +   to SCTLR_ELn[M]=0. Prefix an ISB instruction to fix the problem.
> +
> +   If unsure, say Y.
> +
>  endmenu
>  
>  
> diff --git a/arch/arm64/include/asm/assembler.h 
> b/arch/arm64/include/asm/assembler.h
> index b6dfb4f..4c91efb 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  /*
>   * Enable and disable interrupts.
> @@ -514,6 +515,22 @@
>   *   reg: the value to be written.
>   */
>   .macro  write_sctlr, eln, reg
> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
> +alternative_if ARM64_WORKAROUND_QCOM_FALKOR_E1041
> + tbnz\reg, #0, 8000f  // enable MMU?

Do we really need the branch here? It's not like enabling the MMU is
something we do on the syscall fastpath, and I can't imagine an extra
ISB hurts much (and is probably comparable to a mispredicted branch
anyway). In fact, is there any noticeable hit on other
microarchitectures if we save the alternative bother and just do it
unconditionally always?

Robin.

> + isb
> +8000:
> +alternative_else_nop_endif
> +#endif
> + msr sctlr_\eln, \reg
> + .endm
> +
> + .macro  early_write_sctlr, eln, reg
> +#ifdef CONFIG_QCOM_FALKOR_ERRATUM_1041
> + tbnz\reg, #0, 8000f  // enable MMU?
> + isb
> +8000:
> +#endif
>   msr sctlr_\eln, \reg
>   .endm
>  
> diff --git a/arch/arm64/include/asm/cpucaps.h 
> b/arch/arm64/include/asm/cpucaps.h
> index 8da6216..7f7a59d 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -40,7 +40,8 @@
>  #define ARM64_WORKAROUND_858921  19
>  #define ARM64_WORKAROUND_CAVIUM_3011520
>  #define ARM64_HAS_DCPOP  21
> +#define ARM64_WORKAROUND_QCOM_FALKOR_E1041   22
>  
> -#define ARM64_NCAPS  22
> +#define ARM64_NCAPS