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

[Dmitry Vyukov]

On Mon, 6 Mar 2023 at 12:13, Alexander Potapenko <glider@xxxxxxxxxx> 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@xxxxxxxxx>
> Cc: Marco Elver <elver@xxxxxxxxxx>
> Suggested-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Signed-off-by: Alexander Potapenko <glider@xxxxxxxxxx>

Add:
Reported-by: syzbot <syzkaller@xxxxxxxxxxxxxxxx>

Otherwise:
Reviewed-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>


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