Message-ID: <CAG_fn=VuaiTB11bJraxQjoVxp=0ML7Zoth1CYjczgUof3Rhqmw@mail.gmail.com>
Date: Thu, 8 May 2025 10:19:06 +0200
From: Alexander Potapenko <glider@...gle.com>
To: Marco Elver <elver@...gle.com>
Cc: dvyukov@...gle.com, bvanassche@....org, kent.overstreet@...ux.dev, 
	iii@...ux.ibm.com, akpm@...ux-foundation.org, linux-kernel@...r.kernel.org, 
	kasan-dev@...glegroups.com
Subject: Re: [PATCH 2/5] kmsan: fix usage of kmsan_enter_runtime() in kmsan_vmap_pages_range_noflush()

On Wed, May 7, 2025 at 6:09 PM Marco Elver <elver@...gle.com> wrote:
>
> On Wed, 7 May 2025 at 18:00, Alexander Potapenko <glider@...gle.com> wrote:
> >
> > Only enter the runtime to call __vmap_pages_range_noflush(), so that error
> > handling does not skip kmsan_leave_runtime().
> >
> > This bug was spotted by CONFIG_WARN_CAPABILITY_ANALYSIS=y
>
> Might be worth pointing out this is not yet upstream:
> https://lore.kernel.org/all/20250304092417.2873893-1-elver@google.com/

Thanks! I'll update the description (here and in the other patch) and
post v2 later today.

> Also, for future reference, feel free to dump the diff here that added
> the annotations that helped you find the missing kmsan*runtime()
> calls. I'm sure it'd be of interest to others. At one point we may
> upstream those annotations, too, but we'll need Capability Analysis
> upstream first (which is blocked by some Clang improvements that were
> requested).

The diff is below. I added a __no_matter() macro which isn't strictly
necessary (maybe we can remove it altogether), but I thought it'll be
more descriptive.

Author: Alexander Potapenko <glider@...gle.com>
Date:   Thu Apr 3 15:44:38 2025 +0200

    DO-NOT-SUBMIT: kmsan: enable capability analysis

    Add support for the new capability analysis framework to KMSAN.
    Use the KMSAN_RUNTIME capability token to ensure correctness of
    kmsan_enter_runtime()/kmsan_leave_runtime() usage.

    Cc: Marco Elver <elver@...gle.com>
    Cc: Bart Van Assche <bvanassche@....org>
    Cc: Kent Overstreet <kent.overstreet@...ux.dev>
    Signed-off-by: Alexander Potapenko <glider@...gle.com>

diff --git a/mm/kmsan/Makefile b/mm/kmsan/Makefile
index 91cfdde642d16..94591d612384c 100644
--- a/mm/kmsan/Makefile
+++ b/mm/kmsan/Makefile
@@ -8,6 +8,7 @@ obj-y := core.o instrumentation.o init.o hooks.o
report.o shadow.o
 KMSAN_SANITIZE := n
 KCOV_INSTRUMENT := n
 UBSAN_SANITIZE := n
+CAPABILITY_ANALYSIS := y

 # Disable instrumentation of KMSAN runtime with other tools.
 CC_FLAGS_KMSAN_RUNTIME := -fno-stack-protector
diff --git a/mm/kmsan/kmsan.h b/mm/kmsan/kmsan.h
index bc3d1810f352c..441c9dd39fe2a 100644
--- a/mm/kmsan/kmsan.h
+++ b/mm/kmsan/kmsan.h
@@ -35,6 +35,9 @@
 #define KMSAN_META_SHADOW (false)
 #define KMSAN_META_ORIGIN (true)

+token_capability(KMSAN_RUNTIME);
+#define __no_matter(X)
+
 /*
  * A pair of metadata pointers to be returned by the instrumentation functions.
  */
@@ -74,7 +77,7 @@ void kmsan_print_origin(depot_stack_handle_t origin);
  */
 void kmsan_report(depot_stack_handle_t origin, void *address, int size,
                  int off_first, int off_last, const void __user *user_addr,
-                 enum kmsan_bug_reason reason);
+                 enum kmsan_bug_reason reason) __must_not_hold(KMSAN_RUNTIME);

 DECLARE_PER_CPU(struct kmsan_ctx, kmsan_percpu_ctx);

@@ -107,6 +110,7 @@ static __always_inline bool kmsan_in_runtime(void)
 }

 static __always_inline void kmsan_enter_runtime(void)
+       __acquires(KMSAN_RUNTIME) __no_capability_analysis
 {
        struct kmsan_ctx *ctx;

@@ -115,6 +119,7 @@ static __always_inline void kmsan_enter_runtime(void)
 }

 static __always_inline void kmsan_leave_runtime(void)
+       __releases(KMSAN_RUNTIME) __no_capability_analysis
 {
        struct kmsan_ctx *ctx = kmsan_get_context();

@@ -122,7 +127,8 @@ static __always_inline void kmsan_leave_runtime(void)
 }

 depot_stack_handle_t kmsan_save_stack_with_flags(gfp_t flags,
-                                                unsigned int extra_bits);
+                                                unsigned int extra_bits)
+       __must_hold(KMSAN_RUNTIME);

 /*
  * Pack and unpack the origin chain depth and UAF flag to/from the extra bits
@@ -151,19 +157,26 @@ static __always_inline unsigned int
kmsan_depth_from_eb(unsigned int extra_bits)
  * kmsan_internal_ functions are supposed to be very simple and not require the
  * kmsan_in_runtime() checks.
  */
-void kmsan_internal_memmove_metadata(void *dst, void *src, size_t n);
+void kmsan_internal_memmove_metadata(void *dst, void *src, size_t n)
+       __must_hold(KMSAN_RUNTIME);
 void kmsan_internal_poison_memory(void *address, size_t size, gfp_t flags,
-                                 unsigned int poison_flags);
-void kmsan_internal_unpoison_memory(void *address, size_t size, bool checked);
+                                 unsigned int poison_flags)
+       __must_hold(KMSAN_RUNTIME);
+void kmsan_internal_unpoison_memory(void *address, size_t size, bool checked)
+       __no_matter(KMSAN_RUNTIME);
+
 void kmsan_internal_set_shadow_origin(void *address, size_t size, int b,
-                                     u32 origin, bool checked);
-depot_stack_handle_t kmsan_internal_chain_origin(depot_stack_handle_t id);
+                                     u32 origin, bool checked)
+       __no_matter(KMSAN_RUNTIME);
+depot_stack_handle_t kmsan_internal_chain_origin(depot_stack_handle_t id)
+       __must_hold(KMSAN_RUNTIME);

 void kmsan_internal_task_create(struct task_struct *task);

 bool kmsan_metadata_is_contiguous(void *addr, size_t size);
 void kmsan_internal_check_memory(void *addr, size_t size,
-                                const void __user *user_addr, int reason);
+                                const void __user *user_addr, int reason)
+       __must_not_hold(KMSAN_RUNTIME);

 struct page *kmsan_vmalloc_to_page_or_null(void *vaddr);
 void kmsan_setup_meta(struct page *page, struct page *shadow,

Powered by blists - more mailing lists