[PATCHv2 0/5] efi: detect erroneous firmware IRQ manipulation

[PATCHv2 0/5] efi: detect erroneous firmware IRQ manipulation

[Mark Rutland]

Some firmware erroneously unmask IRQs (and potentially other architecture
specific exceptions) during runtime services functions, in violation of both
common sense and the UEFI specification. This can result in a number of issues
if said exceptions are taken when they are expected to be masked, and
additionally can confuse IRQ tracing if the original mask state is not
restored prior to returning from firmware.

In practice it's difficult to check that firmware never unmasks exceptions, but
we can at least check that the IRQ flags are at least consistent upon entry to
and return from a runtime services function call. This series implements said
check in the shared EFI runtime wrappers code, after an initial round of
refactoring such that this can be generic.

I have left ia64 as-is, without this check, as ia64 has many special cases for
the runtime calls which don't fit well with the generic code, and I don't
expect a new, buggy ia64 firmware to appear soon.

The first time corruption of the IRQ flags is detected, we dump a stack trace,
and set TAINT_FIRMWARE_WORKAROUND. Additionally, and in all subsequent cases,
we log (with ratelimiting) the specific corruption of the flags, and restore
the expected flags to avoid redundant warnings elsewhere.

For example, if the firmware on an arm64 system erroneously unmasked IRQ+FIQ,
we would see warnings in dmesg of the form:

[    5.639616] ------------[ cut here ]------------
[    5.644233] WARNING: CPU: 3 PID: 1120 at drivers/firmware/efi/runtime-wrappers.c:57 efi_call_virt_check_flags+0x84/0x90
[    5.655001] Modules linked in:
[    5.658046] 
[    5.659528] CPU: 3 PID: 1120 Comm: mount Not tainted 4.6.0-rc4+ #6
[    5.674293] task: ffffffc3ecbcc800 ti: ffffffc0fa498000 task.ti: ffffffc0fa498000
[    5.681763] PC is at efi_call_virt_check_flags+0x84/0x90
[    5.687063] LR is at virt_efi_get_next_variable+0x74/0xa0
[    5.692449] pc : [<ffffff8008618ccc>] lr : [<ffffff8008618f2c>] pstate: 20000105
[    5.699831] sp : ffffffc0fa49bbd0
[    5.703133] x29: ffffffc0fa49bbd0 x28: ffffffc0fa498000 
[    5.708436] x27: ffffff8008776000 x26: 0000000000000001 
[    5.713739] x25: ffffff8008bf2000 x24: 0000000000000000 
[    5.719042] x23: ffffffc0fa49bcc0 x22: ffffffc3eb84d400 
[    5.724344] x21: 00000000000001c0 x20: 0000000000000100 
[    5.729646] x19: ffffff8008bf2a20 x18: 0000007fc1f31aa0 
[    5.734948] x17: 0000000000425060 x16: ffffff80081e4578 
[    5.740251] x15: ffffffffffffffff x14: 0000000000000000 
[    5.745553] x13: 0000000000000000 x12: 0000000000000000 
[    5.750855] x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f 
[    5.756157] x9 : 0000000000000000 x8 : 0000000000000000 
[    5.761459] x7 : 0000000000000000 x6 : 0000000000000001 
[    5.766761] x5 : 00000000220e94ba x4 : 0000000022c61190 
[    5.772063] x3 : 0000000000000001 x2 : ffffff8008bc0000 
[    5.777366] x1 : ffffff80089d9d10 x0 : 00000000000001c0 
[    5.782667] 
[    5.784147] ---[ end trace b1612054aa2afdce ]---
[    5.788751] Call trace:
[    5.791186] Exception stack(0xffffffc0fa49ba10 to 0xffffffc0fa49bb30)
[    5.797614] ba00:                                   ffffff8008bf2a20 0000000000000100
[    5.805430] ba20: ffffffc0fa49bbd0 ffffff8008618ccc ffffffc0fa49ba60 00000000220e94be
[    5.813247] ba40: ffffffc0fa49ba70 0000000022c5a208 ffffffc0fa49ba80 0000000000000010
[    5.821063] ba60: 0000000022168ff8 ffffffc0fa49bcc0 ffffffc0fa49baa0 0000000022c4d440
[    5.828880] ba80: ffffffc0fa49baa0 0000000000000010 0000000022168ff8 ffffffc0fa49bcc0
[    5.836696] baa0: ffffffc0fa49bb10 0000000022c4db50 00000000000001c0 ffffff80089d9d10
[    5.844512] bac0: ffffff8008bc0000 0000000000000001 0000000022c61190 00000000220e94ba
[    5.852329] bae0: 0000000000000001 0000000000000000 0000000000000000 0000000000000000
[    5.860145] bb00: 7f7f7f7f7f7f7f7f 0101010101010101 0000000000000000 0000000000000000
[    5.867961] bb20: 0000000000000000 ffffffffffffffff
[    5.872826] [<ffffff8008618ccc>] efi_call_virt_check_flags+0x84/0x90
[    5.879168] [<ffffff8008618f2c>] virt_efi_get_next_variable+0x74/0xa0
[    5.885596] [<ffffff80086182a4>] efivar_init+0x7c/0x2a0
[    5.890810] [<ffffff80082f7c7c>] efivarfs_fill_super+0xac/0xe8
[    5.896633] [<ffffff80081c5e34>] mount_single+0x8c/0xb8
[    5.901846] [<ffffff80082f7bc8>] efivarfs_mount+0x18/0x20
[    5.907232] [<ffffff80081c5f0c>] mount_fs+0x4c/0x168
[    5.912184] [<ffffff80081e0f38>] vfs_kern_mount+0x50/0x118
[    5.917658] [<ffffff80081e3890>] do_mount+0x208/0xc08
[    5.922697] [<ffffff80081e4608>] SyS_mount+0x90/0xf8
[    5.927649] [<ffffff8008085e70>] el0_svc_naked+0x24/0x28
[    5.932960] Disabling lock debugging due to kernel taint
[    5.938263] [Firmware Bug]: IRQ flags corrupted (0x000001c0=>0x00000100) by EFI get_next_variable

