Re: [PATCH 1/2] lib/stackdepot: kmsan: mark API outputs as initialized

Dmitry Vyukov
2023-03-06T11:37:37Z

On Mon, 6 Mar 2023 at 12:13, Alexander Potapenko <glider@google.com> wrote:
>
> KMSAN does not instrument stackdepot and may treat memory allocated by
> it as uninitialized. This is not a problem for KMSAN itself, because its
> functions calling stackdepot API are also not instrumented.
> But other kernel features (e.g. netdev tracker) may access stack depot
> from instrumented code, which will lead to false positives, unless we
> explicitly mark stackdepot outputs as initialized.
>
> Cc: Andrey Konovalov <andreyknvl@gmail.com>
> Cc: Marco Elver <elver@google.com>
> Suggested-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Alexander Potapenko <glider@google.com>

Add:
Reported-by: syzbot <syzkaller@googlegroups.com>

Otherwise:
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>

> ---
> lib/stackdepot.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 036da8e295d19..2f5aa851834eb 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -17,6 +17,7 @@
> #include <linux/gfp.h>
> #include <linux/jhash.h>
> #include <linux/kernel.h>
> +#include <linux/kmsan.h>
> #include <linux/mm.h>
> #include <linux/mutex.h>
> #include <linux/percpu.h>
> @@ -306,6 +307,11 @@ depot_alloc_stack(unsigned long *entries, int size, u32 hash, void **prealloc)
> stack->handle.extra = 0;
> memcpy(stack->entries, entries, flex_array_size(stack, entries, size));
> pool_offset += required_size;
> + /*
> + * Let KMSAN know the stored stack record is initialized. This shall
> + * prevent false positive reports if instrumented code accesses it.
> + */
> + kmsan_unpoison_memory(stack, required_size);
>
> return stack;
> }
> @@ -465,6 +471,12 @@ unsigned int stack_depot_fetch(depot_stack_handle_t handle,
> struct stack_record *stack;
>
> *entries = NULL;
> + /*
> + * Let KMSAN know *entries is initialized. This shall prevent false
> + * positive reports if instrumented code accesses it.
> + */
> + kmsan_unpoison_memory(entries, sizeof(*entries));
> +
> if (!handle)
> return 0;
>
> --
> 2.40.0.rc0.216.gc4246ad0f0-goog
>