x86: suppress KMSAN reports in arch_within_stack_frames()

  arch_within_stack_frames() performs stack walking and may confuse
KMSAN by stepping on stale shadow values. To prevent false positive
reports, disable KMSAN checks in this function.

This fixes KMSAN's interoperability with CONFIG_HARDENED_USERCOPY.

Link: https://github.com/google/kmsan/issues/89
Link: https://lore.kernel.org/lkml/Y3b9AAEKp2Vr3e6O@sol.localdomain/
Cc: Eric Biggers <ebiggers@kernel.org>
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 arch/x86/include/asm/thread_info.h | 5 +++++
 1 file changed, 5 insertions(+)
  

On Fri, Nov 18, 2022 at 06:23:05PM +0100, Alexander Potapenko wrote:
> arch_within_stack_frames() performs stack walking and may confuse
> KMSAN by stepping on stale shadow values. To prevent false positive
> reports, disable KMSAN checks in this function.
> 
> This fixes KMSAN's interoperability with CONFIG_HARDENED_USERCOPY.
> 
> Link: https://github.com/google/kmsan/issues/89
> Link: https://lore.kernel.org/lkml/Y3b9AAEKp2Vr3e6O@sol.localdomain/
> Cc: Eric Biggers <ebiggers@kernel.org>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
>  arch/x86/include/asm/thread_info.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index f0cb881c1d690..f1cccba52eb97 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -163,7 +163,12 @@ struct thread_info {
>   *	GOOD_FRAME	if within a frame
>   *	BAD_STACK	if placed across a frame boundary (or outside stack)
>   *	NOT_STACK	unable to determine (no frame pointers, etc)
> + *
> + * This function reads pointers from the stack and dereferences them. The
> + * pointers may not have their KMSAN shadow set up properly, which may result
> + * in false positive reports. Disable instrumentation to avoid those.
>   */
> +__no_kmsan_checks
>  static inline int arch_within_stack_frames(const void * const stack,
>  					   const void * const stackend,
>  					   const void *obj, unsigned long len)

Seems OK; but now I'm confused as to the exact distinction between
__no_sanitize_memory and __no_kmsan_checks.

The comments there about seem to suggest __no_sanitize_memory ensures no
instrumentation at all, and __no_kmsan_checks some instrumentation but
doesn't actually check anything -- so what's left then?
  

On Mon, Nov 21, 2022 at 11:23 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, Nov 18, 2022 at 06:23:05PM +0100, Alexander Potapenko wrote:
> > arch_within_stack_frames() performs stack walking and may confuse
> > KMSAN by stepping on stale shadow values. To prevent false positive
> > reports, disable KMSAN checks in this function.
> >
> > This fixes KMSAN's interoperability with CONFIG_HARDENED_USERCOPY.
> >
> > Link: https://github.com/google/kmsan/issues/89
> > Link: https://lore.kernel.org/lkml/Y3b9AAEKp2Vr3e6O@sol.localdomain/
> > Cc: Eric Biggers <ebiggers@kernel.org>
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > ---
> >  arch/x86/include/asm/thread_info.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> > index f0cb881c1d690..f1cccba52eb97 100644
> > --- a/arch/x86/include/asm/thread_info.h
> > +++ b/arch/x86/include/asm/thread_info.h
> > @@ -163,7 +163,12 @@ struct thread_info {
> >   *   GOOD_FRAME      if within a frame
> >   *   BAD_STACK       if placed across a frame boundary (or outside stack)
> >   *   NOT_STACK       unable to determine (no frame pointers, etc)
> > + *
> > + * This function reads pointers from the stack and dereferences them. The
> > + * pointers may not have their KMSAN shadow set up properly, which may result
> > + * in false positive reports. Disable instrumentation to avoid those.
> >   */
> > +__no_kmsan_checks
> >  static inline int arch_within_stack_frames(const void * const stack,
> >                                          const void * const stackend,
> >                                          const void *obj, unsigned long len)
>
> Seems OK; but now I'm confused as to the exact distinction between
> __no_sanitize_memory and __no_kmsan_checks.
>
> The comments there about seem to suggest __no_sanitize_memory ensures no
> instrumentation at all, and __no_kmsan_checks some instrumentation but
> doesn't actually check anything -- so what's left then?

__no_sanitize_memory prohibits all instrumentation whatsoever, whereas
__no_kmsan_checks adds instrumentation that suppresses potential false
positives around this function.

Quoting include/linux/compiler-clang.h:

/*
 * The __no_kmsan_checks attribute ensures that a function does not produce
 * false positive reports by:
 *  - initializing all local variables and memory stores in this function;
 *  - skipping all shadow checks;
 *  - passing initialized arguments to this function's callees.
 */

Does this answer your question?
  

On Mon, Nov 21, 2022 at 11:28:39AM +0100, Alexander Potapenko wrote:

> > > +__no_kmsan_checks
> > >  static inline int arch_within_stack_frames(const void * const stack,
> > >                                          const void * const stackend,
> > >                                          const void *obj, unsigned long len)
> >
> > Seems OK; but now I'm confused as to the exact distinction between
> > __no_sanitize_memory and __no_kmsan_checks.
> >
> > The comments there about seem to suggest __no_sanitize_memory ensures no
> > instrumentation at all, and __no_kmsan_checks some instrumentation but
> > doesn't actually check anything -- so what's left then?
> 
> __no_sanitize_memory prohibits all instrumentation whatsoever, whereas
> __no_kmsan_checks adds instrumentation that suppresses potential false
> positives around this function.
> 
> Quoting include/linux/compiler-clang.h:
> 
> /*
>  * The __no_kmsan_checks attribute ensures that a function does not produce
>  * false positive reports by:
>  *  - initializing all local variables and memory stores in this function;
>  *  - skipping all shadow checks;
>  *  - passing initialized arguments to this function's callees.
>  */
> 
> Does this answer your question?

So I read that comment; and it didn't click. So you're explicitly
initializing variables/arguments and explicitly not checking shadow
state vs, not doing explicit initialization and checking shadow state?

That is, it doesn't do the normal checks and adds explicit
initialization to avoid triggering discontent in surrounding functions?
  

On Mon, Nov 21, 2022 at 12:38 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Nov 21, 2022 at 11:28:39AM +0100, Alexander Potapenko wrote:
>
> > > > +__no_kmsan_checks
> > > >  static inline int arch_within_stack_frames(const void * const stack,
> > > >                                          const void * const stackend,
> > > >                                          const void *obj, unsigned long len)
> > >
> > > Seems OK; but now I'm confused as to the exact distinction between
> > > __no_sanitize_memory and __no_kmsan_checks.
> > >
> > > The comments there about seem to suggest __no_sanitize_memory ensures no
> > > instrumentation at all, and __no_kmsan_checks some instrumentation but
> > > doesn't actually check anything -- so what's left then?
> >
> > __no_sanitize_memory prohibits all instrumentation whatsoever, whereas
> > __no_kmsan_checks adds instrumentation that suppresses potential false
> > positives around this function.
> >
> > Quoting include/linux/compiler-clang.h:
> >
> > /*
> >  * The __no_kmsan_checks attribute ensures that a function does not produce
> >  * false positive reports by:
> >  *  - initializing all local variables and memory stores in this function;
> >  *  - skipping all shadow checks;
> >  *  - passing initialized arguments to this function's callees.
> >  */
> >
> > Does this answer your question?
>
> So I read that comment; and it didn't click. So you're explicitly
> initializing variables/arguments and explicitly not checking shadow
> state vs, not doing explicit initialization and checking shadow state?
>
> That is, it doesn't do the normal checks and adds explicit
> initialization to avoid triggering discontent in surrounding functions?
>
Correct

In other words, for normal instrumentation:
 - locals are explicitly marked as uninitialized;
 - shadow values are calculated for arithmetic operations based on their inputs;
 - shadow values are checked for branches, pointer dereferences, and
before passing them as function arguments;
 - memory stores update shadow for affected variables.

For __no_kmsan_checks:
 - locals are explicitly marked as initialized;
 - no instrumentation is added for arithmetic operations, branches,
pointer dereferences;
 - all function arguments are marked as initialized;
 - stores always mark memory as initialized.

For __no_sanitize_memory:
 - no instrumentation for locals (they may end up being initialized or
uninitialized - doesn't matter, because their shadow values are never
used);
 - no instrumentation for arithmetic operations, branches, pointer dereferences;
 - no instrumentation for function calls (an instrumented function
will receive garbage shadow values from a non-instrumented one);
 - no instrumentation for stores (initialization done in these
functions is invisible).

In all the cases explicit calls to
kmsan_poison_memory()/kmsan_unpoison_memory()/kmsan_check_memory()
behave the same way and can be used to tell the tool what is going on
in the absence of instrumentation.
  

On Mon, Nov 21, 2022 at 03:27:49PM +0100, Alexander Potapenko wrote:

> In other words, for normal instrumentation:
>  - locals are explicitly marked as uninitialized;
>  - shadow values are calculated for arithmetic operations based on their inputs;
>  - shadow values are checked for branches, pointer dereferences, and
> before passing them as function arguments;
>  - memory stores update shadow for affected variables.
> 
> For __no_kmsan_checks:
>  - locals are explicitly marked as initialized;
>  - no instrumentation is added for arithmetic operations, branches,
> pointer dereferences;
>  - all function arguments are marked as initialized;
>  - stores always mark memory as initialized.
> 
> For __no_sanitize_memory:
>  - no instrumentation for locals (they may end up being initialized or
> uninitialized - doesn't matter, because their shadow values are never
> used);
>  - no instrumentation for arithmetic operations, branches, pointer dereferences;
>  - no instrumentation for function calls (an instrumented function
> will receive garbage shadow values from a non-instrumented one);
>  - no instrumentation for stores (initialization done in these
> functions is invisible).

Thanks! That is a great summary.
  

On Fri, Nov 18, 2022 at 06:23:05PM +0100, Alexander Potapenko wrote:
> arch_within_stack_frames() performs stack walking and may confuse
> KMSAN by stepping on stale shadow values. To prevent false positive
> reports, disable KMSAN checks in this function.
> 
> This fixes KMSAN's interoperability with CONFIG_HARDENED_USERCOPY.
> 
> Link: https://github.com/google/kmsan/issues/89
> Link: https://lore.kernel.org/lkml/Y3b9AAEKp2Vr3e6O@sol.localdomain/
> Cc: Eric Biggers <ebiggers@kernel.org>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
>  arch/x86/include/asm/thread_info.h | 5 +++++
>  1 file changed, 5 insertions(+)

Tested-by: Eric Biggers <ebiggers@google.com>

- Eric
  

On Tue, Nov 29, 2022 at 09:40:45PM -0800, Eric Biggers wrote:
> On Fri, Nov 18, 2022 at 06:23:05PM +0100, Alexander Potapenko wrote:
> > arch_within_stack_frames() performs stack walking and may confuse
> > KMSAN by stepping on stale shadow values. To prevent false positive
> > reports, disable KMSAN checks in this function.
> > 
> > This fixes KMSAN's interoperability with CONFIG_HARDENED_USERCOPY.
> > 
> > Link: https://github.com/google/kmsan/issues/89
> > Link: https://lore.kernel.org/lkml/Y3b9AAEKp2Vr3e6O@sol.localdomain/
> > Cc: Eric Biggers <ebiggers@kernel.org>
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > ---
> >  arch/x86/include/asm/thread_info.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> 
> Tested-by: Eric Biggers <ebiggers@google.com>
> 

Can this patch be applied to the x86 tree, please?

- Eric
  

On Wed, Jan 11, 2023 at 09:40:47PM -0800, Eric Biggers wrote:
> On Tue, Nov 29, 2022 at 09:40:45PM -0800, Eric Biggers wrote:
> > On Fri, Nov 18, 2022 at 06:23:05PM +0100, Alexander Potapenko wrote:
> > > arch_within_stack_frames() performs stack walking and may confuse
> > > KMSAN by stepping on stale shadow values. To prevent false positive
> > > reports, disable KMSAN checks in this function.
> > > 
> > > This fixes KMSAN's interoperability with CONFIG_HARDENED_USERCOPY.
> > > 
> > > Link: https://github.com/google/kmsan/issues/89
> > > Link: https://lore.kernel.org/lkml/Y3b9AAEKp2Vr3e6O@sol.localdomain/
> > > Cc: Eric Biggers <ebiggers@kernel.org>
> > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > ---
> > >  arch/x86/include/asm/thread_info.h | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > 
> > Tested-by: Eric Biggers <ebiggers@google.com>
> > 
> 
> Can this patch be applied to the x86 tree, please?
> 
> - Eric

Ping.