Re: [syzbot] [kernel?] upstream test error: KMSAN: uninit-value in irqentry_exit_to_kernel_mode_preempt

[Alexander Potapenko]

> There are no checks for `@notify_die` arguments because
> `@do_error_trap` receives them as `noundef`.
> If eager checks are enabled, KMSAN pass knows that these arguments
> were already checked previously (see
> https://www.llvm.org/doxygen/MemorySanitizer_8cpp_source.html#l02149),
> so it omits the checks.
>
> `@do_error_trap` is not instrumented, but even if it were, the check
> for `%error_code` would have been pushed to its caller because of the
> `noundef` attribute.
>
> ; Function Attrs: disable_sanitizer_instrumentation
> fn_ret_thunk_extern noinline noprofile noredzone nosanitize_coverage
> nounwind null_pointer_is_valid
> define dso_local void @exc_invalid_tss(ptr noundef %regs, i64 noundef
> %error_code) local_unnamed_addr #4 section ".noinstr.text" align 16
> !dbg !10467 {
> entry:
>     #dbg_value(ptr %regs, !10469, !DIExpression(), !10472)
>     #dbg_value(i64 %error_code, !10470, !DIExpression(), !10472)
>   %call = call i8 @irqentry_enter(ptr noundef %regs) #21, !dbg !10473
>     #dbg_value(i8 %call, !10471, !DIExpression(), !10472)
>   call void asm sideeffect "801: nop\0A\09.pushsection
> .discard.annotate_insn, \22M\22, @progbits, 8; .long 801b - ., 3;
> .popsection", "i,~{dirflag},~{fpsr},~{flags}"(i32 801) #19, !dbg
> !10474, !srcloc !10477
>     #dbg_value(ptr %regs, !10478, !DIExpression(), !10482)
>     #dbg_value(i64 %error_code, !10481, !DIExpression(), !10482)
>   call fastcc void @do_error_trap(ptr noundef %regs, i64 noundef
> %error_code, ptr noundef nonnull @.str.15, i64 noundef 10, i32 noundef
> 11, i32 noundef 0, ptr noundef null) #22, !dbg !10485
>   call void asm sideeffect "802: nop\0A\09.pushsection
> .discard.annotate_insn, \22M\22, @progbits, 8; .long 802b - ., 4;
> .popsection", "i,~{dirflag},~{fpsr},~{flags}"(i32 802) #19, !dbg
> !10486, !srcloc !10489
>   call void @irqentry_exit(ptr noundef %regs, i8 %call) #21, !dbg !10490
>   ret void, !dbg !10473
> }
>
> So, if a `noundef` value is passed along a chain of function calls, it
> is checked only once, at the point where we don't yet know that it is
> `noundef`.
> As mentioned, for `noundef` arguments, no shadow is stored in the TLS.
> Thus, even if non-instrumented functions are along the way, their
> callees still assume that the `noundef` argument is initialized (which
> may introduce false negatives, but these are inevitable when
> non-instrumented code is present).
> To achieve the same behavior for the function argument with TLS shadow
> propagation, we'll indeed need to enable instrumentation within the
> instrumentation_begin()/instrumentation_end() region.
>
> I think doing so will resolve the issues with instrumented functions
> being called from non-instrumented functions in the same KMSAN
> context, and will not require the call_instr() magic you are
> suggesting.
> So let me finally take a stab at it.

Hi Thomas,

I have a draft Clang patch implementing the following intrinsics at
https://github.com/llvm/llvm-project/pull/202603:
- llvm.kmsan.instrumentation.begin
- llvm.kmsan.instrumentation.end
- llvm.kmsan.instrumentation.update.context (to reload the context
pointer; I am not using it yet).

Below is the kernel patch that I am applying to insert these
intrinsics into the instrumentation regions.
I thought it would be a good idea to warn if
llvm.kmsan.instrumentation.{begin,end} are called from functions that
are fully instrumented or have `no_sanitize("memory")` (aka
__no_kmsan_checks), but surprisingly, there were loads of such
functions.

It turned out that WARN() and BUG() unconditionally invoke
instrumentation_begin() and instrumentation_end(), even for
instrumented functions.
Was there any long-term plan to fix that, or is my warning useless by design?

Patch follows:
```
diff --git a/include/linux/instrumentation.h b/include/linux/instrumentation.h
index bf675a8aef8ab..49a2ee638120b 100644
--- a/include/linux/instrumentation.h
+++ b/include/linux/instrumentation.h
@@ -4,6 +4,7 @@

 #ifdef CONFIG_NOINSTR_VALIDATION

+#include <linux/kmsan-checks.h>
 #include <linux/objtool.h>
 #include <linux/stringify.h>

@@ -12,6 +13,7 @@
  asm volatile(__stringify(c) ": nop\n\t" \
       ANNOTATE_INSTR_BEGIN(__ASM_BREF(c)) \
       : : "i" (c)); \
+ kmsan_instrumentation_begin(); \
 })
 #define instrumentation_begin() __instrumentation_begin(__COUNTER__)

@@ -47,6 +49,7 @@
  * part of the condition block and does not escape.
  */
 #define __instrumentation_end(c) ({ \
+ kmsan_instrumentation_end(); \
  asm volatile(__stringify(c) ": nop\n\t" \
       ANNOTATE_INSTR_END(__ASM_BREF(c)) \
       : : "i" (c)); \
diff --git a/include/linux/kmsan-checks.h b/include/linux/kmsan-checks.h
index e1082dc40abc2..be493f836acf0 100644
--- a/include/linux/kmsan-checks.h
+++ b/include/linux/kmsan-checks.h
@@ -12,6 +12,7 @@

 #include <linux/types.h>

+#ifndef __ASSEMBLY__
 #ifdef CONFIG_KMSAN

 /**
@@ -72,6 +73,11 @@ void kmsan_copy_to_user(void __user *to, const void
*from, size_t to_copy,
  */
 void kmsan_memmove(void *to, const void *from, size_t to_copy);

+void llvm_kmsan_instrumentation_begin(void)
__asm__("llvm.kmsan.instrumentation.begin");
+void llvm_kmsan_instrumentation_end(void)
__asm__("llvm.kmsan.instrumentation.end");
+#define kmsan_instrumentation_begin() llvm_kmsan_instrumentation_begin()
+#define kmsan_instrumentation_end() llvm_kmsan_instrumentation_end()
+
 #else

 static inline void kmsan_poison_memory(const void *address, size_t size,
@@ -93,6 +99,10 @@ static inline void kmsan_memmove(void *to, const
void *from, size_t to_copy)
 {
 }

+#define kmsan_instrumentation_begin() do { } while (0)
+#define kmsan_instrumentation_end() do { } while (0)
+
 #endif
+#endif // !__ASSEMBLY__

 #endif /* _LINUX_KMSAN_CHECKS_H */
```