[PATCH 1/5] kmsan: core: kmsan_in_runtime() should return true in NMI context

[PATCH 1/5] kmsan: core: kmsan_in_runtime() should return true in NMI context

[Alexander Potapenko]

Without that, every call to __msan_poison_alloca() in NMI may end up
allocating memory, which is NMI-unsafe.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Marco Elver <elver@google.com>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/lkml/20221025221755.3810809-1-glider@google.com/
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 mm/kmsan/kmsan.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/kmsan/kmsan.h b/mm/kmsan/kmsan.h
index 961eb658020aa..3cd2050a33e6a 100644
--- a/mm/kmsan/kmsan.h
+++ b/mm/kmsan/kmsan.h
@@ -125,6 +125,8 @@ static __always_inline bool kmsan_in_runtime(void)
 {
 	if ((hardirq_count() >> HARDIRQ_SHIFT) > 1)
 		return true;
+	if (in_nmi())
+		return true;
 	return kmsan_get_context()->kmsan_in_runtime;
 }
 
-- 
2.38.1.273.g43a17bfeac-goog

[PATCH 2/5] x86/uaccess: instrument copy_from_user_nmi()

[Alexander Potapenko]

Make sure usercopy hooks from linux/instrumented.h are invoked for
copy_from_user_nmi().
This fixes KMSAN false positives reported when dumping opcodes for a
stack trace.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: x86@kernel.org
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 arch/x86/lib/usercopy.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
index f1bb186171562..24b48af274173 100644
--- a/arch/x86/lib/usercopy.c
+++ b/arch/x86/lib/usercopy.c
@@ -6,6 +6,7 @@
 
 #include <linux/uaccess.h>
 #include <linux/export.h>
+#include <linux/instrumented.h>
 
 #include <asm/tlbflush.h>
 
@@ -44,7 +45,9 @@ copy_from_user_nmi(void *to, const void __user *from, unsigned long n)
 	 * called from other contexts.
 	 */
 	pagefault_disable();
+	instrument_copy_from_user_before(to, from, n);
 	ret = raw_copy_from_user(to, from, n);
+	instrument_copy_from_user_after(to, from, n, ret);
 	pagefault_enable();
 
 	return ret;
-- 
2.38.1.273.g43a17bfeac-goog

Re: [PATCH 2/5] x86/uaccess: instrument copy_from_user_nmi()

[Peter Zijlstra]

On Wed, Nov 02, 2022 at 12:06:08PM +0100, Alexander Potapenko wrote:
> Make sure usercopy hooks from linux/instrumented.h are invoked for
> copy_from_user_nmi().
> This fixes KMSAN false positives reported when dumping opcodes for a
> stack trace.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: x86@kernel.org
> Signed-off-by: Alexander Potapenko <glider@google.com>

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> ---
>  arch/x86/lib/usercopy.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
> index f1bb186171562..24b48af274173 100644
> --- a/arch/x86/lib/usercopy.c
> +++ b/arch/x86/lib/usercopy.c
> @@ -6,6 +6,7 @@
>  
>  #include <linux/uaccess.h>
>  #include <linux/export.h>
> +#include <linux/instrumented.h>
>  
>  #include <asm/tlbflush.h>
>  
> @@ -44,7 +45,9 @@ copy_from_user_nmi(void *to, const void __user *from, unsigned long n)
>  	 * called from other contexts.
>  	 */
>  	pagefault_disable();
> +	instrument_copy_from_user_before(to, from, n);
>  	ret = raw_copy_from_user(to, from, n);
> +	instrument_copy_from_user_after(to, from, n, ret);
>  	pagefault_enable();
>  
>  	return ret;
> -- 
> 2.38.1.273.g43a17bfeac-goog
> 

[PATCH 3/5] Kconfig.debug: ensure early check for KMSAN in CONFIG_KMSAN_WARN

[Alexander Potapenko]

As pointed out by Masahiro Yamada, Kconfig picks up the first default
entry which has true 'if' condition. Hence, the previously added check
for KMSAN was never used, because it followed the checks for 64BIT and
!64BIT.

Put KMSAN check before others to ensure it is always applied.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: linux-kbuild@vger.kernel.org
Link: https://github.com/google/kmsan/issues/89
Link: https://lore.kernel.org/linux-mm/20221024212144.2852069-3-glider@google.com/
Fixes: 921757bc9b61 ("Kconfig.debug: disable CONFIG_FRAME_WARN for KMSAN by default")
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 lib/Kconfig.debug | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 29280072dc0e4..b4a0988a7ffd2 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -395,12 +395,12 @@ endif # DEBUG_INFO
 config FRAME_WARN
 	int "Warn for stack frames larger than"
 	range 0 8192
+	default 0 if KMSAN
 	default 2048 if GCC_PLUGIN_LATENT_ENTROPY
 	default 2048 if PARISC
 	default 1536 if (!64BIT && XTENSA)
 	default 1024 if !64BIT
 	default 2048 if 64BIT