Then subsequently:

[    5.938263] [Firmware Bug]: IRQ flags corrupted (0x000001c0=>0x00000100) by EFI get_next_variable
[    5.947143] [Firmware Bug]: IRQ flags corrupted (0x000001c0=>0x00000100) by EFI get_variable

Thanks,
Mark.

Mark Rutland (5):
  efi/runtime-wrappers: add {__,}efi_call_virt templates
  arm64/efi: move to generic {__,}efi_call_virt
  arm/efi: move to generic {__,}efi_call_virt
  x86/efi: move to generic {__,}efi_call_virt
  efi/runtime-wrappers: detect FW irq flag corruption

 arch/arm/include/asm/efi.h              | 20 +++------------
 arch/arm64/include/asm/efi.h            | 21 ++++++----------
 arch/x86/include/asm/efi.h              | 41 +++++++++----------------------
 drivers/firmware/efi/runtime-wrappers.c | 43 +++++++++++++++++++++++++++++++++
 4 files changed, 66 insertions(+), 59 deletions(-)

-- 
1.9.1

[PATCHv2 1/5] efi/runtime-wrappers: add {__,}efi_call_virt templates

[Mark Rutland]

Currently each architecture must implement two macros, efi_call_virt and
__efi_call_virt, which only differ by the presence or absence of a
return type. Otherwise, the logic surrounding the call is identical.

As each architecture must define the entire body of each, we can't place
any generic manipulation (e.g. irq flag validation) in the middle.

This patch adds template implementations of these macros. With these,
arch code can implement three template macros, avoiding reptition for
the void/non-void return cases:

* arch_efi_call_virt_setup

  Sets up the environment for the call (e.g. switching page tables,
  allowing kernel-mode use of floating point, if required).

* arch_efi_call_virt

  Performs the call. The last expression in the macro must be the call
  itself, allowing the logic to be shared by the void and non-void
  cases.

* arch_efi_call_virt_teardown

  Restores the usual kernel environment once the call has returned.

While the savings from repition are minimal, we additionally gain the
ability to add common code around the call with the call enviroment set
up. This can be used to detect common firmware issues (e.g. bad irq mask
management).

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: linux-efi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/firmware/efi/runtime-wrappers.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index de69530..1b9fa54 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -20,6 +20,27 @@
 #include <linux/spinlock.h>
 #include <asm/efi.h>
 
+
+#ifndef efi_call_virt
+#define efi_call_virt(f, args...)					\
+({									\
+	efi_status_t __s;						\
+	arch_efi_call_virt_setup();					\
+	__s = arch_efi_call_virt(f, args);				\
+	arch_efi_call_virt_teardown();					\
+	__s;								\
+})
+#endif
+
+#ifndef __efi_call_virt
+#define __efi_call_virt(f, args...)					\
+({									\
+	arch_efi_call_virt_setup();					\
+	arch_efi_call_virt(f, args);					\
+	arch_efi_call_virt_teardown();					\
+})
+#endif
+
 /*
  * According to section 7.1 of the UEFI spec, Runtime Services are not fully
  * reentrant, and there are particular combinations of calls that need to be
-- 
1.9.1

[PATCHv2 2/5] arm64/efi: move to generic {__,}efi_call_virt

[Mark Rutland]

Now there's a common template for {__,}efi_call_virt, remove the
duplicate logic from the arm64 efi code.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Will Deacon <will.deacon@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-efi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 arch/arm64/include/asm/efi.h | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 8e88a69..f4f71224 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -14,28 +14,21 @@ extern void efi_init(void);
 
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
 
-#define efi_call_virt(f, ...)						\
+#define arch_efi_call_virt_setup()					\
 ({									\
-	efi_##f##_t *__f;						\
-	efi_status_t __s;						\
-									\
 	kernel_neon_begin();						\
 	efi_virtmap_load();						\
-	__f = efi.systab->runtime->f;					\
-	__s = __f(__VA_ARGS__);						\
-	efi_virtmap_unload();						\
-	kernel_neon_end();						\
-	__s;								\
 })
 
-#define __efi_call_virt(f, ...)						\
+#define arch_efi_call_virt(f, args...)					\
 ({									\
 	efi_##f##_t *__f;						\
-									\
-	kernel_neon_begin();						\
-	efi_virtmap_load();						\
 	__f = efi.systab->runtime->f;					\
-	__f(__VA_ARGS__);						\
+	__f(args);							\
+})
+
+#define arch_efi_call_virt_teardown()					\
+({									\
 	efi_virtmap_unload();						\
 	kernel_neon_end();						\
 })
-- 
1.9.1

[PATCHv2 3/5] arm/efi: move to generic {__,}efi_call_virt

[Mark Rutland]

Now there's a common template for {__,}efi_call_virt, remove the
duplicate logic from the arm efi code.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-efi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 arch/arm/include/asm/efi.h | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index e0eea72..e1461fc 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -23,26 +23,14 @@ void efi_init(void);
 
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
 
-#define efi_call_virt(f, ...)						\
-({									\
-	efi_##f##_t *__f;						\
-	efi_status_t __s;						\
-									\
-	efi_virtmap_load();						\
-	__f = efi.systab->runtime->f;					\
-	__s = __f(__VA_ARGS__);						\
-	efi_virtmap_unload();						\
-	__s;								\
-})
+#define arch_efi_call_virt_setup()	efi_virtmap_load()
+#define arch_efi_call_virt_teardown()	efi_virtmap_unload()
 
-#define __efi_call_virt(f, ...)						\
+#define arch_efi_call_virt(f, args...)					\
 ({									\
 	efi_##f##_t *__f;						\
-									\
-	efi_virtmap_load();						\
 	__f = efi.systab->runtime->f;					\
-	__f(__VA_ARGS__);						\
-	efi_virtmap_unload();						\
+	__f(args);							\
 })
 
 static inline void efi_set_pgd(struct mm_struct *mm)
-- 
1.9.1

[PATCHv2 4/5] x86/efi: move to generic {__,}efi_call_virt

[Mark Rutland]

Now there's a common template for {__,}efi_call_virt, remove the
duplicate logic from the x86 efi code.

Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: "H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
Cc: Ingo Molnar <mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 arch/x86/include/asm/efi.h | 41 ++++++++++++-----------------------------
 1 file changed, 12 insertions(+), 29 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 53748c4..cf79c8f 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -33,28 +33,16 @@
 
 extern unsigned long asmlinkage efi_call_phys(void *, ...);
 
+#define arch_efi_call_virt_setup()	kernel_fpu_begin()
+#define arch_efi_call_virt_teardown()	kernel_fpu_end()
+
 /*
  * Wrap all the virtual calls in a way that forces the parameters on the stack.
  */
