Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1386464 > unrolled thread

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

Started byMark Rutland <mark.rutland@arm.com>
First post2016-04-25 15:50 +0200
Last post2016-04-25 18:10 +0200
Articles 11 — 4 participants

Back to article view | Back to linux.kernel


Contents

  [PATCHv3 0/5] efi: detect erroneous firmware IRQ manipulation Mark Rutland <mark.rutland@arm.com> - 2016-04-25 15:50 +0200
    [PATCHv3 3/5] arm/efi: enable runtime call flag checking Mark Rutland <mark.rutland@arm.com> - 2016-04-25 15:50 +0200
    [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption Mark Rutland <mark.rutland@arm.com> - 2016-04-25 15:50 +0200
      Re: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption Robin Murphy <robin.murphy@arm.com> - 2016-04-25 16:20 +0200
        Re: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption Matt Fleming <matt@codeblueprint.co.uk> - 2016-04-25 16:20 +0200
          Re: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-04-25 16:20 +0200
            Re: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption Matt Fleming <matt@codeblueprint.co.uk> - 2016-04-25 16:30 +0200
              Re: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption Mark Rutland <mark.rutland@arm.com> - 2016-04-25 16:30 +0200
                Re: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption Matt Fleming <matt@codeblueprint.co.uk> - 2016-04-25 18:00 +0200
                  Re: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption Mark Rutland <mark.rutland@arm.com> - 2016-04-25 18:10 +0200
    Re: [PATCHv3 0/5] efi: detect erroneous firmware IRQ manipulation Matt Fleming <matt@codeblueprint.co.uk> - 2016-04-25 18:10 +0200

#1386464 — [PATCHv3 0/5] efi: detect erroneous firmware IRQ manipulation

FromMark Rutland <mark.rutland@arm.com>
Date2016-04-25 15:50 +0200
Subject[PATCHv3 0/5] efi: detect erroneous firmware IRQ manipulation
Message-ID<rrP3I-6Kr-3@gated-at.bofh.it>
Note: this is largely a rework of the final patch from v2 [2], which now has a
per-arch component (and hence additional patches). The rest of v2 has already
been picked up, and hence dropped from this posting.

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 (patches 1-5 of [2]).

I have left ia64 as-is, without this check, as ia64 doesn't currently use the
generic runtime wrappers, 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.

Since v1 [1]:
* Fix thinko: s/local_irq_save/local_save_flags/
* Remove ifdefs after conversion
* Remove reundant semicolon from x86 patch
* Move efi_call_virt_check_flags before first use
* Add Acked-bys and Reviewed-bys

Since v2 [2]:
* Drop the refactoring patches (1-5), which Matt has queued
* Add per-arch ARCH_EFI_IRQ_FLAGS_MASK, as discussed for v2 [3,4]

As with v2, this has been build-tested for each target, but other than arm64 I
don't have a good platform for testing. Hopefully this causes fewer explosions
than v2.

Thanks,
Mark.

[1] https://lkml.org/lkml/2016/4/21/260
[2] https://lkml.org/lkml/2016/4/22/542
[3] https://lkml.org/lkml/2016/4/25/230
[4] https://lkml.org/lkml/2016/4/25/243

Mark Rutland (5):
  efi/runtime-wrappers: detect FW irq flag corruption
  arm64/efi: enable runtime call flag checking
  arm/efi: enable runtime call flag checking
  x86/efi: enable runtime call flag checking
  efi/runtime-wrappers: remove ARCH_EFI_IRQ_FLAGS_MASK ifdef

 arch/arm/include/asm/efi.h              |  5 +++++
 arch/arm64/include/asm/efi.h            |  3 +++
 arch/x86/include/asm/efi.h              |  4 +++-
 drivers/firmware/efi/runtime-wrappers.c | 25 +++++++++++++++++++++++++
 4 files changed, 36 insertions(+), 1 deletion(-)

-- 
1.9.1

[toc] | [next] | [standalone]


#1386465 — [PATCHv3 3/5] arm/efi: enable runtime call flag checking

FromMark Rutland <mark.rutland@arm.com>
Date2016-04-25 15:50 +0200
Subject[PATCHv3 3/5] arm/efi: enable runtime call flag checking
Message-ID<rrP3K-6Kr-45@gated-at.bofh.it>
In reply to#1386464
Define ARCH_EFI_IRQ_FLAGS_MASK for arm, which will enable the generic
runtime wrapper code to detect when firmware erroneously modifies flags
over a runtime services function call.

We check all allocated flags, barring those which firmware has
legitimate reason to modify (condition flags and IT state). While in
practice corruption of some flags (e.g. J) would already be fatal, we
include these for consistency and documentation purposes.

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
---
 arch/arm/include/asm/efi.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index e1461fc..b05ff68 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -17,6 +17,7 @@
 #include <asm/mach/map.h>
 #include <asm/mmu_context.h>
 #include <asm/pgtable.h>
+#include <asm/ptrace.h>
 
 #ifdef CONFIG_EFI
 void efi_init(void);
@@ -33,6 +34,10 @@ int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
 	__f(args);							\
 })
 