-	default 0 if KMSAN
 	help
 	  Tell the compiler to warn at build time for stack frames larger than this.
 	  Setting this too low will cause a lot of warnings.
-- 
2.38.1.273.g43a17bfeac-goog

[PATCH 4/5] kmsan: make sure PREEMPT_RT is off

[Alexander Potapenko]

As pointed out by Peter Zijlstra, __msan_poison_alloca() does not play
well with IRQ code when PREEMPT_RT is on, because in that mode even
GFP_ATOMIC allocations cannot be performed.

Fixing this would require making stackdepot completely lockless, which
is quite challenging and may be excessive for the time being.

Instead, make sure KMSAN is incompatible with PREEMPT_RT, like other
debug configs are.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Marco Elver <elver@google.com>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/lkml/20221025221755.3810809-1-glider@google.com/
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 lib/Kconfig.kmsan | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/Kconfig.kmsan b/lib/Kconfig.kmsan
index b2489dd6503fa..ef2c8f256c57d 100644
--- a/lib/Kconfig.kmsan
+++ b/lib/Kconfig.kmsan
@@ -12,6 +12,7 @@ config KMSAN
 	bool "KMSAN: detector of uninitialized values use"
 	depends on HAVE_ARCH_KMSAN && HAVE_KMSAN_COMPILER
 	depends on SLUB && DEBUG_KERNEL && !KASAN && !KCSAN
+	depends on !PREEMPT_RT
 	select STACKDEPOT
 	select STACKDEPOT_ALWAYS_INIT
 	help
-- 
2.38.1.273.g43a17bfeac-goog

Re: [PATCH 4/5] kmsan: make sure PREEMPT_RT is off

[Peter Zijlstra]

On Wed, Nov 02, 2022 at 12:06:10PM +0100, Alexander Potapenko wrote:
> As pointed out by Peter Zijlstra, __msan_poison_alloca() does not play
> well with IRQ code when PREEMPT_RT is on, because in that mode even
> GFP_ATOMIC allocations cannot be performed.
> 
> Fixing this would require making stackdepot completely lockless, which
> is quite challenging and may be excessive for the time being.
> 
> Instead, make sure KMSAN is incompatible with PREEMPT_RT, like other
> debug configs are.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Marco Elver <elver@google.com>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Link: https://lore.kernel.org/lkml/20221025221755.3810809-1-glider@google.com/
> Signed-off-by: Alexander Potapenko <glider@google.com>

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> ---
>  lib/Kconfig.kmsan | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/Kconfig.kmsan b/lib/Kconfig.kmsan
> index b2489dd6503fa..ef2c8f256c57d 100644
> --- a/lib/Kconfig.kmsan
> +++ b/lib/Kconfig.kmsan
> @@ -12,6 +12,7 @@ config KMSAN
>  	bool "KMSAN: detector of uninitialized values use"
>  	depends on HAVE_ARCH_KMSAN && HAVE_KMSAN_COMPILER
>  	depends on SLUB && DEBUG_KERNEL && !KASAN && !KCSAN
> +	depends on !PREEMPT_RT
>  	select STACKDEPOT
>  	select STACKDEPOT_ALWAYS_INIT
>  	help
> -- 
> 2.38.1.273.g43a17bfeac-goog
> 

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

[Alexander Potapenko]

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(+)

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

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

[Peter Zijlstra]

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.

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

[Alexander Potapenko]

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.


-- 
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

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

[Peter Zijlstra]

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.

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

[Peter Zijlstra]

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() ?

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

[Alexander Potapenko]

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.

-- 
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

Re: [PATCH 1/5] kmsan: core: kmsan_in_runtime() should return true in NMI context

[Peter Zijlstra]

On Wed, Nov 02, 2022 at 12:06:07PM +0100, Alexander Potapenko wrote:
> Without that, every call to __msan_poison_alloca() in NMI may end up
> allocating memory, which is NMI-unsafe.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Marco Elver <elver@google.com>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Link: https://lore.kernel.org/lkml/20221025221755.3810809-1-glider@google.com/
> Signed-off-by: Alexander Potapenko <glider@google.com>

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> ---
>  mm/kmsan/kmsan.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/kmsan/kmsan.h b/mm/kmsan/kmsan.h
> index 961eb658020aa..3cd2050a33e6a 100644
> --- a/mm/kmsan/kmsan.h
> +++ b/mm/kmsan/kmsan.h
> @@ -125,6 +125,8 @@ static __always_inline bool kmsan_in_runtime(void)
>  {
>  	if ((hardirq_count() >> HARDIRQ_SHIFT) > 1)
>  		return true;
> +	if (in_nmi())
> +		return true;
>  	return kmsan_get_context()->kmsan_in_runtime;
>  }
>  
> -- 
> 2.38.1.273.g43a17bfeac-goog
>