diff mbox series

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

Message ID 20230306111322.205724-1-glider@google.com
State New
Headers
Series [1/2] lib/stackdepot: kmsan: mark API outputs as initialized |

Commit Message

Alexander Potapenko March 6, 2023, 11:13 a.m. UTC 
  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>
---
 lib/stackdepot.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Dmitry Vyukov March 6, 2023, 11:37 a.m. UTC | #1 
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
>
Andrey Konovalov March 10, 2023, 11:50 p.m. UTC | #2 
On Mon, Mar 6, 2023 at 12:13 PM 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>
> ---
>  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
>

Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
diff mbox series

Patch

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;