+#define ARCH_EFI_IRQ_FLAGS_MASK \
+	(PSR_J_BIT | PSR_E_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT | \
+	 PSR_T_BIT | MODE_MASK)
+
 static inline void efi_set_pgd(struct mm_struct *mm)
 {
 	check_and_switch_context(mm, NULL);
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]


#1386466 — [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption

FromMark Rutland <mark.rutland@arm.com>
Date2016-04-25 15:50 +0200
Subject[PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption
Message-ID<rrP3K-6Kr-47@gated-at.bofh.it>
In reply to#1386464
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).

The set of bits in flags which we want to check is architecture-specific
(e.g. we want to check FIQ on arm64, but not the zero flag on x86), so
each arch must provide ARCH_EFI_IRQ_FLAGS_MASK to describe those. In the
absence of this mask, the check is a no-op, and we redundantly save the
flags twice, but that will be short-lived as subsequent patches
will implement this and remove the scaffolding.

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

diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index a2c8e70..1f0277e 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -16,23 +16,55 @@
 
 #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>
 
+/*
+ * Temporary scaffolding until all users provide ARCH_EFI_IRQ_FLAGS_MASK.
+ */
+#ifdef ARCH_EFI_IRQ_FLAGS_MASK
+static void efi_call_virt_check_flags(unsigned long flags, const char *call)
+{
+	unsigned long cur_flags;
+	bool mismatch;
+
+	local_save_flags(cur_flags);
+
+	mismatch = !!((cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK);
+	if (!WARN_ON_ONCE(mismatch))
+		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);
+}
+#else /* ARCH_EFI_IRQ_FLAGS_MASK */
+static inline void efi_call_virt_check_flags(unsigned long flags, const char *call) {}
+#endif /* ARCH_EFI_IRQ_FLAGS_MASK */
+
 #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;								\
 })
 
 #define __efi_call_virt(f, args...)					\
 ({									\
+	unsigned long flags;						\
 	arch_efi_call_virt_setup();					\
+	local_save_flags(flags);					\
 	arch_efi_call_virt(f, args);					\
+	efi_call_virt_check_flags(flags, __stringify(f));		\
 	arch_efi_call_virt_teardown();					\
 })
 
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]


#1386492 — Re: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption

FromRobin Murphy <robin.murphy@arm.com>
Date2016-04-25 16:20 +0200
SubjectRe: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption
Message-ID<rrPwJ-7gP-1@gated-at.bofh.it>
In reply to#1386466
Hi Mark,

On 25/04/16 14:46, Mark Rutland 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).
>
> The set of bits in flags which we want to check is architecture-specific
> (e.g. we want to check FIQ on arm64, but not the zero flag on x86), so
> each arch must provide ARCH_EFI_IRQ_FLAGS_MASK to describe those. In the
> absence of this mask, the check is a no-op, and we redundantly save the
> flags twice, but that will be short-lived as subsequent patches
> will implement this and remove the scaffolding.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Cc: linux-efi@vger.kernel.org
> ---
>   drivers/firmware/efi/runtime-wrappers.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
>
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index a2c8e70..1f0277e 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -16,23 +16,55 @@
>
>   #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>
>
> +/*
> + * Temporary scaffolding until all users provide ARCH_EFI_IRQ_FLAGS_MASK.
> + */
> +#ifdef ARCH_EFI_IRQ_FLAGS_MASK
> +static void efi_call_virt_check_flags(unsigned long flags, const char *call)
> +{
> +	unsigned long cur_flags;
> +	bool mismatch;
> +
> +	local_save_flags(cur_flags);
> +
> +	mismatch = !!((cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK);

nit: the assignment itself is already a conversion to bool, so the 
excitement is redundant here.

Robin.

> +	if (!WARN_ON_ONCE(mismatch))
> +		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);
> +}
> +#else /* ARCH_EFI_IRQ_FLAGS_MASK */
> +static inline void efi_call_virt_check_flags(unsigned long flags, const char *call) {}
> +#endif /* ARCH_EFI_IRQ_FLAGS_MASK */
> +
>   #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;								\
>   })
>
>   #define __efi_call_virt(f, args...)					\
>   ({									\
> +	unsigned long flags;						\
>   	arch_efi_call_virt_setup();					\
> +	local_save_flags(flags);					\
>   	arch_efi_call_virt(f, args);					\
> +	efi_call_virt_check_flags(flags, __stringify(f));		\
>   	arch_efi_call_virt_teardown();					\
>   })
>
>

