Patch "x86/entry/64: Add missing irqflags tracing to native_load_gs_index()" has been added to the 4.9-stable tree

Patch "x86/entry/64: Add missing irqflags tracing to native_load_gs_index()" has been added to the 4.9-stable tree

[gregkh]

This is a note to let you know that I've just added the patch titled

    x86/entry/64: Add missing irqflags tracing to native_load_gs_index()

to the 4.9-stable tree which can be found at:
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
     x86-entry-64-add-missing-irqflags-tracing-to-native_load_gs_index.patch
and it can be found in the queue-4.9 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@vger.kernel.org> know about it.


>From ca37e57bbe0cf1455ea3e84eb89ed04a132d59e1 Mon Sep 17 00:00:00 2001
From: Andy Lutomirski <luto@kernel.org>
Date: Wed, 22 Nov 2017 20:39:16 -0800
Subject: x86/entry/64: Add missing irqflags tracing to native_load_gs_index()

From: Andy Lutomirski <luto@kernel.org>

commit ca37e57bbe0cf1455ea3e84eb89ed04a132d59e1 upstream.

Running this code with IRQs enabled (where dummy_lock is a spinlock):

static void check_load_gs_index(void)
{
	/* This will fail. */
	load_gs_index(0xffff);

	spin_lock(&dummy_lock);
	spin_unlock(&dummy_lock);
}

Will generate a lockdep warning.  The issue is that the actual write
to %gs would cause an exception with IRQs disabled, and the exception
handler would, as an inadvertent side effect, update irqflag tracing
to reflect the IRQs-off status.  native_load_gs_index() would then
turn IRQs back on and return with irqflag tracing still thinking that
IRQs were off.  The dummy lock-and-unlock causes lockdep to notice the
error and warn.

Fix it by adding the missing tracing.

Apparently nothing did this in a context where it mattered.  I haven't
tried to find a code path that would actually exhibit the warning if
appropriately nasty user code were running.

I suspect that the security impact of this bug is very, very low --
production systems don't run with lockdep enabled, and the warning is
mostly harmless anyway.

Found during a quick audit of the entry code to try to track down an
unrelated bug that Ingo found in some still-in-development code.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bpetkov@suse.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/e1aeb0e6ba8dd430ec36c8a35e63b429698b4132.1511411918.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 arch/x86/entry/entry_64.S |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -54,15 +54,19 @@ ENTRY(native_usergs_sysret64)
 ENDPROC(native_usergs_sysret64)
 #endif /* CONFIG_PARAVIRT */
 
-.macro TRACE_IRQS_IRETQ
+.macro TRACE_IRQS_FLAGS flags:req
 #ifdef CONFIG_TRACE_IRQFLAGS
-	bt	$9, EFLAGS(%rsp)		/* interrupts off? */
+	bt	$9, \flags		/* interrupts off? */
 	jnc	1f
 	TRACE_IRQS_ON
 1:
 #endif
 .endm
 
