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/[email protected]/
Cc: Eric Biggers <[email protected]>
Signed-off-by: Alexander Potapenko <[email protected]>
---
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)
--
2.38.1.584.g0f3c55d4c2-goog
On Mon, Nov 21, 2022 at 11:23 AM Peter Zijlstra <[email protected]> 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/[email protected]/
> > Cc: Eric Biggers <[email protected]>
> > Signed-off-by: Alexander Potapenko <[email protected]>
> > ---
> > 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?
--
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 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/[email protected]/
> Cc: Eric Biggers <[email protected]>
> Signed-off-by: Alexander Potapenko <[email protected]>
> ---
> 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: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 <[email protected]> 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.
--
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 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/[email protected]/
> Cc: Eric Biggers <[email protected]>
> Signed-off-by: Alexander Potapenko <[email protected]>
> ---
> arch/x86/include/asm/thread_info.h | 5 +++++
> 1 file changed, 5 insertions(+)
Tested-by: Eric Biggers <[email protected]>
- 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/[email protected]/
> > Cc: Eric Biggers <[email protected]>
> > Signed-off-by: Alexander Potapenko <[email protected]>
> > ---
> > arch/x86/include/asm/thread_info.h | 5 +++++
> > 1 file changed, 5 insertions(+)
>
> Tested-by: Eric Biggers <[email protected]>
>
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/[email protected]/
> > > Cc: Eric Biggers <[email protected]>
> > > Signed-off-by: Alexander Potapenko <[email protected]>
> > > ---
> > > arch/x86/include/asm/thread_info.h | 5 +++++
> > > 1 file changed, 5 insertions(+)
> >
> > Tested-by: Eric Biggers <[email protected]>
> >
>
> Can this patch be applied to the x86 tree, please?
>
> - Eric
Ping.