-
-/* Use this macro if your virtual returns a non-void value */
-#define efi_call_virt(f, args...) \
-({									\
-	efi_status_t __s;						\
-	kernel_fpu_begin();						\
-	__s = ((efi_##f##_t __attribute__((regparm(0)))*)		\
-		efi.systab->runtime->f)(args);				\
-	kernel_fpu_end();						\
-	__s;								\
-})
-
-/* Use this macro if your virtual call does not return any value */
-#define __efi_call_virt(f, args...) \
+#define arch_efi_call_virt(f, args...)					\
 ({									\
-	kernel_fpu_begin();						\
 	((efi_##f##_t __attribute__((regparm(0)))*)			\
 		efi.systab->runtime->f)(args);				\
-	kernel_fpu_end();						\
 })
 
 #define efi_ioremap(addr, size, type, attr)	ioremap_cache(addr, size)
@@ -78,10 +66,8 @@ struct efi_scratch {
 	u64	phys_stack;
 } __packed;
 
-#define efi_call_virt(f, ...)						\
+#define arch_efi_call_virt_setup()					\
 ({									\
-	efi_status_t __s;						\
-									\
 	efi_sync_low_kernel_mappings();					\
 	preempt_disable();						\
 	__kernel_fpu_begin();						\
@@ -91,9 +77,13 @@ struct efi_scratch {
 		write_cr3((unsigned long)efi_scratch.efi_pgt);		\
 		__flush_tlb_all();					\
 	}								\
-									\
-	__s = efi_call((void *)efi.systab->runtime->f, __VA_ARGS__);	\
-									\
+})
+
+#define arch_efi_call_virt(f, args...)					\
+	efi_call((void *)efi.systab->runtime->f, args);			\
+
+#define arch_efi_call_virt_teardown()					\
+({									\
 	if (efi_scratch.use_pgd) {					\
 		write_cr3(efi_scratch.prev_cr3);			\
 		__flush_tlb_all();					\
@@ -101,15 +91,8 @@ struct efi_scratch {
 									\
 	__kernel_fpu_end();						\
 	preempt_enable();						\
-	__s;								\
 })
 
-/*
- * All X86_64 virt calls return non-void values. Thus, use non-void call for
- * virt calls that would be void on X86_32.
- */
-#define __efi_call_virt(f, args...) efi_call_virt(f, args)
-
 extern void __iomem *__init efi_ioremap(unsigned long addr, unsigned long size,
 					u32 type, u64 attribute);
 
-- 
1.9.1

[PATCHv2 5/5] efi/runtime-wrappers: detect FW irq flag corruption

[Mark Rutland]

The UEFI spec allows runtime services to be called with interrupts
masked or unmasked, and if a runtime service function needs to mask
interrupts, it must restore the mask to its original state before
returning (i.e. from the PoV of the OS, this does not change across a
call). Firmware should never unmask exceptions, as these may then be
taken by the OS unexpectedly.

Unfortunately, some firmware has been seen to unmask IRQs (and
potentially other maskable exceptions) across runtime services calls,
leaving irq flags corrupted after returning from a runtime services
function call. This may be detected by the IRQ tracing code, but often
goes unnoticed, leaving a potentially disastrous bug hidden.

This patch detects when the irq flags are corrupted by an EFI runtime
services call, logging the call and specific corruption to the console.
While restoring the expected value of the flags is insufficient to avoid
problems, we do so to avoid redundant warnings from elsewhere (e.g. IRQ
tracing).

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: linux-efi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/firmware/efi/runtime-wrappers.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index 1b9fa54..aeb65f2 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -16,8 +16,10 @@
 
 #include <linux/bug.h>
 #include <linux/efi.h>
+#include <linux/irqflags.h>
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
+#include <linux/stringify.h>
 #include <asm/efi.h>
 
 
@@ -25,8 +27,11 @@
 #define efi_call_virt(f, args...)					\
 ({									\
 	efi_status_t __s;						\
+	unsigned long flags;						\
 	arch_efi_call_virt_setup();					\
+	local_save_flags(flags);					\
 	__s = arch_efi_call_virt(f, args);				\
+	efi_call_virt_check_flags(flags, __stringify(f));		\
 	arch_efi_call_virt_teardown();					\
 	__s;								\
 })
@@ -35,12 +40,29 @@
 #ifndef __efi_call_virt
 #define __efi_call_virt(f, args...)					\
 ({									\
+	unsigned long flags;						\
 	arch_efi_call_virt_setup();					\
+	local_irq_save(flags);						\
 	arch_efi_call_virt(f, args);					\
+	efi_call_virt_check_flags(flags, __stringify(f));		\
 	arch_efi_call_virt_teardown();					\
 })
 #endif
 
+static void efi_call_virt_check_flags(unsigned long flags, const char *call)
+{
+	unsigned long cur_flags;
+
+	local_save_flags(cur_flags);
+	if (!WARN_ON_ONCE(cur_flags != flags))
+		return;
+
+	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_NOW_UNRELIABLE);
+	pr_err_ratelimited(FW_BUG "IRQ flags corrupted (0x%08lx=>0x%08lx) by EFI %s\n",
+			   flags, cur_flags, call);
+	local_irq_restore(flags);
+}
+
 /*
  * According to section 7.1 of the UEFI spec, Runtime Services are not fully
  * reentrant, and there are particular combinations of calls that need to be
-- 
1.9.1

Re: [PATCHv2 1/5] efi/runtime-wrappers: add {__,}efi_call_virt templates

[Leif Lindholm]

On Thu, Apr 21, 2016 at 12:35:25PM +0100, Mark Rutland wrote:
> Currently each architecture must implement two macros, efi_call_virt and
> __efi_call_virt, which only differ by the presence or absence of a
> return type. Otherwise, the logic surrounding the call is identical.
> 
> As each architecture must define the entire body of each, we can't place
> any generic manipulation (e.g. irq flag validation) in the middle.
> 
> This patch adds template implementations of these macros. With these,
> arch code can implement three template macros, avoiding reptition for
> the void/non-void return cases:
> 
> * arch_efi_call_virt_setup
> 
>   Sets up the environment for the call (e.g. switching page tables,
>   allowing kernel-mode use of floating point, if required).
> 
> * arch_efi_call_virt
> 
>   Performs the call. The last expression in the macro must be the call
>   itself, allowing the logic to be shared by the void and non-void
>   cases.
> 
> * arch_efi_call_virt_teardown
> 
>   Restores the usual kernel environment once the call has returned.
> 
> While the savings from repition are minimal, we additionally gain the
> ability to add common code around the call with the call enviroment set
> up. This can be used to detect common firmware issues (e.g. bad irq mask
> management).
> 
> Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  drivers/firmware/efi/runtime-wrappers.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index de69530..1b9fa54 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -20,6 +20,27 @@
>  #include <linux/spinlock.h>
>  #include <asm/efi.h>
>  
> +
> +#ifndef efi_call_virt

So ... not a strong complaint, but I would prefer if these weren't
ifdefd. I presume this is because ia64?
Could that be given a dummy pass-through version instead? If not,
could a comment be added that this is to retain compatibility with
ia64 (so that if that architecture was to mysteriously disappear from
the tree, someone might remember to deconditionalise it)?

> +#define efi_call_virt(f, args...)					\
> +({									\
> +	efi_status_t __s;						\
> +	arch_efi_call_virt_setup();					\
> +	__s = arch_efi_call_virt(f, args);				\
> +	arch_efi_call_virt_teardown();					\
> +	__s;								\
> +})
> +#endif
> +
> +#ifndef __efi_call_virt
> +#define __efi_call_virt(f, args...)					\
> +({									\
> +	arch_efi_call_virt_setup();					\
> +	arch_efi_call_virt(f, args);					\
> +	arch_efi_call_virt_teardown();					\
> +})
> +#endif
> +
>  /*
>   * According to section 7.1 of the UEFI spec, Runtime Services are not fully
>   * reentrant, and there are particular combinations of calls that need to be
> -- 
> 1.9.1
> 

Re: [PATCHv2 0/5] efi: detect erroneous firmware IRQ manipulation

[Leif Lindholm]

On Thu, Apr 21, 2016 at 12:35:24PM +0100, Mark Rutland wrote:
> Some firmware erroneously unmask IRQs (and potentially other architecture
> specific exceptions) during runtime services functions, in violation of both
> common sense and the UEFI specification. This can result in a number of issues
> if said exceptions are taken when they are expected to be masked, and
> additionally can confuse IRQ tracing if the original mask state is not
> restored prior to returning from firmware.
> 
> In practice it's difficult to check that firmware never unmasks exceptions, but
> we can at least check that the IRQ flags are at least consistent upon entry to
> and return from a runtime services function call. This series implements said
> check in the shared EFI runtime wrappers code, after an initial round of
> refactoring such that this can be generic.
> 
> I have left ia64 as-is, without this check, as ia64 has many special cases for
> the runtime calls which don't fit well with the generic code, and I don't
> expect a new, buggy ia64 firmware to appear soon.
> 
> The first time corruption of the IRQ flags is detected, we dump a stack trace,
> and set TAINT_FIRMWARE_WORKAROUND. Additionally, and in all subsequent cases,
> we log (with ratelimiting) the specific corruption of the flags, and restore
> the expected flags to avoid redundant warnings elsewhere.
> 
> For example, if the firmware on an arm64 system erroneously unmasked IRQ+FIQ,
> we would see warnings in dmesg of the form:
> 
> [    5.639616] ------------[ cut here ]------------
> [    5.644233] WARNING: CPU: 3 PID: 1120 at drivers/firmware/efi/runtime-wrappers.c:57 efi_call_virt_check_flags+0x84/0x90
> [    5.655001] Modules linked in:
> [    5.658046] 
> [    5.659528] CPU: 3 PID: 1120 Comm: mount Not tainted 4.6.0-rc4+ #6
> [    5.674293] task: ffffffc3ecbcc800 ti: ffffffc0fa498000 task.ti: ffffffc0fa498000
> [    5.681763] PC is at efi_call_virt_check_flags+0x84/0x90
> [    5.687063] LR is at virt_efi_get_next_variable+0x74/0xa0
> [    5.692449] pc : [<ffffff8008618ccc>] lr : [<ffffff8008618f2c>] pstate: 20000105
> [    5.699831] sp : ffffffc0fa49bbd0
> [    5.703133] x29: ffffffc0fa49bbd0 x28: ffffffc0fa498000 
> [    5.708436] x27: ffffff8008776000 x26: 0000000000000001 
> [    5.713739] x25: ffffff8008bf2000 x24: 0000000000000000 
> [    5.719042] x23: ffffffc0fa49bcc0 x22: ffffffc3eb84d400 
> [    5.724344] x21: 00000000000001c0 x20: 0000000000000100 
> [    5.729646] x19: ffffff8008bf2a20 x18: 0000007fc1f31aa0 
> [    5.734948] x17: 0000000000425060 x16: ffffff80081e4578 
> [    5.740251] x15: ffffffffffffffff x14: 0000000000000000 
> [    5.745553] x13: 0000000000000000 x12: 0000000000000000 
> [    5.750855] x11: 0101010101010101 x10: 7f7f7f7f7f7f7f7f 
> [    5.756157] x9 : 0000000000000000 x8 : 0000000000000000 
> [    5.761459] x7 : 0000000000000000 x6 : 0000000000000001 
> [    5.766761] x5 : 00000000220e94ba x4 : 0000000022c61190 
> [    5.772063] x3 : 0000000000000001 x2 : ffffff8008bc0000 
> [    5.777366] x1 : ffffff80089d9d10 x0 : 00000000000001c0 
> [    5.782667] 
> [    5.784147] ---[ end trace b1612054aa2afdce ]---
> [    5.788751] Call trace:
> [    5.791186] Exception stack(0xffffffc0fa49ba10 to 0xffffffc0fa49bb30)
> [    5.797614] ba00:                                   ffffff8008bf2a20 0000000000000100
> [    5.805430] ba20: ffffffc0fa49bbd0 ffffff8008618ccc ffffffc0fa49ba60 00000000220e94be
> [    5.813247] ba40: ffffffc0fa49ba70 0000000022c5a208 ffffffc0fa49ba80 0000000000000010
> [    5.821063] ba60: 0000000022168ff8 ffffffc0fa49bcc0 ffffffc0fa49baa0 0000000022c4d440
> [    5.828880] ba80: ffffffc0fa49baa0 0000000000000010 0000000022168ff8 ffffffc0fa49bcc0
> [    5.836696] baa0: ffffffc0fa49bb10 0000000022c4db50 00000000000001c0 ffffff80089d9d10
> [    5.844512] bac0: ffffff8008bc0000 0000000000000001 0000000022c61190 00000000220e94ba
> [    5.852329] bae0: 0000000000000001 0000000000000000 0000000000000000 0000000000000000
> [    5.860145] bb00: 7f7f7f7f7f7f7f7f 0101010101010101 0000000000000000 0000000000000000
> [    5.867961] bb20: 0000000000000000 ffffffffffffffff
> [    5.872826] [<ffffff8008618ccc>] efi_call_virt_check_flags+0x84/0x90
> [    5.879168] [<ffffff8008618f2c>] virt_efi_get_next_variable+0x74/0xa0
> [    5.885596] [<ffffff80086182a4>] efivar_init+0x7c/0x2a0
> [    5.890810] [<ffffff80082f7c7c>] efivarfs_fill_super+0xac/0xe8
> [    5.896633] [<ffffff80081c5e34>] mount_single+0x8c/0xb8
> [    5.901846] [<ffffff80082f7bc8>] efivarfs_mount+0x18/0x20
> [    5.907232] [<ffffff80081c5f0c>] mount_fs+0x4c/0x168
> [    5.912184] [<ffffff80081e0f38>] vfs_kern_mount+0x50/0x118
> [    5.917658] [<ffffff80081e3890>] do_mount+0x208/0xc08
> [    5.922697] [<ffffff80081e4608>] SyS_mount+0x90/0xf8
> [    5.927649] [<ffffff8008085e70>] el0_svc_naked+0x24/0x28
> [    5.932960] Disabling lock debugging due to kernel taint
> [    5.938263] [Firmware Bug]: IRQ flags corrupted (0x000001c0=>0x00000100) by EFI get_next_variable
> 
> Then subsequently:
> 
> [    5.938263] [Firmware Bug]: IRQ flags corrupted (0x000001c0=>0x00000100) by EFI get_next_variable
> [    5.947143] [Firmware Bug]: IRQ flags corrupted (0x000001c0=>0x00000100) by EFI get_variable

So, I think this is a good thing, but the diffs end up being quite
hard to deciphre. Is there any non-insane shuffling around of things
that can make the changeset more clear?

/
    Leif

> Thanks,
> Mark.
> 
> Mark Rutland (5):
>   efi/runtime-wrappers: add {__,}efi_call_virt templates
>   arm64/efi: move to generic {__,}efi_call_virt
>   arm/efi: move to generic {__,}efi_call_virt
>   x86/efi: move to generic {__,}efi_call_virt
>   efi/runtime-wrappers: detect FW irq flag corruption
> 
>  arch/arm/include/asm/efi.h              | 20 +++------------
>  arch/arm64/include/asm/efi.h            | 21 ++++++----------
>  arch/x86/include/asm/efi.h              | 41 +++++++++----------------------
>  drivers/firmware/efi/runtime-wrappers.c | 43 +++++++++++++++++++++++++++++++++
>  4 files changed, 66 insertions(+), 59 deletions(-)
> 
> -- 
> 1.9.1
> 

Re: [PATCHv2 0/5] efi: detect erroneous firmware IRQ manipulation

[Mark Rutland]

On Thu, Apr 21, 2016 at 12:47:37PM +0100, Leif Lindholm wrote:
> On Thu, Apr 21, 2016 at 12:35:24PM +0100, Mark Rutland wrote:
> > Some firmware erroneously unmask IRQs (and potentially other architecture
> > specific exceptions) during runtime services functions, in violation of both
> > common sense and the UEFI specification. This can result in a number of issues
> > if said exceptions are taken when they are expected to be masked, and
> > additionally can confuse IRQ tracing if the original mask state is not
> > restored prior to returning from firmware.
> > 
> > In practice it's difficult to check that firmware never unmasks exceptions, but
> > we can at least check that the IRQ flags are at least consistent upon entry to
> > and return from a runtime services function call. This series implements said
> > check in the shared EFI runtime wrappers code, after an initial round of
> > refactoring such that this can be generic.

[...]

> So, I think this is a good thing, but the diffs end up being quite
> hard to deciphre. Is there any non-insane shuffling around of things
> that can make the changeset more clear?

This is the clearest/simplest way I had found to organise them.

I think the arm and arm64 diffs are fairly clear, so I assume you're
mainly asking w.r.t. the x86 patch, which is less so due to the volume
of lines that fall out of the diff context. The 32-bit side of that is
simple, so I could split that into two patches, leaving the diff pain
only with the 64-bit patch.

Otherwise, short of moving things into a different file I think this is
a losing battle against git's diff engine.

Thanks,
Mark.

Re: [PATCHv2 1/5] efi/runtime-wrappers: add {__,}efi_call_virt templates

[Mark Rutland]

On Thu, Apr 21, 2016 at 12:42:56PM +0100, Leif Lindholm wrote:
> On Thu, Apr 21, 2016 at 12:35:25PM +0100, Mark Rutland wrote:
> > Currently each architecture must implement two macros, efi_call_virt and
> > __efi_call_virt, which only differ by the presence or absence of a
> > return type. Otherwise, the logic surrounding the call is identical.
> > 
> > As each architecture must define the entire body of each, we can't place
> > any generic manipulation (e.g. irq flag validation) in the middle.
> > 
> > This patch adds template implementations of these macros. With these,
> > arch code can implement three template macros, avoiding reptition for
> > the void/non-void return cases:
> > 
> > * arch_efi_call_virt_setup
> > 
> >   Sets up the environment for the call (e.g. switching page tables,
> >   allowing kernel-mode use of floating point, if required).
> > 
> > * arch_efi_call_virt
> > 
> >   Performs the call. The last expression in the macro must be the call
> >   itself, allowing the logic to be shared by the void and non-void
> >   cases.
> > 
> > * arch_efi_call_virt_teardown
> > 
> >   Restores the usual kernel environment once the call has returned.
> > 
> > While the savings from repition are minimal, we additionally gain the
> > ability to add common code around the call with the call enviroment set
> > up. This can be used to detect common firmware issues (e.g. bad irq mask
> > management).
> > 
> > Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> > Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > ---
> >  drivers/firmware/efi/runtime-wrappers.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> > index de69530..1b9fa54 100644
> > --- a/drivers/firmware/efi/runtime-wrappers.c
> > +++ b/drivers/firmware/efi/runtime-wrappers.c
> > @@ -20,6 +20,27 @@
> >  #include <linux/spinlock.h>
> >  #include <asm/efi.h>
> >  
> > +
> > +#ifndef efi_call_virt
> 
> So ... not a strong complaint, but I would prefer if these weren't
> ifdefd. I presume this is because ia64?

Yup, and to allow the gradual migration of arm/arm64/x86 without a new
CONFIG_WANT_GENERIC_EFI_CALL_VIRT or something to that effect.

> Could that be given a dummy pass-through version instead?

I'm not exactly sure what you mean by that. Could you elaborate?

> If not, could a comment be added that this is to retain compatibility
> with ia64 (so that if that architecture was to mysteriously disappear
> from the tree, someone might remember to deconditionalise it)?

Sure. I can also add a note to the commit message regarding the
temporary need while arm/arm64/x86 are moved over.

Thanks,
Mark.

> > +#define efi_call_virt(f, args...)					\
> > +({									\
> > +	efi_status_t __s;						\
> > +	arch_efi_call_virt_setup();					\
> > +	__s = arch_efi_call_virt(f, args);				\
> > +	arch_efi_call_virt_teardown();					\
> > +	__s;								\
> > +})
> > +#endif
> > +
> > +#ifndef __efi_call_virt
> > +#define __efi_call_virt(f, args...)					\
> > +({									\
> > +	arch_efi_call_virt_setup();					\
> > +	arch_efi_call_virt(f, args);					\
> > +	arch_efi_call_virt_teardown();					\
> > +})
> > +#endif
> > +
> >  /*
> >   * According to section 7.1 of the UEFI spec, Runtime Services are not fully
> >   * reentrant, and there are particular combinations of calls that need to be
> > -- 
> > 1.9.1
> > 
> 

Re: [PATCHv2 1/5] efi/runtime-wrappers: add {__,}efi_call_virt templates

[Mark Rutland]

On Thu, Apr 21, 2016 at 01:55:07PM +0100, Mark Rutland wrote:
> On Thu, Apr 21, 2016 at 12:42:56PM +0100, Leif Lindholm wrote:
> > On Thu, Apr 21, 2016 at 12:35:25PM +0100, Mark Rutland wrote:
> > > Currently each architecture must implement two macros, efi_call_virt and
> > > __efi_call_virt, which only differ by the presence or absence of a
> > > return type. Otherwise, the logic surrounding the call is identical.
> > > 
> > > As each architecture must define the entire body of each, we can't place
> > > any generic manipulation (e.g. irq flag validation) in the middle.
> > > 
> > > This patch adds template implementations of these macros. With these,
> > > arch code can implement three template macros, avoiding reptition for
> > > the void/non-void return cases:
> > > 
> > > * arch_efi_call_virt_setup
> > > 
> > >   Sets up the environment for the call (e.g. switching page tables,
> > >   allowing kernel-mode use of floating point, if required).
> > > 
> > > * arch_efi_call_virt
> > > 
> > >   Performs the call. The last expression in the macro must be the call
> > >   itself, allowing the logic to be shared by the void and non-void
> > >   cases.
> > > 
> > > * arch_efi_call_virt_teardown
> > > 
> > >   Restores the usual kernel environment once the call has returned.
> > > 
> > > While the savings from repition are minimal, we additionally gain the
> > > ability to add common code around the call with the call enviroment set
> > > up. This can be used to detect common firmware issues (e.g. bad irq mask
> > > management).
> > > 
> > > Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > > Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> > > Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > ---
> > >  drivers/firmware/efi/runtime-wrappers.c | 21 +++++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > > 
> > > diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> > > index de69530..1b9fa54 100644
> > > --- a/drivers/firmware/efi/runtime-wrappers.c
> > > +++ b/drivers/firmware/efi/runtime-wrappers.c
> > > @@ -20,6 +20,27 @@
> > >  #include <linux/spinlock.h>
> > >  #include <asm/efi.h>
> > >  
> > > +
> > > +#ifndef efi_call_virt
> > 
> > So ... not a strong complaint, but I would prefer if these weren't
> > ifdefd. I presume this is because ia64?
> 
> Yup, and to allow the gradual migration of arm/arm64/x86 without a new
> CONFIG_WANT_GENERIC_EFI_CALL_VIRT or something to that effect.

Actually, ia64 never uses this code, so I can add a final patch to
remove the ifdefs after the existing arch code is moved over.

I'll do that...

Mark.

Re: [PATCHv2 2/5] arm64/efi: move to generic {__,}efi_call_virt

[Will Deacon]

On Thu, Apr 21, 2016 at 12:35:26PM +0100, Mark Rutland wrote:
> Now there's a common template for {__,}efi_call_virt, remove the
> duplicate logic from the arm64 efi code.
> 
> Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
> Cc: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  arch/arm64/include/asm/efi.h | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 8e88a69..f4f71224 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -14,28 +14,21 @@ extern void efi_init(void);
>  
>  int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
>  
> -#define efi_call_virt(f, ...)						\
> +#define arch_efi_call_virt_setup()					\
>  ({									\
> -	efi_##f##_t *__f;						\
> -	efi_status_t __s;						\
> -									\
>  	kernel_neon_begin();						\
>  	efi_virtmap_load();						\
> -	__f = efi.systab->runtime->f;					\
> -	__s = __f(__VA_ARGS__);						\
> -	efi_virtmap_unload();						\
> -	kernel_neon_end();						\
> -	__s;								\
>  })
>  
> -#define __efi_call_virt(f, ...)						\
> +#define arch_efi_call_virt(f, args...)					\
>  ({									\
>  	efi_##f##_t *__f;						\
> -									\
> -	kernel_neon_begin();						\
> -	efi_virtmap_load();						\
>  	__f = efi.systab->runtime->f;					\
> -	__f(__VA_ARGS__);						\
> +	__f(args);							\

Any reason to change this to a named argument? This patch is hard enough
to review as it is, given the way the diff has been generated!

Either way:

Acked-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>

Will

Re: [PATCHv2 2/5] arm64/efi: move to generic {__,}efi_call_virt

[Mark Rutland]

On Thu, Apr 21, 2016 at 05:48:40PM +0100, Will Deacon wrote:
> On Thu, Apr 21, 2016 at 12:35:26PM +0100, Mark Rutland wrote:
> > Now there's a common template for {__,}efi_call_virt, remove the
> > duplicate logic from the arm64 efi code.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > Cc: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
> > Cc: Leif Lindholm <leif.lindholm-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> > Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> > Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > ---
> >  arch/arm64/include/asm/efi.h | 21 +++++++--------------
> >  1 file changed, 7 insertions(+), 14 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> > index 8e88a69..f4f71224 100644
> > --- a/arch/arm64/include/asm/efi.h
> > +++ b/arch/arm64/include/asm/efi.h
> > @@ -14,28 +14,21 @@ extern void efi_init(void);
> >  
> >  int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
> >  
> > -#define efi_call_virt(f, ...)						\
> > +#define arch_efi_call_virt_setup()					\
> >  ({									\
> > -	efi_##f##_t *__f;						\
> > -	efi_status_t __s;						\
> > -									\
> >  	kernel_neon_begin();						\
> >  	efi_virtmap_load();						\
> > -	__f = efi.systab->runtime->f;					\
> > -	__s = __f(__VA_ARGS__);						\
> > -	efi_virtmap_unload();						\
> > -	kernel_neon_end();						\
> > -	__s;								\
> >  })
> >  
> > -#define __efi_call_virt(f, ...)						\
> > +#define arch_efi_call_virt(f, args...)					\
> >  ({									\
> >  	efi_##f##_t *__f;						\
> > -									\
> > -	kernel_neon_begin();						\
> > -	efi_virtmap_load();						\
> >  	__f = efi.systab->runtime->f;					\
> > -	__f(__VA_ARGS__);						\
> > +	__f(args);							\
> 
> Any reason to change this to a named argument? This patch is hard enough
> to review as it is, given the way the diff has been generated!

That was to enforce consistency across arm/arm64/x86 for this.

It seemed nicer to make them all use a named argument than it did to
make them all use __VA_ARGS__, either of which was nice than each of
them doing something slightly different.

Sorry for the pain that evidently caused!

> Either way:
> 
> Acked-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>

Cheers!

Mark.

Re: [PATCHv2 5/5] efi/runtime-wrappers: detect FW irq flag corruption

[Ard Biesheuvel]

On 21 April 2016 at 13:35, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> The UEFI spec allows runtime services to be called with interrupts
> masked or unmasked, and if a runtime service function needs to mask
> interrupts, it must restore the mask to its original state before
> returning (i.e. from the PoV of the OS, this does not change across a
> call). Firmware should never unmask exceptions, as these may then be
> taken by the OS unexpectedly.
>
> Unfortunately, some firmware has been seen to unmask IRQs (and
> potentially other maskable exceptions) across runtime services calls,
> leaving irq flags corrupted after returning from a runtime services
> function call. This may be detected by the IRQ tracing code, but often
> goes unnoticed, leaving a potentially disastrous bug hidden.
>
> This patch detects when the irq flags are corrupted by an EFI runtime
> services call, logging the call and specific corruption to the console.
> While restoring the expected value of the flags is insufficient to avoid
> problems, we do so to avoid redundant warnings from elsewhere (e.g. IRQ
> tracing).
>
> Signed-off-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  drivers/firmware/efi/runtime-wrappers.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index 1b9fa54..aeb65f2 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -16,8 +16,10 @@
>
>  #include <linux/bug.h>
>  #include <linux/efi.h>
> +#include <linux/irqflags.h>
>  #include <linux/mutex.h>
>  #include <linux/spinlock.h>
> +#include <linux/stringify.h>
>  #include <asm/efi.h>
>
>
> @@ -25,8 +27,11 @@
>  #define efi_call_virt(f, args...)                                      \
>  ({                                                                     \
>         efi_status_t __s;                                               \
> +       unsigned long flags;                                            \
>         arch_efi_call_virt_setup();                                     \
> +       local_save_flags(flags);                                        \
>         __s = arch_efi_call_virt(f, args);                              \
> +       efi_call_virt_check_flags(flags, __stringify(f));               \
>         arch_efi_call_virt_teardown();                                  \
>         __s;                                                            \
>  })
> @@ -35,12 +40,29 @@
>  #ifndef __efi_call_virt
>  #define __efi_call_virt(f, args...)                                    \
>  ({                                                                     \
> +       unsigned long flags;                                            \
>         arch_efi_call_virt_setup();                                     \
> +       local_irq_save(flags);                                          \

We shouldn't disable interrupts here. I assume this is a typo, and you
intended to use local_save_flags() as above?

>         arch_efi_call_virt(f, args);                                    \
> +       efi_call_virt_check_flags(flags, __stringify(f));               \
>         arch_efi_call_virt_teardown();                                  \
>  })
>  #endif
>
> +static void efi_call_virt_check_flags(unsigned long flags, const char *call)
> +{
> +       unsigned long cur_flags;
> +
> +       local_save_flags(cur_flags);
> +       if (!WARN_ON_ONCE(cur_flags != flags))
> +               return;
> +
> +       add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_NOW_UNRELIABLE);
> +       pr_err_ratelimited(FW_BUG "IRQ flags corrupted (0x%08lx=>0x%08lx) by EFI %s\n",
> +                          flags, cur_flags, call);
> +       local_irq_restore(flags);
> +}
> +
>  /*
>   * According to section 7.1 of the UEFI spec, Runtime Services are not fully
>   * reentrant, and there are particular combinations of calls that need to be

Other than that, this series looks fine to me.

With the above fixed:

For the series (except the x86 patch)

Reviewed-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Re: [PATCHv2 5/5] efi/runtime-wrappers: detect FW irq flag corruption

[Mark Rutland]

On Thu, Apr 21, 2016 at 07:05:32PM +0200, Ard Biesheuvel wrote:
> On 21 April 2016 at 13:35, Mark Rutland <mark.rutland@arm.com> wrote:
> > @@ -25,8 +27,11 @@
> >  #define efi_call_virt(f, args...)                                      \
> >  ({                                                                     \
> >         efi_status_t __s;                                               \
> > +       unsigned long flags;                                            \
> >         arch_efi_call_virt_setup();                                     \
> > +       local_save_flags(flags);                                        \
> >         __s = arch_efi_call_virt(f, args);                              \
> > +       efi_call_virt_check_flags(flags, __stringify(f));               \
> >         arch_efi_call_virt_teardown();                                  \
> >         __s;                                                            \
> >  })
> > @@ -35,12 +40,29 @@
> >  #ifndef __efi_call_virt
> >  #define __efi_call_virt(f, args...)                                    \
> >  ({                                                                     \
> > +       unsigned long flags;                                            \
> >         arch_efi_call_virt_setup();                                     \
> > +       local_irq_save(flags);                                          \
> 
> We shouldn't disable interrupts here. I assume this is a typo, and you
> intended to use local_save_flags() as above?

Oh, yes. That's an impressive mistake on my behalf; thanks for spotting
that!

I've been seeing issues with GetVariable and GetNextVariable, which
happen to only exercise the correct macro above.

> Other than that, this series looks fine to me.
> 
> With the above fixed:
> 
> For the series (except the x86 patch)
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Cheers!

Mark.