+.macro TRACE_IRQS_IRETQ
+	TRACE_IRQS_FLAGS EFLAGS(%rsp)
+.endm
+
 /*
  * When dynamic function tracer is enabled it will add a breakpoint
  * to all locations that it is about to modify, sync CPUs, update
@@ -868,11 +872,13 @@ idtentry simd_coprocessor_error		do_simd
 ENTRY(native_load_gs_index)
 	pushfq
 	DISABLE_INTERRUPTS(CLBR_ANY & ~CLBR_RDI)
+	TRACE_IRQS_OFF
 	SWAPGS
 .Lgs_change:
 	movl	%edi, %gs
 2:	ALTERNATIVE "", "mfence", X86_BUG_SWAPGS_FENCE
 	SWAPGS
+	TRACE_IRQS_FLAGS (%rsp)
 	popfq
 	ret
 END(native_load_gs_index)


Patches currently in stable-queue which might be from luto@kernel.org are

queue-4.9/x86-entry-64-add-missing-irqflags-tracing-to-native_load_gs_index.patch

Re: Patch "x86/entry/64: Add missing irqflags tracing to native_load_gs_index()" has been added to the 4.9-stable tree

[Andy Lutomirski]

If this hasn't been released yet, please hold off.  It's buggy.

On Mon, Nov 27, 2017 at 4:41 AM,  <gregkh@linuxfoundation.org> wrote:
>
> This is a note to let you know that I've just added the patch titled
>
>     x86/entry/64: Add missing irqflags tracing to native_load_gs_index()
>
> to the 4.9-stable tree which can be found at:
>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>
> The filename of the patch is:
>      x86-entry-64-add-missing-irqflags-tracing-to-native_load_gs_index.patch
> and it can be found in the queue-4.9 subdirectory.
>
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.
>
>
> From ca37e57bbe0cf1455ea3e84eb89ed04a132d59e1 Mon Sep 17 00:00:00 2001
> From: Andy Lutomirski <luto@kernel.org>
> Date: Wed, 22 Nov 2017 20:39:16 -0800
> Subject: x86/entry/64: Add missing irqflags tracing to native_load_gs_index()
>
> From: Andy Lutomirski <luto@kernel.org>
>
> commit ca37e57bbe0cf1455ea3e84eb89ed04a132d59e1 upstream.
>
> Running this code with IRQs enabled (where dummy_lock is a spinlock):
>
> static void check_load_gs_index(void)
> {
>         /* This will fail. */
>         load_gs_index(0xffff);
>
>         spin_lock(&dummy_lock);
>         spin_unlock(&dummy_lock);
> }
>
> Will generate a lockdep warning.  The issue is that the actual write
> to %gs would cause an exception with IRQs disabled, and the exception
> handler would, as an inadvertent side effect, update irqflag tracing
> to reflect the IRQs-off status.  native_load_gs_index() would then
> turn IRQs back on and return with irqflag tracing still thinking that
> IRQs were off.  The dummy lock-and-unlock causes lockdep to notice the
> error and warn.
>
> Fix it by adding the missing tracing.
>
> Apparently nothing did this in a context where it mattered.  I haven't
> tried to find a code path that would actually exhibit the warning if
> appropriately nasty user code were running.
>
> I suspect that the security impact of this bug is very, very low --
> production systems don't run with lockdep enabled, and the warning is
> mostly harmless anyway.
>
> Found during a quick audit of the entry code to try to track down an
> unrelated bug that Ingo found in some still-in-development code.
>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> Cc: Borislav Petkov <bpetkov@suse.de>
> Cc: Brian Gerst <brgerst@gmail.com>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: http://lkml.kernel.org/r/e1aeb0e6ba8dd430ec36c8a35e63b429698b4132.1511411918.git.luto@kernel.org
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> ---
>  arch/x86/entry/entry_64.S |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -54,15 +54,19 @@ ENTRY(native_usergs_sysret64)
>  ENDPROC(native_usergs_sysret64)
>  #endif /* CONFIG_PARAVIRT */
>
> -.macro TRACE_IRQS_IRETQ
> +.macro TRACE_IRQS_FLAGS flags:req
>  #ifdef CONFIG_TRACE_IRQFLAGS
> -       bt      $9, EFLAGS(%rsp)                /* interrupts off? */
> +       bt      $9, \flags              /* interrupts off? */
>         jnc     1f
>         TRACE_IRQS_ON
>  1:
>  #endif
>  .endm
>
> +.macro TRACE_IRQS_IRETQ
> +       TRACE_IRQS_FLAGS EFLAGS(%rsp)
> +.endm
> +
>  /*
>   * When dynamic function tracer is enabled it will add a breakpoint
>   * to all locations that it is about to modify, sync CPUs, update
> @@ -868,11 +872,13 @@ idtentry simd_coprocessor_error           do_simd
>  ENTRY(native_load_gs_index)
>         pushfq
>         DISABLE_INTERRUPTS(CLBR_ANY & ~CLBR_RDI)
> +       TRACE_IRQS_OFF
>         SWAPGS
>  .Lgs_change:
>         movl    %edi, %gs
>  2:     ALTERNATIVE "", "mfence", X86_BUG_SWAPGS_FENCE
>         SWAPGS
> +       TRACE_IRQS_FLAGS (%rsp)
>         popfq
>         ret
>  END(native_load_gs_index)
>
>
> Patches currently in stable-queue which might be from luto@kernel.org are
>
> queue-4.9/x86-entry-64-add-missing-irqflags-tracing-to-native_load_gs_index.patch

Re: Patch "x86/entry/64: Add missing irqflags tracing to native_load_gs_index()" has been added to the 4.9-stable tree

[Greg KH]

On Wed, Nov 29, 2017 at 08:23:10AM -0800, Andy Lutomirski wrote:
> If this hasn't been released yet, please hold off.  It's buggy.

Ah crap, I missed this and just released it.  Is there a fix for this in
Linus's tree?  How "buggy" is it?

thanks,

greg k-h

Re: Patch "x86/entry/64: Add missing irqflags tracing to native_load_gs_index()" has been added to the 4.9-stable tree

[Andy Lutomirski]

> On Nov 30, 2017, at 12:44 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
>> On Wed, Nov 29, 2017 at 08:23:10AM -0800, Andy Lutomirski wrote:
>> If this hasn't been released yet, please hold off.  It's buggy.
> 
> Ah crap, I missed this and just released it.  Is there a fix for this in
> Linus's tree?  How "buggy" is it?

It breaks suspend/resume on some configuration that I haven't quite figured out.  I think it only happens if (lockdep is on and some other unknown condition occurs) or (ftrace is actively tracing irq flags).  I have a fix, but I was waiting to for the OP to test it.

I'll send the fix Linusward later today without the Tested-by if I don't hear anything.

> 
> thanks,
> 
> greg k-h

Re: Patch "x86/entry/64: Add missing irqflags tracing to native_load_gs_index()" has been added to the 4.9-stable tree

[Andy Lutomirski]

On Thu, Nov 30, 2017 at 6:29 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>
>
>> On Nov 30, 2017, at 12:44 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>>> On Wed, Nov 29, 2017 at 08:23:10AM -0800, Andy Lutomirski wrote:
>>> If this hasn't been released yet, please hold off.  It's buggy.
>>
>> Ah crap, I missed this and just released it.  Is there a fix for this in
>> Linus's tree?  How "buggy" is it?
>
> It breaks suspend/resume on some configuration that I haven't quite figured out.  I think it only happens if (lockdep is on and some other unknown condition occurs) or (ftrace is actively tracing irq flags).  I have a fix, but I was waiting to for the OP to test it.
>
> I'll send the fix Linusward later today without the Tested-by if I don't hear anything.

I think the thing to do is to revert the patch from -stable.  The bug
it fixes is very minor, and the regression is that it made a
pre-existing bug in some nearly-undebuggable core resume code much
easier to hit.  I don't feel comfortable with a backport of the latter
fix until it has a good long soak in Linus' tree.

Re: Patch "x86/entry/64: Add missing irqflags tracing to native_load_gs_index()" has been added to the 4.9-stable tree

[Greg KH]

On Thu, Nov 30, 2017 at 10:50:22AM -0800, Andy Lutomirski wrote:
> On Thu, Nov 30, 2017 at 6:29 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> >
> >
> >> On Nov 30, 2017, at 12:44 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >>
> >>> On Wed, Nov 29, 2017 at 08:23:10AM -0800, Andy Lutomirski wrote:
> >>> If this hasn't been released yet, please hold off.  It's buggy.
> >>
> >> Ah crap, I missed this and just released it.  Is there a fix for this in
> >> Linus's tree?  How "buggy" is it?
> >
> > It breaks suspend/resume on some configuration that I haven't quite figured out.  I think it only happens if (lockdep is on and some other unknown condition occurs) or (ftrace is actively tracing irq flags).  I have a fix, but I was waiting to for the OP to test it.
> >
> > I'll send the fix Linusward later today without the Tested-by if I don't hear anything.
> 
> I think the thing to do is to revert the patch from -stable.  The bug
> it fixes is very minor, and the regression is that it made a
> pre-existing bug in some nearly-undebuggable core resume code much
> easier to hit.  I don't feel comfortable with a backport of the latter
> fix until it has a good long soak in Linus' tree.

Ok, now reverted.  Should I do the same thing for the patch in
4.14-stable as well?

thanks,

greg k-h

Re: Patch "x86/entry/64: Add missing irqflags tracing to native_load_gs_index()" has been added to the 4.9-stable tree

[Andy Lutomirski]

> On Dec 4, 2017, at 4:02 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
>> On Thu, Nov 30, 2017 at 10:50:22AM -0800, Andy Lutomirski wrote:
>>> On Thu, Nov 30, 2017 at 6:29 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>>> 
>>> 
>>>>> On Nov 30, 2017, at 12:44 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>>> 
>>>>> On Wed, Nov 29, 2017 at 08:23:10AM -0800, Andy Lutomirski wrote:
>>>>> If this hasn't been released yet, please hold off.  It's buggy.
>>>> 
>>>> Ah crap, I missed this and just released it.  Is there a fix for this in
>>>> Linus's tree?  How "buggy" is it?
>>> 
>>> It breaks suspend/resume on some configuration that I haven't quite figured out.  I think it only happens if (lockdep is on and some other unknown condition occurs) or (ftrace is actively tracing irq flags).  I have a fix, but I was waiting to for the OP to test it.
>>> 
>>> I'll send the fix Linusward later today without the Tested-by if I don't hear anything.
>> 
>> I think the thing to do is to revert the patch from -stable.  The bug
>> it fixes is very minor, and the regression is that it made a
>> pre-existing bug in some nearly-undebuggable core resume code much
>> easier to hit.  I don't feel comfortable with a backport of the latter
>> fix until it has a good long soak in Linus' tree.
> 
> Ok, now reverted.  Should I do the same thing for the patch in
> 4.14-stable as well?

Yes please.

> 
> thanks,
> 
> greg k-h

Re: Patch "x86/entry/64: Add missing irqflags tracing to native_load_gs_index()" has been added to the 4.9-stable tree

[Greg KH]

On Mon, Dec 04, 2017 at 05:52:55AM -0800, Andy Lutomirski wrote:
> 
> 
> > On Dec 4, 2017, at 4:02 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> >> On Thu, Nov 30, 2017 at 10:50:22AM -0800, Andy Lutomirski wrote:
> >>> On Thu, Nov 30, 2017 at 6:29 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> >>> 
> >>> 
> >>>>> On Nov 30, 2017, at 12:44 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >>>>> 
> >>>>> On Wed, Nov 29, 2017 at 08:23:10AM -0800, Andy Lutomirski wrote:
> >>>>> If this hasn't been released yet, please hold off.  It's buggy.
> >>>> 
> >>>> Ah crap, I missed this and just released it.  Is there a fix for this in
> >>>> Linus's tree?  How "buggy" is it?
> >>> 
> >>> It breaks suspend/resume on some configuration that I haven't quite figured out.  I think it only happens if (lockdep is on and some other unknown condition occurs) or (ftrace is actively tracing irq flags).  I have a fix, but I was waiting to for the OP to test it.
> >>> 
> >>> I'll send the fix Linusward later today without the Tested-by if I don't hear anything.
> >> 
> >> I think the thing to do is to revert the patch from -stable.  The bug
> >> it fixes is very minor, and the regression is that it made a
> >> pre-existing bug in some nearly-undebuggable core resume code much
> >> easier to hit.  I don't feel comfortable with a backport of the latter
> >> fix until it has a good long soak in Linus' tree.
> > 
> > Ok, now reverted.  Should I do the same thing for the patch in
> > 4.14-stable as well?
> 
> Yes please.

Great, now done.

greg k-h