[toc] | [prev] | [next] | [standalone]


#1386496 — Re: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption

FromMatt Fleming <matt@codeblueprint.co.uk>
Date2016-04-25 16:20 +0200
SubjectRe: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption
Message-ID<rrPwK-7gP-17@gated-at.bofh.it>
In reply to#1386492
On Mon, 25 Apr, at 03:12:01PM, Robin Murphy wrote:
> >+static void efi_call_virt_check_flags(unsigned long flags, const char *call)
> >+{
> >+	unsigned long cur_flags;
> >+	bool mismatch;
> >+
> >+	local_save_flags(cur_flags);
> >+
> >+	mismatch = !!((cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK);
> 
> nit: the assignment itself is already a conversion to bool, so the
> excitement is redundant here.

This was intentional. I asked Mark to make this change so that it's
explicit for the developer that we're performing the type conversion. 

[toc] | [prev] | [next] | [standalone]


#1386503 — Re: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption

FromArd Biesheuvel <ard.biesheuvel@linaro.org>
Date2016-04-25 16:20 +0200
SubjectRe: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption
Message-ID<rrPwK-7gP-31@gated-at.bofh.it>
In reply to#1386496
On 25 April 2016 at 16:15, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Mon, 25 Apr, at 03:12:01PM, Robin Murphy wrote:
>> >+static void efi_call_virt_check_flags(unsigned long flags, const char *call)
>> >+{
>> >+    unsigned long cur_flags;
>> >+    bool mismatch;
>> >+
>> >+    local_save_flags(cur_flags);
>> >+
>> >+    mismatch = !!((cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK);
>>
>> nit: the assignment itself is already a conversion to bool, so the
>> excitement is redundant here.
>
> This was intentional. I asked Mark to make this change so that it's
> explicit for the developer that we're performing the type conversion.

But replacing an implicit boolean cast with an explicit one makes
little sense, no? Don't we simply want '!= 0' here if you need a
boolean expression?

[toc] | [prev] | [next] | [standalone]


#1386512 — Re: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption

FromMatt Fleming <matt@codeblueprint.co.uk>
Date2016-04-25 16:30 +0200
SubjectRe: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption
Message-ID<rrPGq-7oh-33@gated-at.bofh.it>
In reply to#1386503
On Mon, 25 Apr, at 04:18:41PM, Ard Biesheuvel wrote:
> On 25 April 2016 at 16:15, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> > On Mon, 25 Apr, at 03:12:01PM, Robin Murphy wrote:
> >> >+static void efi_call_virt_check_flags(unsigned long flags, const char *call)
> >> >+{
> >> >+    unsigned long cur_flags;
> >> >+    bool mismatch;
> >> >+
> >> >+    local_save_flags(cur_flags);
> >> >+
> >> >+    mismatch = !!((cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK);
> >>
> >> nit: the assignment itself is already a conversion to bool, so the
> >> excitement is redundant here.
> >
> > This was intentional. I asked Mark to make this change so that it's
> > explicit for the developer that we're performing the type conversion.
> 
> But replacing an implicit boolean cast with an explicit one makes
> little sense, no? Don't we simply want '!= 0' here if you need a
> boolean expression?

Aha but '!!' is fewer characters to type!!

I'm not that bothered as long as we don't stuff an int into a bool
without giving the programmer some idea we're doing that. It's not
about the compiler getting it wrong, more about a developer
introducing a bug when they change the code in the future. 

Unless anyone objects, I'll fix this up to use '!= 0' when I apply it.

[toc] | [prev] | [next] | [standalone]


#1386514 — Re: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption

FromMark Rutland <mark.rutland@arm.com>
Date2016-04-25 16:30 +0200
SubjectRe: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption
Message-ID<rrPGq-7oh-37@gated-at.bofh.it>
In reply to#1386512
On Mon, Apr 25, 2016 at 03:24:35PM +0100, Matt Fleming wrote:
> On Mon, 25 Apr, at 04:18:41PM, Ard Biesheuvel wrote:
> > On 25 April 2016 at 16:15, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> > > On Mon, 25 Apr, at 03:12:01PM, Robin Murphy wrote:
> > >> >+static void efi_call_virt_check_flags(unsigned long flags, const char *call)
> > >> >+{
> > >> >+    unsigned long cur_flags;
> > >> >+    bool mismatch;
> > >> >+
> > >> >+    local_save_flags(cur_flags);
> > >> >+
> > >> >+    mismatch = !!((cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK);
> > >>
> > >> nit: the assignment itself is already a conversion to bool, so the
> > >> excitement is redundant here.
> > >
> > > This was intentional. I asked Mark to make this change so that it's
> > > explicit for the developer that we're performing the type conversion.
> > 
> > But replacing an implicit boolean cast with an explicit one makes
> > little sense, no? Don't we simply want '!= 0' here if you need a
> > boolean expression?
> 
> Aha but '!!' is fewer characters to type!!
> 
> I'm not that bothered as long as we don't stuff an int into a bool
> without giving the programmer some idea we're doing that. It's not
> about the compiler getting it wrong, more about a developer
> introducing a bug when they change the code in the future. 
> 
> Unless anyone objects, I'll fix this up to use '!= 0' when I apply it.

I have no strong preference so long as the code is correct.

Another option is to get rid of the bool entirely:

	flags ^= cur_flags;
	if (!WARN_ON(flags & ARCH_EFI_IRQ_FLAGS_MASK))
		return;

Mark.

[toc] | [prev] | [next] | [standalone]


#1386615 — Re: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption

FromMatt Fleming <matt@codeblueprint.co.uk>
Date2016-04-25 18:00 +0200
SubjectRe: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption
Message-ID<rrR5x-8ma-23@gated-at.bofh.it>
In reply to#1386514
On Mon, 25 Apr, at 03:27:35PM, Mark Rutland wrote:
> 
> I have no strong preference so long as the code is correct.
> 
> Another option is to get rid of the bool entirely:
> 
> 	flags ^= cur_flags;
> 	if (!WARN_ON(flags & ARCH_EFI_IRQ_FLAGS_MASK))
> 		return;

OK, let's do the following because we need flags to be preserved for
printing,

---

	unsigned long cur_flags, mismatch;

	local_save_flags(cur_flags);

	mismatch = flags ^ cur_flags;
	if (!WARN_ON_ONCE(mismatch & ARCH_EFI_IRQ_FLAGS_MASK))
		return;

[toc] | [prev] | [next] | [standalone]


#1386622 — Re: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption

FromMark Rutland <mark.rutland@arm.com>
Date2016-04-25 18:10 +0200
SubjectRe: [PATCHv3 1/5] efi/runtime-wrappers: detect FW irq flag corruption
Message-ID<rrRfd-hZ-29@gated-at.bofh.it>
In reply to#1386615
On Mon, Apr 25, 2016 at 04:59:22PM +0100, Matt Fleming wrote:
> On Mon, 25 Apr, at 03:27:35PM, Mark Rutland wrote:
> > 
> > I have no strong preference so long as the code is correct.
> > 
> > Another option is to get rid of the bool entirely:
> > 
> > 	flags ^= cur_flags;
> > 	if (!WARN_ON(flags & ARCH_EFI_IRQ_FLAGS_MASK))
> > 		return;
> 
> OK, let's do the following because we need flags to be preserved for
> printing,
> 
> ---
> 
> 	unsigned long cur_flags, mismatch;
> 
> 	local_save_flags(cur_flags);
> 
> 	mismatch = flags ^ cur_flags;
> 	if (!WARN_ON_ONCE(mismatch & ARCH_EFI_IRQ_FLAGS_MASK))
> 		return;
> 

Sure; that looks good to me.

Cheers,
Mark.

[toc] | [prev] | [next] | [standalone]


#1386621

FromMatt Fleming <matt@codeblueprint.co.uk>
Date2016-04-25 18:10 +0200
Message-ID<rrRfd-hZ-25@gated-at.bofh.it>
In reply to#1386464
On Mon, 25 Apr, at 02:46:29PM, Mark Rutland wrote:
> Note: this is largely a rework of the final patch from v2 [2], which now has a
> per-arch component (and hence additional patches). The rest of v2 has already
> been picked up, and hence dropped from this posting.
> 
> 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 (patches 1-5 of [2]).
> 
> I have left ia64 as-is, without this check, as ia64 doesn't currently use the
> generic runtime wrappers, 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.

Thanks Mark. I've picked up the series and applied it to the v4.7
queue.

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web