diff mbox series

[5/5] x86/traps: avoid KMSAN bugs originating from handle_bug()

Message ID 20221102110611.1085175-5-glider@google.com
State New
Headers
Series [1/5] kmsan: core: kmsan_in_runtime() should return true in NMI context |

Commit Message

Alexander Potapenko Nov. 2, 2022, 11:06 a.m. UTC 
  There is a case in exc_invalid_op handler that is executed outside the
irqentry_enter()/irqentry_exit() region when an UD2 instruction is used
to encode a call to __warn().

In that case the `struct pt_regs` passed to the interrupt handler is
never unpoisoned by KMSAN (this is normally done in irqentry_enter()),
which leads to false positives inside handle_bug().

Use kmsan_unpoison_entry_regs() to explicitly unpoison those registers
before using them.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: x86@kernel.org
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 arch/x86/kernel/traps.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Peter Zijlstra Nov. 2, 2022, 12:50 p.m. UTC | #1 
On Wed, Nov 02, 2022 at 12:06:11PM +0100, Alexander Potapenko wrote:
> There is a case in exc_invalid_op handler that is executed outside the
> irqentry_enter()/irqentry_exit() region when an UD2 instruction is used
> to encode a call to __warn().
> 
> In that case the `struct pt_regs` passed to the interrupt handler is
> never unpoisoned by KMSAN (this is normally done in irqentry_enter()),
> which leads to false positives inside handle_bug().
> 
> Use kmsan_unpoison_entry_regs() to explicitly unpoison those registers
> before using them.

As does poke_int3_handler(); does that need fixing up too? OTOH look
*very very* carefully at the contraints there.
Alexander Potapenko Nov. 2, 2022, 1:37 p.m. UTC | #2 
On Wed, Nov 2, 2022 at 1:51 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Nov 02, 2022 at 12:06:11PM +0100, Alexander Potapenko wrote:
> > There is a case in exc_invalid_op handler that is executed outside the
> > irqentry_enter()/irqentry_exit() region when an UD2 instruction is used
> > to encode a call to __warn().
> >
> > In that case the `struct pt_regs` passed to the interrupt handler is
> > never unpoisoned by KMSAN (this is normally done in irqentry_enter()),
> > which leads to false positives inside handle_bug().
> >
> > Use kmsan_unpoison_entry_regs() to explicitly unpoison those registers
> > before using them.
>
> As does poke_int3_handler(); does that need fixing up too? OTOH look
> *very very* carefully at the contraints there.

Fortunately poke_int3_handler() is a noinstr function, so KMSAN
doesn't add any checks to it.
It also does not pass regs to other instrumented functions, at least
for now, so we're good.
Peter Zijlstra Nov. 3, 2022, 11:18 a.m. UTC | #3 
On Wed, Nov 02, 2022 at 12:06:11PM +0100, Alexander Potapenko wrote:

> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 178015a820f08..d3fdec706f1d2 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -15,6 +15,7 @@
>  #include <linux/context_tracking.h>
>  #include <linux/interrupt.h>
>  #include <linux/kallsyms.h>
> +#include <linux/kmsan.h>
>  #include <linux/spinlock.h>
>  #include <linux/kprobes.h>
>  #include <linux/uaccess.h>
> @@ -301,6 +302,12 @@ static noinstr bool handle_bug(struct pt_regs *regs)
>  {
>  	bool handled = false;
>  
> +	/*
> +	 * Normally @regs are unpoisoned by irqentry_enter(), but handle_bug()
> +	 * is a rare case that uses @regs without passing them to
> +	 * irqentry_enter().
> +	 */
> +	kmsan_unpoison_entry_regs(regs);
>  	if (!is_valid_bugaddr(regs->ip))
>  		return handled;
>  

Should we place this kmsan_unpoison_entry_regs() after the
instrumentation_begin() ?
Peter Zijlstra Nov. 3, 2022, 11:18 a.m. UTC | #4 
On Wed, Nov 02, 2022 at 02:37:19PM +0100, Alexander Potapenko wrote:
> On Wed, Nov 2, 2022 at 1:51 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Wed, Nov 02, 2022 at 12:06:11PM +0100, Alexander Potapenko wrote:
> > > There is a case in exc_invalid_op handler that is executed outside the
> > > irqentry_enter()/irqentry_exit() region when an UD2 instruction is used
> > > to encode a call to __warn().
> > >
> > > In that case the `struct pt_regs` passed to the interrupt handler is
> > > never unpoisoned by KMSAN (this is normally done in irqentry_enter()),
> > > which leads to false positives inside handle_bug().
> > >
> > > Use kmsan_unpoison_entry_regs() to explicitly unpoison those registers
> > > before using them.
> >
> > As does poke_int3_handler(); does that need fixing up too? OTOH look
> > *very very* carefully at the contraints there.
> 
> Fortunately poke_int3_handler() is a noinstr function, so KMSAN
> doesn't add any checks to it.
> It also does not pass regs to other instrumented functions, at least
> for now, so we're good.

Ah indeed; because it is fully noinstr, nothing will trigger the lack of
annotation.
Alexander Potapenko Nov. 3, 2022, 1:37 p.m. UTC | #5 
On Thu, Nov 3, 2022 at 12:18 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Nov 02, 2022 at 12:06:11PM +0100, Alexander Potapenko wrote:
>
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index 178015a820f08..d3fdec706f1d2 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/context_tracking.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/kallsyms.h>
> > +#include <linux/kmsan.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/kprobes.h>
> >  #include <linux/uaccess.h>
> > @@ -301,6 +302,12 @@ static noinstr bool handle_bug(struct pt_regs *regs)
> >  {
> >       bool handled = false;
> >
> > +     /*
> > +      * Normally @regs are unpoisoned by irqentry_enter(), but handle_bug()
> > +      * is a rare case that uses @regs without passing them to
> > +      * irqentry_enter().
> > +      */
> > +     kmsan_unpoison_entry_regs(regs);
> >       if (!is_valid_bugaddr(regs->ip))
> >               return handled;
> >
>
> Should we place this kmsan_unpoison_entry_regs() after the
> instrumentation_begin() ?

Agreed, let me send an update.
diff mbox series

Patch

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 178015a820f08..d3fdec706f1d2 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -15,6 +15,7 @@ 
 #include <linux/context_tracking.h>
 #include <linux/interrupt.h>
 #include <linux/kallsyms.h>
+#include <linux/kmsan.h>
 #include <linux/spinlock.h>
 #include <linux/kprobes.h>
 #include <linux/uaccess.h>
@@ -301,6 +302,12 @@  static noinstr bool handle_bug(struct pt_regs *regs)
 {
 	bool handled = false;
 
+	/*
+	 * Normally @regs are unpoisoned by irqentry_enter(), but handle_bug()
+	 * is a rare case that uses @regs without passing them to
+	 * irqentry_enter().
+	 */
+	kmsan_unpoison_entry_regs(regs);
 	if (!is_valid_bugaddr(regs->ip))
 		return handled;