| Message ID | 20221102110611.1085175-5-glider@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/5] kmsan: core: kmsan_in_runtime() should return true in NMI context | expand |
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.
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.
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() ?
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.
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 --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;
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(+)