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

[Matt Fleming]

On Mon, 25 Apr, at 12:04:55PM, Mark Rutland wrote:
> On Mon, Apr 25, 2016 at 11:51:53AM +0100, Matt Fleming wrote:
> > On Mon, 25 Apr, at 11:40:09AM, Mark Rutland wrote:
> > > 
> > > It looks like irqs_disabled_flags() will do what you expect, and ignore
> > > everything but the interrupt flag.
> > > 
> > > However, for ARM that will ignore the other exceptions we've seen FW
> > > erroneously unmask (e.g. FIQ), which is unfortunate, as fiddling with
> > > those is just as disastrous.
> >  
> > Bah, right.
> > 
> > > Would you be happy with an arch_efi_call_check_flags(before, after),
> > > instead? That way we can make the flags we check arch-specific.
> > 
> > Could we just make the flag mask arch-specific instead of the call
> > since the rest of efi_call_virt_check_flags() is good?
> 
> Yup, I meant that arch_efi_call_check_flags would only do the flag
> comparison, only replacing the bit currently in the WARN_ON_ONCE().
> 
> > Something like this (uncompiled, untested, half-baked),
> > 
> > ---
> > 
> > diff --git a/drivers/firmware/efi/runtime-wrappers.c 
> > b/drivers/firmware/efi/runtime-wrappers.c
> > index c38b1cfc26e2..057d00bee7d6 100644
> > --- a/drivers/firmware/efi/runtime-wrappers.c
> > +++ b/drivers/firmware/efi/runtime-wrappers.c
> > @@ -25,9 +25,12 @@
> >  static void efi_call_virt_check_flags(unsigned long flags, const char 
> > *call)
> >  {
> > unsigned long cur_flags;
> > +   bool mismatch;
> >  
> > local_save_flags(cur_flags);
> > -   if (!WARN_ON_ONCE(cur_flags != flags))
> > +
> > +   mismatch = (cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK;
> > +   if (!WARN_ON_ONCE(mismatch))
> > return;
> 
> This style also works for me.
 
Cool. One thing that occurred to me after I sent it is that
technically we should either,

  1) make 'mismatch' an int or
  2) do mismatch = !!((cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK)

Either is fine with me, I just don't want to leave the implicit
conversion to C's type system.

> Should I respin patch 6 as a series doing the above?
> 
> I assume that the first 5 patches are fine as-is.

Yep, they're fine. Sure, go ahead and respin patch 6.

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

[Matt Fleming]

On Mon, 25 Apr, at 12:04:55PM, Mark Rutland wrote:
> On Mon, Apr 25, 2016 at 11:51:53AM +0100, Matt Fleming wrote:
> > On Mon, 25 Apr, at 11:40:09AM, Mark Rutland wrote:
> > > 
> > > It looks like irqs_disabled_flags() will do what you expect, and ignore
> > > everything but the interrupt flag.
> > > 
> > > However, for ARM that will ignore the other exceptions we've seen FW
> > > erroneously unmask (e.g. FIQ), which is unfortunate, as fiddling with
> > > those is just as disastrous.
> >  
> > Bah, right.
> > 
> > > Would you be happy with an arch_efi_call_check_flags(before, after),
> > > instead? That way we can make the flags we check arch-specific.
> > 
> > Could we just make the flag mask arch-specific instead of the call
> > since the rest of efi_call_virt_check_flags() is good?
> 
> Yup, I meant that arch_efi_call_check_flags would only do the flag
> comparison, only replacing the bit currently in the WARN_ON_ONCE().
> 
> > Something like this (uncompiled, untested, half-baked),
> > 
> > ---
> > 
> > diff --git a/drivers/firmware/efi/runtime-wrappers.c 
> > b/drivers/firmware/efi/runtime-wrappers.c
> > index c38b1cfc26e2..057d00bee7d6 100644
> > --- a/drivers/firmware/efi/runtime-wrappers.c
> > +++ b/drivers/firmware/efi/runtime-wrappers.c
> > @@ -25,9 +25,12 @@
> >  static void efi_call_virt_check_flags(unsigned long flags, const char 
> > *call)
> >  {
> > unsigned long cur_flags;
> > +   bool mismatch;
> >  
> > local_save_flags(cur_flags);
> > -   if (!WARN_ON_ONCE(cur_flags != flags))
> > +
> > +   mismatch = (cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK;
> > +   if (!WARN_ON_ONCE(mismatch))
> > return;
> 
> This style also works for me.
 
Cool. One thing that occurred to me after I sent it is that
technically we should either,

  1) make 'mismatch' an int or
  2) do mismatch = !!((cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK)

Either is fine with me, I just don't want to leave the implicit
conversion to C's type system.

> Should I respin patch 6 as a series doing the above?
> 
> I assume that the first 5 patches are fine as-is.

Yep, they're fine. Sure, go ahead and respin patch 6.

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

[Mark Rutland]

On Mon, Apr 25, 2016 at 11:51:53AM +0100, Matt Fleming wrote:
> On Mon, 25 Apr, at 11:40:09AM, Mark Rutland wrote:
> > 
> > It looks like irqs_disabled_flags() will do what you expect, and ignore
> > everything but the interrupt flag.
> > 
> > However, for ARM that will ignore the other exceptions we've seen FW
> > erroneously unmask (e.g. FIQ), which is unfortunate, as fiddling with
> > those is just as disastrous.
>  
> Bah, right.
> 
> > Would you be happy with an arch_efi_call_check_flags(before, after),
> > instead? That way we can make the flags we check arch-specific.
> 
> Could we just make the flag mask arch-specific instead of the call
> since the rest of efi_call_virt_check_flags() is good?

Yup, I meant that arch_efi_call_check_flags would only do the flag
comparison, only replacing the bit currently in the WARN_ON_ONCE().

> Something like this (uncompiled, untested, half-baked),
> 
> ---
> 
> diff --git a/drivers/firmware/efi/runtime-wrappers.c 
> b/drivers/firmware/efi/runtime-wrappers.c
> index c38b1cfc26e2..057d00bee7d6 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -25,9 +25,12 @@
>  static void efi_call_virt_check_flags(unsigned long flags, const char *call)
>  {
>   unsigned long cur_flags;
> + bool mismatch;
>  
>   local_save_flags(cur_flags);
> - if (!WARN_ON_ONCE(cur_flags != flags))
> +
> + mismatch = (cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK;
> + if (!WARN_ON_ONCE(mismatch))
>   return;

This style also works for me.

Should I respin patch 6 as a series doing the above?

I assume that the first 5 patches are fine as-is.

Thanks,
Mark.

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

[Mark Rutland]

On Mon, Apr 25, 2016 at 11:51:53AM +0100, Matt Fleming wrote:
> On Mon, 25 Apr, at 11:40:09AM, Mark Rutland wrote:
> > 
> > It looks like irqs_disabled_flags() will do what you expect, and ignore
> > everything but the interrupt flag.
> > 
> > However, for ARM that will ignore the other exceptions we've seen FW
> > erroneously unmask (e.g. FIQ), which is unfortunate, as fiddling with
> > those is just as disastrous.
>  
> Bah, right.
> 
> > Would you be happy with an arch_efi_call_check_flags(before, after),
> > instead? That way we can make the flags we check arch-specific.
> 
> Could we just make the flag mask arch-specific instead of the call
> since the rest of efi_call_virt_check_flags() is good?

Yup, I meant that arch_efi_call_check_flags would only do the flag
comparison, only replacing the bit currently in the WARN_ON_ONCE().

> Something like this (uncompiled, untested, half-baked),
> 
> ---
> 
> diff --git a/drivers/firmware/efi/runtime-wrappers.c 
> b/drivers/firmware/efi/runtime-wrappers.c
> index c38b1cfc26e2..057d00bee7d6 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -25,9 +25,12 @@
>  static void efi_call_virt_check_flags(unsigned long flags, const char *call)
>  {
>   unsigned long cur_flags;
> + bool mismatch;
>  
>   local_save_flags(cur_flags);
> - if (!WARN_ON_ONCE(cur_flags != flags))
> +
> + mismatch = (cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK;
> + if (!WARN_ON_ONCE(mismatch))
>   return;

This style also works for me.

Should I respin patch 6 as a series doing the above?

I assume that the first 5 patches are fine as-is.

Thanks,
Mark.

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

[Matt Fleming]

On Mon, 25 Apr, at 11:40:09AM, Mark Rutland wrote:
> 
> It looks like irqs_disabled_flags() will do what you expect, and ignore
> everything but the interrupt flag.
> 
> However, for ARM that will ignore the other exceptions we've seen FW
> erroneously unmask (e.g. FIQ), which is unfortunate, as fiddling with
> those is just as disastrous.
 
Bah, right.

> Would you be happy with an arch_efi_call_check_flags(before, after),
> instead? That way we can make the flags we check arch-specific.

Could we just make the flag mask arch-specific instead of the call
since the rest of efi_call_virt_check_flags() is good?

Something like this (uncompiled, untested, half-baked),

---

diff --git a/drivers/firmware/efi/runtime-wrappers.c 
b/drivers/firmware/efi/runtime-wrappers.c
index c38b1cfc26e2..057d00bee7d6 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -25,9 +25,12 @@
 static void efi_call_virt_check_flags(unsigned long flags, const char *call)
 {
unsigned long cur_flags;
+   bool mismatch;
 
local_save_flags(cur_flags);
-   if (!WARN_ON_ONCE(cur_flags != flags))
+
+   mismatch = (cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK;
+   if (!WARN_ON_ONCE(mismatch))
return;
 
add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_NOW_UNRELIABLE);

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

[Matt Fleming]

On Mon, 25 Apr, at 11:40:09AM, Mark Rutland wrote:
> 
> It looks like irqs_disabled_flags() will do what you expect, and ignore
> everything but the interrupt flag.
> 
> However, for ARM that will ignore the other exceptions we've seen FW
> erroneously unmask (e.g. FIQ), which is unfortunate, as fiddling with
> those is just as disastrous.
 
Bah, right.

> Would you be happy with an arch_efi_call_check_flags(before, after),
> instead? That way we can make the flags we check arch-specific.

Could we just make the flag mask arch-specific instead of the call
since the rest of efi_call_virt_check_flags() is good?

Something like this (uncompiled, untested, half-baked),

---

diff --git a/drivers/firmware/efi/runtime-wrappers.c 
b/drivers/firmware/efi/runtime-wrappers.c
index c38b1cfc26e2..057d00bee7d6 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -25,9 +25,12 @@
 static void efi_call_virt_check_flags(unsigned long flags, const char *call)
 {
unsigned long cur_flags;
+   bool mismatch;
 
local_save_flags(cur_flags);
-   if (!WARN_ON_ONCE(cur_flags != flags))
+
+   mismatch = (cur_flags ^ flags) & ARCH_EFI_IRQ_FLAGS_MASK;
+   if (!WARN_ON_ONCE(mismatch))
return;
 
add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_NOW_UNRELIABLE);

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

[Mark Rutland]

On Mon, Apr 25, 2016 at 11:28:21AM +0100, Matt Fleming wrote:
> On Mon, 25 Apr, at 12:21:18PM, Ard Biesheuvel wrote:
> > (+ Laszlo)
> > 
> > On 25 April 2016 at 12:15, Matt Fleming  wrote:
> > > On Sun, 24 Apr, at 10:22:41PM, Matt Fleming wrote:
> > >>
> > >> I like this series a lot (well, ignoring the fact that the firmware is
> > >> trying to eat itself). The runtime call code is much cleaner now, and
> > >> this is a great precedent for any future multi-architecture quirks we
> > >> may need.
> > >>
> > >> Queued for v4.7, thanks everyone!
> > >
> > > Hmm... Booting this series with Qemu and OVMF results in lots of
> > > warnings,
> > >
> > > [ 0.102173] [ cut here ]
> > > [ 0.103000] WARNING: CPU: 0 PID: 0 at 
> > > /dev/shm/mfleming/git/efi/drivers/firmware/efi/runtime-wrappers.c:30 
> > > efi_call_virt_check_flags+0x69/0x90
> > > [ 0.103505] Modules linked in:
> > > [ 0.104519] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.6.0-rc4+ #1
> > > [0.105000]   81e03e30 8132206f 
> > > 
> > > [0.105000]   81e03e70 8105a47c 
> > > 001e000a
> > > [0.105000]  0246 0286 81bed975 
> > > 81e03f10
> > > [0.105000] Call Trace:
> > > [0.105000]  [] dump_stack+0x4d/0x6e
> > > [0.105000]  [] __warn+0xcc/0xf0
> > > [0.105000]  [] warn_slowpath_null+0x18/0x20
> > > [0.105000]  [] efi_call_virt_check_flags+0x69/0x90
> > > [0.105000]  [] virt_efi_set_variable+0x82/0x190
> > > [0.105000]  [] efi_delete_dummy_variable+0x75/0x80
> > > [0.105000]  [] efi_enter_virtual_mode+0x463/0x472
> > > [0.105000]  [] start_kernel+0x38f/0x415
> > > [0.105000]  [] ? set_init_arg+0x55/0x55
> > > [0.105000]  [] x86_64_start_reservations+0x2a/0x2c
> > > [0.105000]  [] x86_64_start_kernel+0xea/0xed
> > > [0.107181] ---[ end trace 0081cc453369d969 ]---
> > > [0.107499] Disabling lock debugging due to kernel taint
> > > [0.108226] [Firmware Bug]: IRQ flags corrupted 
> > > (0x0246=>0x0286) by EFI set_variable
> > >
> > > Has anyone tested this series on x86 to ensure that this is a rare
> > > case? I'll go and test some physical x86 machines now.
> > 
> > I suppose that it is quite likely that this issue occurs in the wild
> > if it is present in OVMF. Could anyone check which flag is actually
> > clobbered here?
> 
> X86_EFLAGS_ZF (zero flag) gets turned off and X86_EFLAGS_SF (signed
> flag) gets turned on. Which is totally legitimate and isn't grounds
> for a warning... 
> 
> I think the function we want to use instead of local_save_flags() is
> irqs_disabled_flags().

It looks like irqs_disabled_flags() will do what you expect, and ignore
everything but the interrupt flag.

However, for ARM that will ignore the other exceptions we've seen FW
erroneously unmask (e.g. FIQ), which is unfortunate, as fiddling with
those is just as disastrous.

Would you be happy with an arch_efi_call_check_flags(before, after),
instead? That way we can make the flags we check arch-specific.

Thanks,
Mark.

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

[Mark Rutland]

On Mon, Apr 25, 2016 at 11:28:21AM +0100, Matt Fleming wrote:
> On Mon, 25 Apr, at 12:21:18PM, Ard Biesheuvel wrote:
> > (+ Laszlo)
> > 
> > On 25 April 2016 at 12:15, Matt Fleming  wrote:
> > > On Sun, 24 Apr, at 10:22:41PM, Matt Fleming wrote:
> > >>
> > >> I like this series a lot (well, ignoring the fact that the firmware is
> > >> trying to eat itself). The runtime call code is much cleaner now, and
> > >> this is a great precedent for any future multi-architecture quirks we
> > >> may need.
> > >>
> > >> Queued for v4.7, thanks everyone!
> > >
> > > Hmm... Booting this series with Qemu and OVMF results in lots of
> > > warnings,
> > >
> > > [ 0.102173] [ cut here ]
> > > [ 0.103000] WARNING: CPU: 0 PID: 0 at 
> > > /dev/shm/mfleming/git/efi/drivers/firmware/efi/runtime-wrappers.c:30 
> > > efi_call_virt_check_flags+0x69/0x90
> > > [ 0.103505] Modules linked in:
> > > [ 0.104519] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.6.0-rc4+ #1
> > > [0.105000]   81e03e30 8132206f 
> > > 
> > > [0.105000]   81e03e70 8105a47c 
> > > 001e000a
> > > [0.105000]  0246 0286 81bed975 
> > > 81e03f10
> > > [0.105000] Call Trace:
> > > [0.105000]  [] dump_stack+0x4d/0x6e
> > > [0.105000]  [] __warn+0xcc/0xf0
> > > [0.105000]  [] warn_slowpath_null+0x18/0x20
> > > [0.105000]  [] efi_call_virt_check_flags+0x69/0x90
> > > [0.105000]  [] virt_efi_set_variable+0x82/0x190
> > > [0.105000]  [] efi_delete_dummy_variable+0x75/0x80
> > > [0.105000]  [] efi_enter_virtual_mode+0x463/0x472
> > > [0.105000]  [] start_kernel+0x38f/0x415
> > > [0.105000]  [] ? set_init_arg+0x55/0x55
> > > [0.105000]  [] x86_64_start_reservations+0x2a/0x2c
> > > [0.105000]  [] x86_64_start_kernel+0xea/0xed
> > > [0.107181] ---[ end trace 0081cc453369d969 ]---
> > > [0.107499] Disabling lock debugging due to kernel taint
> > > [0.108226] [Firmware Bug]: IRQ flags corrupted 
> > > (0x0246=>0x0286) by EFI set_variable
> > >
> > > Has anyone tested this series on x86 to ensure that this is a rare
> > > case? I'll go and test some physical x86 machines now.
> > 
> > I suppose that it is quite likely that this issue occurs in the wild
> > if it is present in OVMF. Could anyone check which flag is actually
> > clobbered here?
> 
> X86_EFLAGS_ZF (zero flag) gets turned off and X86_EFLAGS_SF (signed
> flag) gets turned on. Which is totally legitimate and isn't grounds
> for a warning... 
> 
> I think the function we want to use instead of local_save_flags() is
> irqs_disabled_flags().

It looks like irqs_disabled_flags() will do what you expect, and ignore
everything but the interrupt flag.

However, for ARM that will ignore the other exceptions we've seen FW
erroneously unmask (e.g. FIQ), which is unfortunate, as fiddling with
those is just as disastrous.

Would you be happy with an arch_efi_call_check_flags(before, after),
instead? That way we can make the flags we check arch-specific.

Thanks,
Mark.

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

[Matt Fleming]

On Mon, 25 Apr, at 12:21:18PM, Ard Biesheuvel wrote:
> (+ Laszlo)
> 
> On 25 April 2016 at 12:15, Matt Fleming  wrote:
> > On Sun, 24 Apr, at 10:22:41PM, Matt Fleming wrote:
> >>
> >> I like this series a lot (well, ignoring the fact that the firmware is
> >> trying to eat itself). The runtime call code is much cleaner now, and
> >> this is a great precedent for any future multi-architecture quirks we
> >> may need.
> >>
> >> Queued for v4.7, thanks everyone!
> >
> > Hmm... Booting this series with Qemu and OVMF results in lots of
> > warnings,
> >
> > [ 0.102173] [ cut here ]
> > [ 0.103000] WARNING: CPU: 0 PID: 0 at 
> > /dev/shm/mfleming/git/efi/drivers/firmware/efi/runtime-wrappers.c:30 
> > efi_call_virt_check_flags+0x69/0x90
> > [ 0.103505] Modules linked in:
> > [ 0.104519] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.6.0-rc4+ #1
> > [0.105000]   81e03e30 8132206f 
> > 
> > [0.105000]   81e03e70 8105a47c 
> > 001e000a
> > [0.105000]  0246 0286 81bed975 
> > 81e03f10
> > [0.105000] Call Trace:
> > [0.105000]  [] dump_stack+0x4d/0x6e
> > [0.105000]  [] __warn+0xcc/0xf0
> > [0.105000]  [] warn_slowpath_null+0x18/0x20
> > [0.105000]  [] efi_call_virt_check_flags+0x69/0x90
> > [0.105000]  [] virt_efi_set_variable+0x82/0x190
> > [0.105000]  [] efi_delete_dummy_variable+0x75/0x80
> > [0.105000]  [] efi_enter_virtual_mode+0x463/0x472
> > [0.105000]  [] start_kernel+0x38f/0x415
> > [0.105000]  [] ? set_init_arg+0x55/0x55
> > [0.105000]  [] x86_64_start_reservations+0x2a/0x2c
> > [0.105000]  [] x86_64_start_kernel+0xea/0xed
> > [0.107181] ---[ end trace 0081cc453369d969 ]---
> > [0.107499] Disabling lock debugging due to kernel taint
> > [0.108226] [Firmware Bug]: IRQ flags corrupted (0x0246=>0x0286) 
> > by EFI set_variable
> >
> > Has anyone tested this series on x86 to ensure that this is a rare
> > case? I'll go and test some physical x86 machines now.
> 
> I suppose that it is quite likely that this issue occurs in the wild
> if it is present in OVMF. Could anyone check which flag is actually
> clobbered here?

X86_EFLAGS_ZF (zero flag) gets turned off and X86_EFLAGS_SF (signed
flag) gets turned on. Which is totally legitimate and isn't grounds
for a warning... 

I think the function we want to use instead of local_save_flags() is
irqs_disabled_flags().

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

[Matt Fleming]

On Mon, 25 Apr, at 12:21:18PM, Ard Biesheuvel wrote:
> (+ Laszlo)
> 
> On 25 April 2016 at 12:15, Matt Fleming  wrote:
> > On Sun, 24 Apr, at 10:22:41PM, Matt Fleming wrote:
> >>
> >> I like this series a lot (well, ignoring the fact that the firmware is
> >> trying to eat itself). The runtime call code is much cleaner now, and
> >> this is a great precedent for any future multi-architecture quirks we
> >> may need.
> >>
> >> Queued for v4.7, thanks everyone!
> >
> > Hmm... Booting this series with Qemu and OVMF results in lots of
> > warnings,
> >
> > [ 0.102173] [ cut here ]
> > [ 0.103000] WARNING: CPU: 0 PID: 0 at 
> > /dev/shm/mfleming/git/efi/drivers/firmware/efi/runtime-wrappers.c:30 
> > efi_call_virt_check_flags+0x69/0x90
> > [ 0.103505] Modules linked in:
> > [ 0.104519] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.6.0-rc4+ #1
> > [0.105000]   81e03e30 8132206f 
> > 
> > [0.105000]   81e03e70 8105a47c 
> > 001e000a
> > [0.105000]  0246 0286 81bed975 
> > 81e03f10
> > [0.105000] Call Trace:
> > [0.105000]  [] dump_stack+0x4d/0x6e
> > [0.105000]  [] __warn+0xcc/0xf0
> > [0.105000]  [] warn_slowpath_null+0x18/0x20
> > [0.105000]  [] efi_call_virt_check_flags+0x69/0x90
> > [0.105000]  [] virt_efi_set_variable+0x82/0x190
> > [0.105000]  [] efi_delete_dummy_variable+0x75/0x80
> > [0.105000]  [] efi_enter_virtual_mode+0x463/0x472
> > [0.105000]  [] start_kernel+0x38f/0x415
> > [0.105000]  [] ? set_init_arg+0x55/0x55
> > [0.105000]  [] x86_64_start_reservations+0x2a/0x2c
> > [0.105000]  [] x86_64_start_kernel+0xea/0xed
> > [0.107181] ---[ end trace 0081cc453369d969 ]---
> > [0.107499] Disabling lock debugging due to kernel taint
> > [0.108226] [Firmware Bug]: IRQ flags corrupted (0x0246=>0x0286) 
> > by EFI set_variable
> >
> > Has anyone tested this series on x86 to ensure that this is a rare
> > case? I'll go and test some physical x86 machines now.
> 
> I suppose that it is quite likely that this issue occurs in the wild
> if it is present in OVMF. Could anyone check which flag is actually
> clobbered here?

X86_EFLAGS_ZF (zero flag) gets turned off and X86_EFLAGS_SF (signed
flag) gets turned on. Which is totally legitimate and isn't grounds
for a warning... 

I think the function we want to use instead of local_save_flags() is
irqs_disabled_flags().

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

[Ard Biesheuvel]

(+ Laszlo)

On 25 April 2016 at 12:15, Matt Fleming  wrote:
> On Sun, 24 Apr, at 10:22:41PM, Matt Fleming wrote:
>>
>> I like this series a lot (well, ignoring the fact that the firmware is
>> trying to eat itself). The runtime call code is much cleaner now, and
>> this is a great precedent for any future multi-architecture quirks we
>> may need.
>>
>> Queued for v4.7, thanks everyone!
>
> Hmm... Booting this series with Qemu and OVMF results in lots of
> warnings,
>
> [ 0.102173] [ cut here ]
> [ 0.103000] WARNING: CPU: 0 PID: 0 at 
> /dev/shm/mfleming/git/efi/drivers/firmware/efi/runtime-wrappers.c:30 
> efi_call_virt_check_flags+0x69/0x90
> [ 0.103505] Modules linked in:
> [ 0.104519] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.6.0-rc4+ #1
> [0.105000]   81e03e30 8132206f 
> 
> [0.105000]   81e03e70 8105a47c 
> 001e000a
> [0.105000]  0246 0286 81bed975 
> 81e03f10
> [0.105000] Call Trace:
> [0.105000]  [] dump_stack+0x4d/0x6e
> [0.105000]  [] __warn+0xcc/0xf0
> [0.105000]  [] warn_slowpath_null+0x18/0x20
> [0.105000]  [] efi_call_virt_check_flags+0x69/0x90
> [0.105000]  [] virt_efi_set_variable+0x82/0x190
> [0.105000]  [] efi_delete_dummy_variable+0x75/0x80
> [0.105000]  [] efi_enter_virtual_mode+0x463/0x472
> [0.105000]  [] start_kernel+0x38f/0x415
> [0.105000]  [] ? set_init_arg+0x55/0x55
> [0.105000]  [] x86_64_start_reservations+0x2a/0x2c
> [0.105000]  [] x86_64_start_kernel+0xea/0xed
> [0.107181] ---[ end trace 0081cc453369d969 ]---
> [0.107499] Disabling lock debugging due to kernel taint
> [0.108226] [Firmware Bug]: IRQ flags corrupted (0x0246=>0x0286) 
> by EFI set_variable
>
> Has anyone tested this series on x86 to ensure that this is a rare
> case? I'll go and test some physical x86 machines now.

I suppose that it is quite likely that this issue occurs in the wild
if it is present in OVMF. Could anyone check which flag is actually
clobbered here?

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

[Ard Biesheuvel]

(+ Laszlo)

On 25 April 2016 at 12:15, Matt Fleming  wrote:
> On Sun, 24 Apr, at 10:22:41PM, Matt Fleming wrote:
>>
>> I like this series a lot (well, ignoring the fact that the firmware is
>> trying to eat itself). The runtime call code is much cleaner now, and
>> this is a great precedent for any future multi-architecture quirks we
>> may need.
>>
>> Queued for v4.7, thanks everyone!
>
> Hmm... Booting this series with Qemu and OVMF results in lots of
> warnings,
>
> [ 0.102173] [ cut here ]
> [ 0.103000] WARNING: CPU: 0 PID: 0 at 
> /dev/shm/mfleming/git/efi/drivers/firmware/efi/runtime-wrappers.c:30 
> efi_call_virt_check_flags+0x69/0x90
> [ 0.103505] Modules linked in:
> [ 0.104519] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.6.0-rc4+ #1
> [0.105000]   81e03e30 8132206f 
> 
> [0.105000]   81e03e70 8105a47c 
> 001e000a
> [0.105000]  0246 0286 81bed975 
> 81e03f10
> [0.105000] Call Trace:
> [0.105000]  [] dump_stack+0x4d/0x6e
> [0.105000]  [] __warn+0xcc/0xf0
> [0.105000]  [] warn_slowpath_null+0x18/0x20
> [0.105000]  [] efi_call_virt_check_flags+0x69/0x90
> [0.105000]  [] virt_efi_set_variable+0x82/0x190
> [0.105000]  [] efi_delete_dummy_variable+0x75/0x80
> [0.105000]  [] efi_enter_virtual_mode+0x463/0x472
> [0.105000]  [] start_kernel+0x38f/0x415
> [0.105000]  [] ? set_init_arg+0x55/0x55
> [0.105000]  [] x86_64_start_reservations+0x2a/0x2c
> [0.105000]  [] x86_64_start_kernel+0xea/0xed
> [0.107181] ---[ end trace 0081cc453369d969 ]---
> [0.107499] Disabling lock debugging due to kernel taint
> [0.108226] [Firmware Bug]: IRQ flags corrupted (0x0246=>0x0286) 
> by EFI set_variable
>
> Has anyone tested this series on x86 to ensure that this is a rare
> case? I'll go and test some physical x86 machines now.

I suppose that it is quite likely that this issue occurs in the wild
if it is present in OVMF. Could anyone check which flag is actually
clobbered here?

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

[Matt Fleming]

On Sun, 24 Apr, at 10:22:41PM, Matt Fleming wrote:
> 
> I like this series a lot (well, ignoring the fact that the firmware is
> trying to eat itself). The runtime call code is much cleaner now, and
> this is a great precedent for any future multi-architecture quirks we
> may need.
> 
> Queued for v4.7, thanks everyone!

Hmm... Booting this series with Qemu and OVMF results in lots of
warnings,

[0.102173] [ cut here ]
[0.103000] WARNING: CPU: 0 PID: 0 at 
/dev/shm/mfleming/git/efi/drivers/firmware/efi/runtime-wrappers.c:30 
efi_call_virt_check_flags+0x69/0x90
[0.103505] Modules linked in:
[0.104519] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.6.0-rc4+ #1
[0.105000]   81e03e30 8132206f 

[0.105000]   81e03e70 8105a47c 
001e000a
[0.105000]  0246 0286 81bed975 
81e03f10
[0.105000] Call Trace:
[0.105000]  [] dump_stack+0x4d/0x6e
[0.105000]  [] __warn+0xcc/0xf0
[0.105000]  [] warn_slowpath_null+0x18/0x20
[0.105000]  [] efi_call_virt_check_flags+0x69/0x90
[0.105000]  [] virt_efi_set_variable+0x82/0x190
[0.105000]  [] efi_delete_dummy_variable+0x75/0x80
[0.105000]  [] efi_enter_virtual_mode+0x463/0x472
[0.105000]  [] start_kernel+0x38f/0x415
[0.105000]  [] ? set_init_arg+0x55/0x55
[0.105000]  [] x86_64_start_reservations+0x2a/0x2c
[0.105000]  [] x86_64_start_kernel+0xea/0xed
[0.107181] ---[ end trace 0081cc453369d969 ]---
[0.107499] Disabling lock debugging due to kernel taint
[0.108226] [Firmware Bug]: IRQ flags corrupted (0x0246=>0x0286) by 
EFI set_variable

Has anyone tested this series on x86 to ensure that this is a rare
case? I'll go and test some physical x86 machines now.

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

[Matt Fleming]

On Sun, 24 Apr, at 10:22:41PM, Matt Fleming wrote:
> 
> I like this series a lot (well, ignoring the fact that the firmware is
> trying to eat itself). The runtime call code is much cleaner now, and
> this is a great precedent for any future multi-architecture quirks we
> may need.
> 
> Queued for v4.7, thanks everyone!

Hmm... Booting this series with Qemu and OVMF results in lots of
warnings,

[0.102173] [ cut here ]
[0.103000] WARNING: CPU: 0 PID: 0 at 
/dev/shm/mfleming/git/efi/drivers/firmware/efi/runtime-wrappers.c:30 
efi_call_virt_check_flags+0x69/0x90
[0.103505] Modules linked in:
[0.104519] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.6.0-rc4+ #1
[0.105000]   81e03e30 8132206f 

[0.105000]   81e03e70 8105a47c 
001e000a
[0.105000]  0246 0286 81bed975 
81e03f10
[0.105000] Call Trace:
[0.105000]  [] dump_stack+0x4d/0x6e
[0.105000]  [] __warn+0xcc/0xf0
[0.105000]  [] warn_slowpath_null+0x18/0x20
[0.105000]  [] efi_call_virt_check_flags+0x69/0x90
[0.105000]  [] virt_efi_set_variable+0x82/0x190
[0.105000]  [] efi_delete_dummy_variable+0x75/0x80
[0.105000]  [] efi_enter_virtual_mode+0x463/0x472
[0.105000]  [] start_kernel+0x38f/0x415
[0.105000]  [] ? set_init_arg+0x55/0x55
[0.105000]  [] x86_64_start_reservations+0x2a/0x2c
[0.105000]  [] x86_64_start_kernel+0xea/0xed
[0.107181] ---[ end trace 0081cc453369d969 ]---
[0.107499] Disabling lock debugging due to kernel taint
[0.108226] [Firmware Bug]: IRQ flags corrupted (0x0246=>0x0286) by 
EFI set_variable

Has anyone tested this series on x86 to ensure that this is a rare
case? I'll go and test some physical x86 machines now.

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

[Matt Fleming]

On Fri, 22 Apr, at 04:12:59PM, Ard Biesheuvel wrote:
> On 22 April 2016 at 15:51, 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 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
> >
> > Ard, I assume that your Reviewed-by still stands for the final patch, even
> > though efi_call_virt_check_flags moved. Please shout if that's not the case!
> >
> 
> No, that's fine. Thanks for respinning so quickly.
> 
> > Hopefully you're also happy to extend that to the new patch removing the
> > ifdefs once they become superfluous.
> >
> 
> Matt: in case your review bandwidth is limited atm, I'd much prefer
> this series making v4.7 than the GOP stuff or the other stuff i have
> been posting over the past weeks.

I like this series a lot (well, ignoring the fact that the firmware is
trying to eat itself). The runtime call code is much cleaner now, and
this is a great precedent for any future multi-architecture quirks we
may need.

Queued for v4.7, thanks everyone!

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

[Matt Fleming]

On Fri, 22 Apr, at 04:12:59PM, Ard Biesheuvel wrote:
> On 22 April 2016 at 15:51, 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 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
> >
> > Ard, I assume that your Reviewed-by still stands for the final patch, even
> > though efi_call_virt_check_flags moved. Please shout if that's not the case!
> >
> 
> No, that's fine. Thanks for respinning so quickly.
> 
> > Hopefully you're also happy to extend that to the new patch removing the
> > ifdefs once they become superfluous.
> >
> 
> Matt: in case your review bandwidth is limited atm, I'd much prefer
> this series making v4.7 than the GOP stuff or the other stuff i have
> been posting over the past weeks.

I like this series a lot (well, ignoring the fact that the firmware is
trying to eat itself). The runtime call code is much cleaner now, and
this is a great precedent for any future multi-architecture quirks we
may need.

Queued for v4.7, thanks everyone!

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

[Ard Biesheuvel]

On 22 April 2016 at 15:51, 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 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
>
> Ard, I assume that your Reviewed-by still stands for the final patch, even
> though efi_call_virt_check_flags moved. Please shout if that's not the case!
>

No, that's fine. Thanks for respinning so quickly.

> Hopefully you're also happy to extend that to the new patch removing the
> ifdefs once they become superfluous.
>

Matt: in case your review bandwidth is limited atm, I'd much prefer
this series making v4.7 than the GOP stuff or the other stuff i have
been posting over the past weeks.

Thanks,
Ard.

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

[Ard Biesheuvel]

On 22 April 2016 at 15:51, 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 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
>
> Ard, I assume that your Reviewed-by still stands for the final patch, even
> though efi_call_virt_check_flags moved. Please shout if that's not the case!
>

No, that's fine. Thanks for respinning so quickly.

> Hopefully you're also happy to extend that to the new patch removing the
> ifdefs once they become superfluous.
>

Matt: in case your review bandwidth is limited atm, I'd much prefer
this series making v4.7 than the GOP stuff or the other stuff i have
been posting over the past weeks.

Thanks,
Ard.