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 <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: [email protected]
Signed-off-by: Alexander Potapenko <[email protected]>
---
arch/x86/kernel/traps.c | 7 +++++++
1 file changed, 7 insertions(+)
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;
--
2.38.1.273.g43a17bfeac-goog
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 <[email protected]> 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.
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
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 <[email protected]> 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 <[email protected]> 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.
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg