v1: https://lore.kernel.org/lkml/[email protected]/
v1 -> v2: Add comments, sort #includes, introduce
memset_no_sanitize_memory() and use it to avoid unpoisoning
of redzones, change vmalloc alignment to _REGION3_SIZE, add
R-bs (Alexander P.).
Fix building
[PATCH 28/33] s390/string: Add KMSAN support
with FORTIFY_SOURCE.
Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Hi,
This series provides the minimal support for Kernel Memory Sanitizer on
s390. Kernel Memory Sanitizer is clang-only instrumentation for finding
accesses to uninitialized memory. The clang support for s390 has already
been merged [1].
With this series, I can successfully boot s390 defconfig and
debug_defconfig with kmsan.panic=1. The tool found one real
s390-specific bug (fixed in master).
Best regards,
Ilya
[1] https://reviews.llvm.org/D148596
Ilya Leoshkevich (33):
ftrace: Unpoison ftrace_regs in ftrace_ops_list_func()
kmsan: Make the tests compatible with kmsan.panic=1
kmsan: Disable KMSAN when DEFERRED_STRUCT_PAGE_INIT is enabled
kmsan: Increase the maximum store size to 4096
kmsan: Fix is_bad_asm_addr() on arches with overlapping address spaces
kmsan: Fix kmsan_copy_to_user() on arches with overlapping address
spaces
kmsan: Remove a useless assignment from
kmsan_vmap_pages_range_noflush()
kmsan: Remove an x86-specific #include from kmsan.h
kmsan: Introduce kmsan_memmove_metadata()
kmsan: Expose kmsan_get_metadata()
kmsan: Export panic_on_kmsan
kmsan: Allow disabling KMSAN checks for the current task
kmsan: Introduce memset_no_sanitize_memory()
kmsan: Support SLAB_POISON
kmsan: Use ALIGN_DOWN() in kmsan_get_metadata()
mm: slub: Let KMSAN access metadata
mm: kfence: Disable KMSAN when checking the canary
lib/string: Add KMSAN support to strlcpy() and strlcat()
lib/zlib: Unpoison DFLTCC output buffers
kmsan: Accept ranges starting with 0 on s390
s390: Turn off KMSAN for boot, vdso and purgatory
s390: Use a larger stack for KMSAN
s390/boot: Add the KMSAN runtime stub
s390/checksum: Add a KMSAN check
s390/cpacf: Unpoison the results of cpacf_trng()
s390/ftrace: Unpoison ftrace_regs in kprobe_ftrace_handler()
s390/mm: Define KMSAN metadata for vmalloc and modules
s390/string: Add KMSAN support
s390/traps: Unpoison the kernel_stack_overflow()'s pt_regs
s390/uaccess: Add KMSAN support to put_user() and get_user()
s390/unwind: Disable KMSAN checks
s390: Implement the architecture-specific kmsan functions
kmsan: Enable on s390
Documentation/dev-tools/kmsan.rst | 4 +-
arch/s390/Kconfig | 1 +
arch/s390/Makefile | 2 +-
arch/s390/boot/Makefile | 3 +
arch/s390/boot/kmsan.c | 6 ++
arch/s390/boot/startup.c | 8 ++
arch/s390/boot/string.c | 16 ++++
arch/s390/include/asm/checksum.h | 2 +
arch/s390/include/asm/cpacf.h | 2 +
arch/s390/include/asm/kmsan.h | 36 +++++++++
arch/s390/include/asm/pgtable.h | 10 +++
arch/s390/include/asm/string.h | 20 +++--
arch/s390/include/asm/thread_info.h | 2 +-
arch/s390/include/asm/uaccess.h | 110 ++++++++++++++++++++--------
arch/s390/kernel/ftrace.c | 1 +
arch/s390/kernel/traps.c | 6 ++
arch/s390/kernel/unwind_bc.c | 4 +
arch/s390/kernel/vdso32/Makefile | 3 +-
arch/s390/kernel/vdso64/Makefile | 3 +-
arch/s390/purgatory/Makefile | 2 +
include/linux/kmsan-checks.h | 26 +++++++
include/linux/kmsan.h | 23 ++++++
include/linux/kmsan_types.h | 2 +-
kernel/trace/ftrace.c | 1 +
lib/string.c | 6 ++
lib/zlib_dfltcc/dfltcc.h | 1 +
lib/zlib_dfltcc/dfltcc_util.h | 23 ++++++
mm/Kconfig | 1 +
mm/kfence/core.c | 5 +-
mm/kmsan/core.c | 2 +-
mm/kmsan/hooks.c | 30 +++++++-
mm/kmsan/init.c | 5 +-
mm/kmsan/instrumentation.c | 11 +--
mm/kmsan/kmsan.h | 9 +--
mm/kmsan/kmsan_test.c | 5 ++
mm/kmsan/report.c | 7 +-
mm/kmsan/shadow.c | 9 +--
mm/slub.c | 12 ++-
38 files changed, 345 insertions(+), 74 deletions(-)
create mode 100644 arch/s390/boot/kmsan.c
create mode 100644 arch/s390/include/asm/kmsan.h
--
2.41.0
KMSAN relies on memblock returning all available pages to it
(see kmsan_memblock_free_pages()). It partitions these pages into 3
categories: pages available to the buddy allocator, shadow pages and
origin pages. This partitioning is static.
If new pages appear after kmsan_init_runtime(), it is considered
an error. DEFERRED_STRUCT_PAGE_INIT causes this, so mark it as
incompatible with KMSAN.
Reviewed-by: Alexander Potapenko <[email protected]>
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
mm/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/mm/Kconfig b/mm/Kconfig
index 89971a894b60..4f2f99339fc7 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -985,6 +985,7 @@ config DEFERRED_STRUCT_PAGE_INIT
depends on SPARSEMEM
depends on !NEED_PER_CPU_KM
depends on 64BIT
+ depends on !KMSAN
select PADATA
help
Ordinarily all struct pages are initialised during early boot in a
--
2.41.0
It's useful to have both tests and kmsan.panic=1 during development,
but right now the warnings, that the tests cause, lead to kernel
panics.
Temporarily set kmsan.panic=0 for the duration of the KMSAN testing.
Reviewed-by: Alexander Potapenko <[email protected]>
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
mm/kmsan/kmsan_test.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/mm/kmsan/kmsan_test.c b/mm/kmsan/kmsan_test.c
index 07d3a3a5a9c5..9bfd11674fe3 100644
--- a/mm/kmsan/kmsan_test.c
+++ b/mm/kmsan/kmsan_test.c
@@ -659,9 +659,13 @@ static void test_exit(struct kunit *test)
{
}
+static int orig_panic_on_kmsan;
+
static int kmsan_suite_init(struct kunit_suite *suite)
{
register_trace_console(probe_console, NULL);
+ orig_panic_on_kmsan = panic_on_kmsan;
+ panic_on_kmsan = 0;
return 0;
}
@@ -669,6 +673,7 @@ static void kmsan_suite_exit(struct kunit_suite *suite)
{
unregister_trace_console(probe_console, NULL);
tracepoint_synchronize_unregister();
+ panic_on_kmsan = orig_panic_on_kmsan;
}
static struct kunit_suite kmsan_test_suite = {
--
2.41.0
The unwind code can read uninitialized frames. Furthermore, even in
the good case, KMSAN does not emit shadow for backchains. Therefore
disable it for the unwinding functions.
Reviewed-by: Alexander Potapenko <[email protected]>
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
arch/s390/kernel/unwind_bc.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/s390/kernel/unwind_bc.c b/arch/s390/kernel/unwind_bc.c
index 0ece156fdd7c..cd44be2b6ce8 100644
--- a/arch/s390/kernel/unwind_bc.c
+++ b/arch/s390/kernel/unwind_bc.c
@@ -49,6 +49,8 @@ static inline bool is_final_pt_regs(struct unwind_state *state,
READ_ONCE_NOCHECK(regs->psw.mask) & PSW_MASK_PSTATE;
}
+/* Avoid KMSAN false positives from touching uninitialized frames. */
+__no_kmsan_checks
bool unwind_next_frame(struct unwind_state *state)
{
struct stack_info *info = &state->stack_info;
@@ -118,6 +120,8 @@ bool unwind_next_frame(struct unwind_state *state)
}
EXPORT_SYMBOL_GPL(unwind_next_frame);
+/* Avoid KMSAN false positives from touching uninitialized frames. */
+__no_kmsan_checks
void __unwind_start(struct unwind_state *state, struct task_struct *task,
struct pt_regs *regs, unsigned long first_frame)
{
--
2.41.0
Add KMSAN support for the s390 implementations of the string functions.
Do this similar to how it's already done for KASAN, except that the
optimized memset{16,32,64}() functions need to be disabled: it's
important for KMSAN to know that they initialized something.
The way boot code is built with regard to string functions is
problematic, since most files think it's configured with sanitizers,
but boot/string.c doesn't. This creates various problems with the
memset64() definitions, depending on whether the code is built with
sanitizers or fortify. This should probably be streamlined, but in the
meantime resolve the issues by introducing the IN_BOOT_STRING_C macro,
similar to the existing IN_ARCH_STRING_C macro.
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
arch/s390/boot/string.c | 16 ++++++++++++++++
arch/s390/include/asm/string.h | 20 +++++++++++++++-----
2 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/arch/s390/boot/string.c b/arch/s390/boot/string.c
index faccb33b462c..f6b9b1df48a8 100644
--- a/arch/s390/boot/string.c
+++ b/arch/s390/boot/string.c
@@ -1,11 +1,18 @@
// SPDX-License-Identifier: GPL-2.0
+#define IN_BOOT_STRING_C 1
#include <linux/ctype.h>
#include <linux/kernel.h>
#include <linux/errno.h>
#undef CONFIG_KASAN
#undef CONFIG_KASAN_GENERIC
+#undef CONFIG_KMSAN
#include "../lib/string.c"
+/*
+ * Duplicate some functions from the common lib/string.c
+ * instead of fully including it.
+ */
+
int strncmp(const char *cs, const char *ct, size_t count)
{
unsigned char c1, c2;
@@ -22,6 +29,15 @@ int strncmp(const char *cs, const char *ct, size_t count)
return 0;
}
+void *memset64(uint64_t *s, uint64_t v, size_t count)
+{
+ uint64_t *xs = s;
+
+ while (count--)
+ *xs++ = v;
+ return s;
+}
+
char *skip_spaces(const char *str)
{
while (isspace(*str))
diff --git a/arch/s390/include/asm/string.h b/arch/s390/include/asm/string.h
index 351685de53d2..2ab868cbae6c 100644
--- a/arch/s390/include/asm/string.h
+++ b/arch/s390/include/asm/string.h
@@ -15,15 +15,12 @@
#define __HAVE_ARCH_MEMCPY /* gcc builtin & arch function */
#define __HAVE_ARCH_MEMMOVE /* gcc builtin & arch function */
#define __HAVE_ARCH_MEMSET /* gcc builtin & arch function */
-#define __HAVE_ARCH_MEMSET16 /* arch function */
-#define __HAVE_ARCH_MEMSET32 /* arch function */
-#define __HAVE_ARCH_MEMSET64 /* arch function */
void *memcpy(void *dest, const void *src, size_t n);
void *memset(void *s, int c, size_t n);
void *memmove(void *dest, const void *src, size_t n);
-#ifndef CONFIG_KASAN
+#if !defined(CONFIG_KASAN) && !defined(CONFIG_KMSAN)
#define __HAVE_ARCH_MEMCHR /* inline & arch function */
#define __HAVE_ARCH_MEMCMP /* arch function */
#define __HAVE_ARCH_MEMSCAN /* inline & arch function */
@@ -36,6 +33,9 @@ void *memmove(void *dest, const void *src, size_t n);
#define __HAVE_ARCH_STRNCPY /* arch function */
#define __HAVE_ARCH_STRNLEN /* inline & arch function */
#define __HAVE_ARCH_STRSTR /* arch function */
+#define __HAVE_ARCH_MEMSET16 /* arch function */
+#define __HAVE_ARCH_MEMSET32 /* arch function */
+#define __HAVE_ARCH_MEMSET64 /* arch function */
/* Prototypes for non-inlined arch strings functions. */
int memcmp(const void *s1, const void *s2, size_t n);
@@ -44,7 +44,7 @@ size_t strlcat(char *dest, const char *src, size_t n);
char *strncat(char *dest, const char *src, size_t n);
char *strncpy(char *dest, const char *src, size_t n);
char *strstr(const char *s1, const char *s2);
-#endif /* !CONFIG_KASAN */
+#endif /* !defined(CONFIG_KASAN) && !defined(CONFIG_KMSAN) */
#undef __HAVE_ARCH_STRCHR
#undef __HAVE_ARCH_STRNCHR
@@ -74,20 +74,30 @@ void *__memset16(uint16_t *s, uint16_t v, size_t count);
void *__memset32(uint32_t *s, uint32_t v, size_t count);
void *__memset64(uint64_t *s, uint64_t v, size_t count);
+#ifdef __HAVE_ARCH_MEMSET16
static inline void *memset16(uint16_t *s, uint16_t v, size_t count)
{
return __memset16(s, v, count * sizeof(v));
}
+#endif
+#ifdef __HAVE_ARCH_MEMSET32
static inline void *memset32(uint32_t *s, uint32_t v, size_t count)
{
return __memset32(s, v, count * sizeof(v));
}
+#endif
+#ifdef __HAVE_ARCH_MEMSET64
+#ifdef IN_BOOT_STRING_C
+void *memset64(uint64_t *s, uint64_t v, size_t count);
+#else
static inline void *memset64(uint64_t *s, uint64_t v, size_t count)
{
return __memset64(s, v, count * sizeof(v));
}
+#endif
+#endif
#if !defined(IN_ARCH_STRING_C) && (!defined(CONFIG_FORTIFY_SOURCE) || defined(__NO_FORTIFY))
--
2.41.0
Comparing pointers with TASK_SIZE does not make sense when kernel and
userspace overlap. Assume that we are handling user memory access in
this case.
Reported-by: Alexander Gordeev <[email protected]>
Reviewed-by: Alexander Potapenko <[email protected]>
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
mm/kmsan/hooks.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/kmsan/hooks.c b/mm/kmsan/hooks.c
index 5d6e2dee5692..eafc45f937eb 100644
--- a/mm/kmsan/hooks.c
+++ b/mm/kmsan/hooks.c
@@ -267,7 +267,8 @@ void kmsan_copy_to_user(void __user *to, const void *from, size_t to_copy,
return;
ua_flags = user_access_save();
- if ((u64)to < TASK_SIZE) {
+ if (!IS_ENABLED(CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE) ||
+ (u64)to < TASK_SIZE) {
/* This is a user memory access, check it. */
kmsan_internal_check_memory((void *)from, to_copy - left, to,
REASON_COPY_TO_USER);
--
2.41.0
All other sanitizers are disabled for these components as well.
While at it, add a comment to boot and purgatory.
Reviewed-by: Alexander Gordeev <[email protected]>
Reviewed-by: Alexander Potapenko <[email protected]>
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
arch/s390/boot/Makefile | 2 ++
arch/s390/kernel/vdso32/Makefile | 3 ++-
arch/s390/kernel/vdso64/Makefile | 3 ++-
arch/s390/purgatory/Makefile | 2 ++
4 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/s390/boot/Makefile b/arch/s390/boot/Makefile
index c7c81e5f9218..fb10fcd21221 100644
--- a/arch/s390/boot/Makefile
+++ b/arch/s390/boot/Makefile
@@ -3,11 +3,13 @@
# Makefile for the linux s390-specific parts of the memory manager.
#
+# Tooling runtimes are unavailable and cannot be linked for early boot code
KCOV_INSTRUMENT := n
GCOV_PROFILE := n
UBSAN_SANITIZE := n
KASAN_SANITIZE := n
KCSAN_SANITIZE := n
+KMSAN_SANITIZE := n
KBUILD_AFLAGS := $(KBUILD_AFLAGS_DECOMPRESSOR)
KBUILD_CFLAGS := $(KBUILD_CFLAGS_DECOMPRESSOR)
diff --git a/arch/s390/kernel/vdso32/Makefile b/arch/s390/kernel/vdso32/Makefile
index caec7db6f966..7cbec6b0b11f 100644
--- a/arch/s390/kernel/vdso32/Makefile
+++ b/arch/s390/kernel/vdso32/Makefile
@@ -32,11 +32,12 @@ obj-y += vdso32_wrapper.o
targets += vdso32.lds
CPPFLAGS_vdso32.lds += -P -C -U$(ARCH)
-# Disable gcov profiling, ubsan and kasan for VDSO code
+# Disable gcov profiling, ubsan, kasan and kmsan for VDSO code
GCOV_PROFILE := n
UBSAN_SANITIZE := n
KASAN_SANITIZE := n
KCSAN_SANITIZE := n
+KMSAN_SANITIZE := n
# Force dependency (incbin is bad)
$(obj)/vdso32_wrapper.o : $(obj)/vdso32.so
diff --git a/arch/s390/kernel/vdso64/Makefile b/arch/s390/kernel/vdso64/Makefile
index e3c9085f8fa7..6f3252712f64 100644
--- a/arch/s390/kernel/vdso64/Makefile
+++ b/arch/s390/kernel/vdso64/Makefile
@@ -36,11 +36,12 @@ obj-y += vdso64_wrapper.o
targets += vdso64.lds
CPPFLAGS_vdso64.lds += -P -C -U$(ARCH)
-# Disable gcov profiling, ubsan and kasan for VDSO code
+# Disable gcov profiling, ubsan, kasan and kmsan for VDSO code
GCOV_PROFILE := n
UBSAN_SANITIZE := n
KASAN_SANITIZE := n
KCSAN_SANITIZE := n
+KMSAN_SANITIZE := n
# Force dependency (incbin is bad)
$(obj)/vdso64_wrapper.o : $(obj)/vdso64.so
diff --git a/arch/s390/purgatory/Makefile b/arch/s390/purgatory/Makefile
index 4e930f566878..4e421914e50f 100644
--- a/arch/s390/purgatory/Makefile
+++ b/arch/s390/purgatory/Makefile
@@ -15,11 +15,13 @@ CFLAGS_sha256.o := -D__DISABLE_EXPORTS -D__NO_FORTIFY
$(obj)/mem.o: $(srctree)/arch/s390/lib/mem.S FORCE
$(call if_changed_rule,as_o_S)
+# Tooling runtimes are unavailable and cannot be linked for purgatory code
KCOV_INSTRUMENT := n
GCOV_PROFILE := n
UBSAN_SANITIZE := n
KASAN_SANITIZE := n
KCSAN_SANITIZE := n
+KMSAN_SANITIZE := n
KBUILD_CFLAGS := -fno-strict-aliasing -Wall -Wstrict-prototypes
KBUILD_CFLAGS += -Wno-pointer-sign -Wno-sign-compare
--
2.41.0
put_user() uses inline assembly with precise constraints, so Clang is
in principle capable of instrumenting it automatically. Unfortunately,
one of the constraints contains a dereferenced user pointer, and Clang
does not currently distinguish user and kernel pointers. Therefore
KMSAN attempts to access shadow for user pointers, which is not a right
thing to do.
An obvious fix to add __no_sanitize_memory to __put_user_fn() does not
work, since it's __always_inline. And __always_inline cannot be removed
due to the __put_user_bad() trick.
A different obvious fix of using the "a" instead of the "+Q" constraint
degrades the code quality, which is very important here, since it's a
hot path.
Instead, repurpose the __put_user_asm() macro to define
__put_user_{char,short,int,long}_noinstr() functions and mark them with
__no_sanitize_memory. For the non-KMSAN builds make them
__always_inline in order to keep the generated code quality. Also
define __put_user_{char,short,int,long}() functions, which call the
aforementioned ones and which *are* instrumented, because they call
KMSAN hooks, which may be implemented as macros.
The same applies to get_user() as well.
Acked-by: Heiko Carstens <[email protected]>
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
arch/s390/include/asm/uaccess.h | 110 ++++++++++++++++++++++----------
1 file changed, 78 insertions(+), 32 deletions(-)
diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
index 81ae8a98e7ec..b0715b88b55a 100644
--- a/arch/s390/include/asm/uaccess.h
+++ b/arch/s390/include/asm/uaccess.h
@@ -78,13 +78,23 @@ union oac {
int __noreturn __put_user_bad(void);
-#define __put_user_asm(to, from, size) \
-({ \
+#ifdef CONFIG_KMSAN
+#define GET_PUT_USER_NOINSTR_ATTRIBUTES inline __no_sanitize_memory
+#else
+#define GET_PUT_USER_NOINSTR_ATTRIBUTES __always_inline
+#endif
+
+#define DEFINE_PUT_USER(type) \
+static GET_PUT_USER_NOINSTR_ATTRIBUTES int \
+__put_user_##type##_noinstr(unsigned type __user *to, \
+ unsigned type *from, \
+ unsigned long size) \
+{ \
union oac __oac_spec = { \
.oac1.as = PSW_BITS_AS_SECONDARY, \
.oac1.a = 1, \
}; \
- int __rc; \
+ int rc; \
\
asm volatile( \
" lr 0,%[spec]\n" \
@@ -93,12 +103,28 @@ int __noreturn __put_user_bad(void);
"2:\n" \
EX_TABLE_UA_STORE(0b, 2b, %[rc]) \
EX_TABLE_UA_STORE(1b, 2b, %[rc]) \
- : [rc] "=&d" (__rc), [_to] "+Q" (*(to)) \
+ : [rc] "=&d" (rc), [_to] "+Q" (*(to)) \
: [_size] "d" (size), [_from] "Q" (*(from)), \
[spec] "d" (__oac_spec.val) \
: "cc", "0"); \
- __rc; \
-})
+ return rc; \
+} \
+ \
+static __always_inline int \
+__put_user_##type(unsigned type __user *to, unsigned type *from, \
+ unsigned long size) \
+{ \
+ int rc; \
+ \
+ rc = __put_user_##type##_noinstr(to, from, size); \
+ instrument_put_user(*from, to, size); \
+ return rc; \
+}
+
+DEFINE_PUT_USER(char);
+DEFINE_PUT_USER(short);
+DEFINE_PUT_USER(int);
+DEFINE_PUT_USER(long);
static __always_inline int __put_user_fn(void *x, void __user *ptr, unsigned long size)
{
@@ -106,24 +132,24 @@ static __always_inline int __put_user_fn(void *x, void __user *ptr, unsigned lon
switch (size) {
case 1:
- rc = __put_user_asm((unsigned char __user *)ptr,
- (unsigned char *)x,
- size);
+ rc = __put_user_char((unsigned char __user *)ptr,
+ (unsigned char *)x,
+ size);
break;
case 2:
- rc = __put_user_asm((unsigned short __user *)ptr,
- (unsigned short *)x,
- size);
+ rc = __put_user_short((unsigned short __user *)ptr,
+ (unsigned short *)x,
+ size);
break;
case 4:
- rc = __put_user_asm((unsigned int __user *)ptr,
+ rc = __put_user_int((unsigned int __user *)ptr,
(unsigned int *)x,
size);
break;
case 8:
- rc = __put_user_asm((unsigned long __user *)ptr,
- (unsigned long *)x,
- size);
+ rc = __put_user_long((unsigned long __user *)ptr,
+ (unsigned long *)x,
+ size);
break;
default:
__put_user_bad();
@@ -134,13 +160,17 @@ static __always_inline int __put_user_fn(void *x, void __user *ptr, unsigned lon
int __noreturn __get_user_bad(void);
-#define __get_user_asm(to, from, size) \
-({ \
+#define DEFINE_GET_USER(type) \
+static GET_PUT_USER_NOINSTR_ATTRIBUTES int \
+__get_user_##type##_noinstr(unsigned type *to, \
+ unsigned type __user *from, \
+ unsigned long size) \
+{ \
union oac __oac_spec = { \
.oac2.as = PSW_BITS_AS_SECONDARY, \
.oac2.a = 1, \
}; \
- int __rc; \
+ int rc; \
\
asm volatile( \
" lr 0,%[spec]\n" \
@@ -149,13 +179,29 @@ int __noreturn __get_user_bad(void);
"2:\n" \
EX_TABLE_UA_LOAD_MEM(0b, 2b, %[rc], %[_to], %[_ksize]) \
EX_TABLE_UA_LOAD_MEM(1b, 2b, %[rc], %[_to], %[_ksize]) \
- : [rc] "=&d" (__rc), "=Q" (*(to)) \
+ : [rc] "=&d" (rc), "=Q" (*(to)) \
: [_size] "d" (size), [_from] "Q" (*(from)), \
[spec] "d" (__oac_spec.val), [_to] "a" (to), \
[_ksize] "K" (size) \
: "cc", "0"); \
- __rc; \
-})
+ return rc; \
+} \
+ \
+static __always_inline int \
+__get_user_##type(unsigned type *to, unsigned type __user *from, \
+ unsigned long size) \
+{ \
+ int rc; \
+ \
+ rc = __get_user_##type##_noinstr(to, from, size); \
+ instrument_get_user(*to); \
+ return rc; \
+}
+
+DEFINE_GET_USER(char);
+DEFINE_GET_USER(short);
+DEFINE_GET_USER(int);
+DEFINE_GET_USER(long);
static __always_inline int __get_user_fn(void *x, const void __user *ptr, unsigned long size)
{
@@ -163,24 +209,24 @@ static __always_inline int __get_user_fn(void *x, const void __user *ptr, unsign
switch (size) {
case 1:
- rc = __get_user_asm((unsigned char *)x,
- (unsigned char __user *)ptr,
- size);
+ rc = __get_user_char((unsigned char *)x,
+ (unsigned char __user *)ptr,
+ size);
break;
case 2:
- rc = __get_user_asm((unsigned short *)x,
- (unsigned short __user *)ptr,
- size);
+ rc = __get_user_short((unsigned short *)x,
+ (unsigned short __user *)ptr,
+ size);
break;
case 4:
- rc = __get_user_asm((unsigned int *)x,
+ rc = __get_user_int((unsigned int *)x,
(unsigned int __user *)ptr,
size);
break;
case 8:
- rc = __get_user_asm((unsigned long *)x,
- (unsigned long __user *)ptr,
- size);
+ rc = __get_user_long((unsigned long *)x,
+ (unsigned long __user *)ptr,
+ size);
break;
default:
__get_user_bad();
--
2.41.0
It is useful to manually copy metadata in order to describe the effects
of memmove()-like logic in uninstrumented code or inline asm. Introduce
kmsan_memmove_metadata() for this purpose.
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
include/linux/kmsan-checks.h | 14 ++++++++++++++
mm/kmsan/hooks.c | 11 +++++++++++
2 files changed, 25 insertions(+)
diff --git a/include/linux/kmsan-checks.h b/include/linux/kmsan-checks.h
index c4cae333deec..5218973f0ad0 100644
--- a/include/linux/kmsan-checks.h
+++ b/include/linux/kmsan-checks.h
@@ -61,6 +61,17 @@ void kmsan_check_memory(const void *address, size_t size);
void kmsan_copy_to_user(void __user *to, const void *from, size_t to_copy,
size_t left);
+/**
+ * kmsan_memmove_metadata() - Copy kernel memory range metadata.
+ * @dst: start of the destination kernel memory range.
+ * @src: start of the source kernel memory range.
+ * @n: size of the memory ranges.
+ *
+ * KMSAN will treat the destination range as if its contents were memmove()d
+ * from the source range.
+ */
+void kmsan_memmove_metadata(void *dst, const void *src, size_t n);
+
#else
static inline void kmsan_poison_memory(const void *address, size_t size,
@@ -77,6 +88,9 @@ static inline void kmsan_copy_to_user(void __user *to, const void *from,
size_t to_copy, size_t left)
{
}
+static inline void kmsan_memmove_metadata(void *dst, const void *src, size_t n)
+{
+}
#endif
diff --git a/mm/kmsan/hooks.c b/mm/kmsan/hooks.c
index eafc45f937eb..4d477a0a356c 100644
--- a/mm/kmsan/hooks.c
+++ b/mm/kmsan/hooks.c
@@ -286,6 +286,17 @@ void kmsan_copy_to_user(void __user *to, const void *from, size_t to_copy,
}
EXPORT_SYMBOL(kmsan_copy_to_user);
+void kmsan_memmove_metadata(void *dst, const void *src, size_t n)
+{
+ if (!kmsan_enabled || kmsan_in_runtime())
+ return;
+
+ kmsan_enter_runtime();
+ kmsan_internal_memmove_metadata(dst, (void *)src, n);
+ kmsan_leave_runtime();
+}
+EXPORT_SYMBOL(kmsan_memmove_metadata);
+
/* Helper function to check an URB. */
void kmsan_handle_urb(const struct urb *urb, bool is_out)
{
--
2.41.0
The inline assembly block in s390's chsc() stores that much.
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
mm/kmsan/instrumentation.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/mm/kmsan/instrumentation.c b/mm/kmsan/instrumentation.c
index cc3907a9c33a..470b0b4afcc4 100644
--- a/mm/kmsan/instrumentation.c
+++ b/mm/kmsan/instrumentation.c
@@ -110,11 +110,10 @@ void __msan_instrument_asm_store(void *addr, uintptr_t size)
ua_flags = user_access_save();
/*
- * Most of the accesses are below 32 bytes. The two exceptions so far
- * are clwb() (64 bytes) and FPU state (512 bytes).
- * It's unlikely that the assembly will touch more than 512 bytes.
+ * Most of the accesses are below 32 bytes. The exceptions so far are
+ * clwb() (64 bytes), FPU state (512 bytes) and chsc() (4096 bytes).
*/
- if (size > 512) {
+ if (size > 4096) {
WARN_ONCE(1, "assembly store size too big: %ld\n", size);
size = 8;
}
--
2.41.0
The constraints of the DFLTCC inline assembly are not precise: they
do not communicate the size of the output buffers to the compiler, so
it cannot automatically instrument it.
Add the manual kmsan_unpoison_memory() calls for the output buffers.
The logic is the same as in [1].
[1] https://github.com/zlib-ng/zlib-ng/commit/1f5ddcc009ac3511e99fc88736a9e1a6381168c5
Reported-by: Alexander Gordeev <[email protected]>
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
lib/zlib_dfltcc/dfltcc.h | 1 +
lib/zlib_dfltcc/dfltcc_util.h | 23 +++++++++++++++++++++++
2 files changed, 24 insertions(+)
diff --git a/lib/zlib_dfltcc/dfltcc.h b/lib/zlib_dfltcc/dfltcc.h
index b96232bdd44d..0f2a16d7a48a 100644
--- a/lib/zlib_dfltcc/dfltcc.h
+++ b/lib/zlib_dfltcc/dfltcc.h
@@ -80,6 +80,7 @@ struct dfltcc_param_v0 {
uint8_t csb[1152];
};
+static_assert(offsetof(struct dfltcc_param_v0, csb) == 384);
static_assert(sizeof(struct dfltcc_param_v0) == 1536);
#define CVT_CRC32 0
diff --git a/lib/zlib_dfltcc/dfltcc_util.h b/lib/zlib_dfltcc/dfltcc_util.h
index 4a46b5009f0d..ce2e039a55b5 100644
--- a/lib/zlib_dfltcc/dfltcc_util.h
+++ b/lib/zlib_dfltcc/dfltcc_util.h
@@ -2,6 +2,7 @@
#ifndef DFLTCC_UTIL_H
#define DFLTCC_UTIL_H
+#include "dfltcc.h"
#include <linux/zutil.h>
/*
@@ -20,6 +21,7 @@ typedef enum {
#define DFLTCC_CMPR 2
#define DFLTCC_XPND 4
#define HBT_CIRCULAR (1 << 7)
+#define DFLTCC_FN_MASK ((1 << 7) - 1)
#define HB_BITS 15
#define HB_SIZE (1 << HB_BITS)
@@ -34,6 +36,7 @@ static inline dfltcc_cc dfltcc(
)
{
Byte *t2 = op1 ? *op1 : NULL;
+ unsigned char *orig_t2 = t2;
size_t t3 = len1 ? *len1 : 0;
const Byte *t4 = op2 ? *op2 : NULL;
size_t t5 = len2 ? *len2 : 0;
@@ -59,6 +62,26 @@ static inline dfltcc_cc dfltcc(
: "cc", "memory");
t2 = r2; t3 = r3; t4 = r4; t5 = r5;
+ switch (fn & DFLTCC_FN_MASK) {
+ case DFLTCC_QAF:
+ kmsan_unpoison_memory(param, sizeof(struct dfltcc_qaf_param));
+ break;
+ case DFLTCC_GDHT:
+ kmsan_unpoison_memory(param, offsetof(struct dfltcc_param_v0, csb));
+ break;
+ case DFLTCC_CMPR:
+ kmsan_unpoison_memory(param, sizeof(struct dfltcc_param_v0));
+ kmsan_unpoison_memory(
+ orig_t2,
+ t2 - orig_t2 +
+ (((struct dfltcc_param_v0 *)param)->sbb == 0 ? 0 : 1));
+ break;
+ case DFLTCC_XPND:
+ kmsan_unpoison_memory(param, sizeof(struct dfltcc_param_v0));
+ kmsan_unpoison_memory(orig_t2, t2 - orig_t2);
+ break;
+ }
+
if (op1)
*op1 = t2;
if (len1)
--
2.41.0
The pages for the KMSAN metadata associated with most kernel mappings
are taken from memblock by the common code. However, vmalloc and module
metadata needs to be defined by the architectures.
Be a little bit more careful than x86: allocate exactly MODULES_LEN
for the module shadow and origins, and then take 2/3 of vmalloc for
the vmalloc shadow and origins. This ensures that users passing small
vmalloc= values on the command line do not cause module metadata
collisions.
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
arch/s390/boot/startup.c | 8 ++++++++
arch/s390/include/asm/pgtable.h | 10 ++++++++++
2 files changed, 18 insertions(+)
diff --git a/arch/s390/boot/startup.c b/arch/s390/boot/startup.c
index 8104e0e3d188..e37e7ffda430 100644
--- a/arch/s390/boot/startup.c
+++ b/arch/s390/boot/startup.c
@@ -253,9 +253,17 @@ static unsigned long setup_kernel_memory_layout(void)
MODULES_END = round_down(__abs_lowcore, _SEGMENT_SIZE);
MODULES_VADDR = MODULES_END - MODULES_LEN;
VMALLOC_END = MODULES_VADDR;
+#ifdef CONFIG_KMSAN
+ VMALLOC_END -= MODULES_LEN * 2;
+#endif
/* allow vmalloc area to occupy up to about 1/2 of the rest virtual space left */
vmalloc_size = min(vmalloc_size, round_down(VMALLOC_END / 2, _REGION3_SIZE));
+#ifdef CONFIG_KMSAN
+ /* take 2/3 of vmalloc area for KMSAN shadow and origins */
+ vmalloc_size = round_down(vmalloc_size / 3, _REGION3_SIZE);
+ VMALLOC_END -= vmalloc_size * 2;
+#endif
VMALLOC_START = VMALLOC_END - vmalloc_size;
/* split remaining virtual space between 1:1 mapping & vmemmap array */
diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
index 601e87fa8a9a..d764abeb9e6d 100644
--- a/arch/s390/include/asm/pgtable.h
+++ b/arch/s390/include/asm/pgtable.h
@@ -107,6 +107,16 @@ static inline int is_module_addr(void *addr)
return 1;
}
+#ifdef CONFIG_KMSAN
+#define KMSAN_VMALLOC_SIZE (VMALLOC_END - VMALLOC_START)
+#define KMSAN_VMALLOC_SHADOW_START VMALLOC_END
+#define KMSAN_VMALLOC_ORIGIN_START (KMSAN_VMALLOC_SHADOW_START + \
+ KMSAN_VMALLOC_SIZE)
+#define KMSAN_MODULES_SHADOW_START (KMSAN_VMALLOC_ORIGIN_START + \
+ KMSAN_VMALLOC_SIZE)
+#define KMSAN_MODULES_ORIGIN_START (KMSAN_MODULES_SHADOW_START + MODULES_LEN)
+#endif
+
/*
* A 64 bit pagetable entry of S390 has following format:
* | PFRA |0IPC| OS |
--
2.41.0
Like for KASAN, it's useful to temporarily disable KMSAN checks around,
e.g., redzone accesses. Introduce kmsan_disable_current() and
kmsan_enable_current(), which are similar to their KASAN counterparts.
Even though it's not strictly necessary, make them reentrant, in order
to match the KASAN behavior. Repurpose the allow_reporting field for
this.
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
Documentation/dev-tools/kmsan.rst | 4 ++--
include/linux/kmsan-checks.h | 12 ++++++++++++
include/linux/kmsan_types.h | 2 +-
mm/kmsan/core.c | 2 +-
mm/kmsan/hooks.c | 14 +++++++++++++-
mm/kmsan/report.c | 6 +++---
6 files changed, 32 insertions(+), 8 deletions(-)
diff --git a/Documentation/dev-tools/kmsan.rst b/Documentation/dev-tools/kmsan.rst
index 323eedad53cd..022a823f5f1b 100644
--- a/Documentation/dev-tools/kmsan.rst
+++ b/Documentation/dev-tools/kmsan.rst
@@ -338,11 +338,11 @@ Per-task KMSAN state
~~~~~~~~~~~~~~~~~~~~
Every task_struct has an associated KMSAN task state that holds the KMSAN
-context (see above) and a per-task flag disallowing KMSAN reports::
+context (see above) and a per-task counter disallowing KMSAN reports::
struct kmsan_context {
...
- bool allow_reporting;
+ unsigned int depth;
struct kmsan_context_state cstate;
...
}
diff --git a/include/linux/kmsan-checks.h b/include/linux/kmsan-checks.h
index 5218973f0ad0..bab2603685f7 100644
--- a/include/linux/kmsan-checks.h
+++ b/include/linux/kmsan-checks.h
@@ -72,6 +72,10 @@ void kmsan_copy_to_user(void __user *to, const void *from, size_t to_copy,
*/
void kmsan_memmove_metadata(void *dst, const void *src, size_t n);
+void kmsan_enable_current(void);
+
+void kmsan_disable_current(void);
+
#else
static inline void kmsan_poison_memory(const void *address, size_t size,
@@ -92,6 +96,14 @@ static inline void kmsan_memmove_metadata(void *dst, const void *src, size_t n)
{
}
+static inline void kmsan_enable_current(void)
+{
+}
+
+static inline void kmsan_disable_current(void)
+{
+}
+
#endif
#endif /* _LINUX_KMSAN_CHECKS_H */
diff --git a/include/linux/kmsan_types.h b/include/linux/kmsan_types.h
index 8bfa6c98176d..27bb146ece95 100644
--- a/include/linux/kmsan_types.h
+++ b/include/linux/kmsan_types.h
@@ -29,7 +29,7 @@ struct kmsan_context_state {
struct kmsan_ctx {
struct kmsan_context_state cstate;
int kmsan_in_runtime;
- bool allow_reporting;
+ unsigned int depth;
};
#endif /* _LINUX_KMSAN_TYPES_H */
diff --git a/mm/kmsan/core.c b/mm/kmsan/core.c
index c19f47af0424..b8767378cf8a 100644
--- a/mm/kmsan/core.c
+++ b/mm/kmsan/core.c
@@ -43,7 +43,7 @@ void kmsan_internal_task_create(struct task_struct *task)
struct thread_info *info = current_thread_info();
__memset(ctx, 0, sizeof(*ctx));
- ctx->allow_reporting = true;
+ ctx->depth = 0;
kmsan_internal_unpoison_memory(info, sizeof(*info), false);
}
diff --git a/mm/kmsan/hooks.c b/mm/kmsan/hooks.c
index 4d477a0a356c..7b5814412e9f 100644
--- a/mm/kmsan/hooks.c
+++ b/mm/kmsan/hooks.c
@@ -44,7 +44,7 @@ void kmsan_task_exit(struct task_struct *task)
if (!kmsan_enabled || kmsan_in_runtime())
return;
- ctx->allow_reporting = false;
+ ctx->depth++;
}
void kmsan_slab_alloc(struct kmem_cache *s, void *object, gfp_t flags)
@@ -434,3 +434,15 @@ void kmsan_check_memory(const void *addr, size_t size)
REASON_ANY);
}
EXPORT_SYMBOL(kmsan_check_memory);
+
+void kmsan_enable_current(void)
+{
+ current->kmsan_ctx.depth--;
+}
+EXPORT_SYMBOL(kmsan_enable_current);
+
+void kmsan_disable_current(void)
+{
+ current->kmsan_ctx.depth++;
+}
+EXPORT_SYMBOL(kmsan_disable_current);
diff --git a/mm/kmsan/report.c b/mm/kmsan/report.c
index c79d3b0d2d0d..edcf53ca428e 100644
--- a/mm/kmsan/report.c
+++ b/mm/kmsan/report.c
@@ -158,12 +158,12 @@ void kmsan_report(depot_stack_handle_t origin, void *address, int size,
if (!kmsan_enabled)
return;
- if (!current->kmsan_ctx.allow_reporting)
+ if (current->kmsan_ctx.depth)
return;
if (!origin)
return;
- current->kmsan_ctx.allow_reporting = false;
+ current->kmsan_ctx.depth++;
ua_flags = user_access_save();
raw_spin_lock(&kmsan_report_lock);
pr_err("=====================================================\n");
@@ -216,5 +216,5 @@ void kmsan_report(depot_stack_handle_t origin, void *address, int size,
if (panic_on_kmsan)
panic("kmsan.panic set ...\n");
user_access_restore(ua_flags);
- current->kmsan_ctx.allow_reporting = true;
+ current->kmsan_ctx.depth--;
}
--
2.41.0
Add a wrapper for memset() that prevents unpoisoning. This is useful
for filling memory allocator redzones.
Signed-off-by: Ilya Leoshkevich <[email protected]>
---
include/linux/kmsan.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/linux/kmsan.h b/include/linux/kmsan.h
index ff8fd95733fa..439df72c8dc6 100644
--- a/include/linux/kmsan.h
+++ b/include/linux/kmsan.h
@@ -345,4 +345,13 @@ static inline void *kmsan_get_metadata(void *addr, bool is_origin)
#endif
+/**
+ * memset_no_sanitize_memory() - memset() without the KMSAN instrumentation.
+ */
+__no_sanitize_memory
+static inline void *memset_no_sanitize_memory(void *s, int c, size_t n)
+{
+ return memset(s, c, n);
+}
+
#endif /* _LINUX_KMSAN_H */
--
2.41.0
On Tue, Nov 21, 2023 at 11:07 PM Ilya Leoshkevich <[email protected]> wrote:
>
> The constraints of the DFLTCC inline assembly are not precise: they
> do not communicate the size of the output buffers to the compiler, so
> it cannot automatically instrument it.
KMSAN usually does a poor job instrumenting inline assembly.
Wouldn't be it better to switch to pure C ZLIB implementation, making
ZLIB_DFLTCC depend on !KMSAN?
On Tue, Nov 21, 2023 at 11:06 PM Ilya Leoshkevich <[email protected]> wrote:
>
> Add a wrapper for memset() that prevents unpoisoning.
We have __memset() already, won't it work for this case?
On the other hand, I am not sure you want to preserve the redzone in
its previous state (unless it's known to be poisoned).
You might consider explicitly unpoisoning the redzone instead.
...
> +__no_sanitize_memory
> +static inline void *memset_no_sanitize_memory(void *s, int c, size_t n)
> +{
> + return memset(s, c, n);
> +}
I think depending on the compiler optimizations this might end up
being a call to normal memset, that would still change the shadow
bytes.
On Fri, 2023-12-08 at 14:48 +0100, Alexander Potapenko wrote:
> On Tue, Nov 21, 2023 at 11:06 PM Ilya Leoshkevich <[email protected]>
> wrote:
> >
> > Add a wrapper for memset() that prevents unpoisoning.
>
> We have __memset() already, won't it work for this case?
A problem with __memset() is that, at least for me, it always ends
up being a call. There is a use case where we need to write only 1
byte, so I thought that introducing a call there (when compiling
without KMSAN) would be unacceptable.
> On the other hand, I am not sure you want to preserve the redzone in
> its previous state (unless it's known to be poisoned).
That's exactly the problem with unpoisoning: it removes the distinction
between a new allocation and a UAF.
> You might consider explicitly unpoisoning the redzone instead.
That was my first attempt, but it resulted in test failures due to the
above.
> ...
>
> > +__no_sanitize_memory
> > +static inline void *memset_no_sanitize_memory(void *s, int c,
> > size_t n)
> > +{
> > + return memset(s, c, n);
> > +}
>
> I think depending on the compiler optimizations this might end up
> being a call to normal memset, that would still change the shadow
> bytes.
Interesting, do you have some specific scenario in mind? I vaguely
remember that in the past there were cases when sanitizer annotations
were lost after inlining, but I thought they were sorted out?
And, in any case, if this were to happen, would not it be considered a
compiler bug that needs fixing there, and not in the kernel?
On Fri, 2023-12-08 at 14:32 +0100, Alexander Potapenko wrote:
> On Tue, Nov 21, 2023 at 11:07 PM Ilya Leoshkevich <[email protected]>
> wrote:
> >
> > The constraints of the DFLTCC inline assembly are not precise: they
> > do not communicate the size of the output buffers to the compiler,
> > so
> > it cannot automatically instrument it.
>
> KMSAN usually does a poor job instrumenting inline assembly.
> Wouldn't be it better to switch to pure C ZLIB implementation, making
> ZLIB_DFLTCC depend on !KMSAN?
Normally I would agree, but the kernel DFLTCC code base is synced with
the zlib-ng code base to the extent that it uses the zlib-ng code style
instead of the kernel code style, and MSAN annotations are already a
part of the zlib-ng code base. So I would prefer to keep them for
consistency.
The code is also somewhat tricky in the are of buffer management, so I
find it beneficial to have it checked for uninitialized memory
accesses.
On Fri, Dec 8, 2023 at 3:14 PM Ilya Leoshkevich <[email protected]> wrote:
>
> On Fri, 2023-12-08 at 14:32 +0100, Alexander Potapenko wrote:
> > On Tue, Nov 21, 2023 at 11:07 PM Ilya Leoshkevich <[email protected]>
> > wrote:
> > >
> > > The constraints of the DFLTCC inline assembly are not precise: they
> > > do not communicate the size of the output buffers to the compiler,
> > > so
> > > it cannot automatically instrument it.
> >
> > KMSAN usually does a poor job instrumenting inline assembly.
> > Wouldn't be it better to switch to pure C ZLIB implementation, making
> > ZLIB_DFLTCC depend on !KMSAN?
>
> Normally I would agree, but the kernel DFLTCC code base is synced with
> the zlib-ng code base to the extent that it uses the zlib-ng code style
> instead of the kernel code style, and MSAN annotations are already a
> part of the zlib-ng code base. So I would prefer to keep them for
> consistency.
Hm, I didn't realize this code is being taken from elsewhere.
If so, maybe we should come up with an annotation that can be
contributed to zlib-ng, so that it doesn't cause merge conflicts every
time Mikhail is doing an update?
(leaving this up to you to decide).
If you decide to go with the current solution, please consider adding
an #include for kmsan-checks.h, which introduces
kmsan_unpoison_memory().
> A problem with __memset() is that, at least for me, it always ends
> up being a call. There is a use case where we need to write only 1
> byte, so I thought that introducing a call there (when compiling
> without KMSAN) would be unacceptable.
Wonder what happens with that use case if we e.g. build with fortify-source.
Calling memset() for a single byte might be indicating the code is not hot.
> > ...
> >
> > > +__no_sanitize_memory
> > > +static inline void *memset_no_sanitize_memory(void *s, int c,
> > > size_t n)
> > > +{
> > > + return memset(s, c, n);
> > > +}
> >
> > I think depending on the compiler optimizations this might end up
> > being a call to normal memset, that would still change the shadow
> > bytes.
>
> Interesting, do you have some specific scenario in mind? I vaguely
> remember that in the past there were cases when sanitizer annotations
> were lost after inlining, but I thought they were sorted out?
Sanitizer annotations are indeed lost after inlining, and we cannot do
much about that.
They are implemented using function attributes, and if a function
dissolves after inlining, we cannot possibly know which instructions
belonged to it.
Consider the following example (also available at
https://godbolt.org/z/5r7817G8e):
==================================
void *kmalloc(int size);
__attribute__((no_sanitize("kernel-memory")))
__attribute__((always_inline))
static void *memset_nosanitize(void *s, int c, int n) {
return __builtin_memset(s, c, n);
}
void *do_something_nosanitize(int size) {
void *ptr = kmalloc(size);
memset_nosanitize(ptr, 0, size);
return ptr;
}
void *do_something_sanitize(int size) {
void *ptr = kmalloc(size);
__builtin_memset(ptr, 0, size);
return ptr;
}
==================================
If memset_nosanitize() has __attribute__((always_inline)), the
compiler generates the same LLVM IR calling __msan_memset() for both
do_something_nosanitize() and do_something_sanitize().
If we comment out this attribute, do_something_nosanitize() calls
memset_nosanitize(), which doesn't have the sanitize_memory attribute.
But even now __builtin_memset() is still calling __msan_memset(),
because __attribute__((no_sanitize("kernel-memory"))) somewhat
counterintuitively still preserves some instrumentation (see
include/linux/compiler-clang.h for details).
Replacing __attribute__((no_sanitize("kernel-memory"))) with
__attribute__((disable_sanitizer_instrumentation)) fixes this
situation:
define internal fastcc noundef ptr @memset_nosanitize(void*, int,
int)(ptr noundef returned writeonly %s, i32 noundef %n) unnamed_addr
#2 {
entry:
%conv = sext i32 %n to i64
tail call void @llvm.memset.p0.i64(ptr align 1 %s, i8 0, i64 %conv, i1 false)
ret ptr %s
}
>
> And, in any case, if this were to happen, would not it be considered a
> compiler bug that needs fixing there, and not in the kernel?
As stated above, I don't think this is more or less working as intended.
If we really want the ability to inline __memset(), we could transform
it into memset() in non-sanitizer builds, but perhaps having a call is
also acceptable?
On Tue, Nov 21, 2023 at 11:07 PM Ilya Leoshkevich <[email protected]> wrote:
>
> The inline assembly block in s390's chsc() stores that much.
>
> Signed-off-by: Ilya Leoshkevich <[email protected]>
Reviewed-by: Alexander Potapenko <[email protected]>
On Tue, Nov 21, 2023 at 11:07 PM Ilya Leoshkevich <[email protected]> wrote:
>
> It is useful to manually copy metadata in order to describe the effects
> of memmove()-like logic in uninstrumented code or inline asm. Introduce
> kmsan_memmove_metadata() for this purpose.
>
> Signed-off-by: Ilya Leoshkevich <[email protected]>
> ---
> include/linux/kmsan-checks.h | 14 ++++++++++++++
> mm/kmsan/hooks.c | 11 +++++++++++
> 2 files changed, 25 insertions(+)
>
> diff --git a/include/linux/kmsan-checks.h b/include/linux/kmsan-checks.h
> index c4cae333deec..5218973f0ad0 100644
> --- a/include/linux/kmsan-checks.h
> +++ b/include/linux/kmsan-checks.h
> @@ -61,6 +61,17 @@ void kmsan_check_memory(const void *address, size_t size);
> void kmsan_copy_to_user(void __user *to, const void *from, size_t to_copy,
> size_t left);
>
> +/**
> + * kmsan_memmove_metadata() - Copy kernel memory range metadata.
> + * @dst: start of the destination kernel memory range.
> + * @src: start of the source kernel memory range.
> + * @n: size of the memory ranges.
> + *
> + * KMSAN will treat the destination range as if its contents were memmove()d
> + * from the source range.
> + */
> +void kmsan_memmove_metadata(void *dst, const void *src, size_t n);
As noted in patch 18/33, I am pretty sure we shouldn't need this function.
On Tue, Nov 21, 2023 at 11:07 PM Ilya Leoshkevich <[email protected]> wrote:
>
> The pages for the KMSAN metadata associated with most kernel mappings
> are taken from memblock by the common code. However, vmalloc and module
> metadata needs to be defined by the architectures.
>
> Be a little bit more careful than x86: allocate exactly MODULES_LEN
> for the module shadow and origins, and then take 2/3 of vmalloc for
> the vmalloc shadow and origins. This ensures that users passing small
> vmalloc= values on the command line do not cause module metadata
> collisions.
>
> Signed-off-by: Ilya Leoshkevich <[email protected]>
Reviewed-by: Alexander Potapenko <[email protected]>
(hope some s390 maintainer acks this as well)
On Tue, Nov 21, 2023 at 11:03 PM Ilya Leoshkevich <[email protected]> wrote:
>
> put_user() uses inline assembly with precise constraints, so Clang is
> in principle capable of instrumenting it automatically. Unfortunately,
> one of the constraints contains a dereferenced user pointer, and Clang
> does not currently distinguish user and kernel pointers. Therefore
> KMSAN attempts to access shadow for user pointers, which is not a right
> thing to do.
>
> An obvious fix to add __no_sanitize_memory to __put_user_fn() does not
> work, since it's __always_inline. And __always_inline cannot be removed
> due to the __put_user_bad() trick.
>
> A different obvious fix of using the "a" instead of the "+Q" constraint
> degrades the code quality, which is very important here, since it's a
> hot path.
>
> Instead, repurpose the __put_user_asm() macro to define
> __put_user_{char,short,int,long}_noinstr() functions and mark them with
> __no_sanitize_memory. For the non-KMSAN builds make them
> __always_inline in order to keep the generated code quality. Also
> define __put_user_{char,short,int,long}() functions, which call the
> aforementioned ones and which *are* instrumented, because they call
> KMSAN hooks, which may be implemented as macros.
>
> The same applies to get_user() as well.
>
> Acked-by: Heiko Carstens <[email protected]>
> Signed-off-by: Ilya Leoshkevich <[email protected]>
I think this patch makes sense, but I don't feel myself qualified
enough to stamp it. Hope Heiko's ack is enough.
On Tue, Nov 21, 2023 at 11:03 PM Ilya Leoshkevich <[email protected]> wrote:
>
> Add KMSAN support for the s390 implementations of the string functions.
> Do this similar to how it's already done for KASAN, except that the
> optimized memset{16,32,64}() functions need to be disabled: it's
> important for KMSAN to know that they initialized something.
>
> The way boot code is built with regard to string functions is
> problematic, since most files think it's configured with sanitizers,
> but boot/string.c doesn't. This creates various problems with the
> memset64() definitions, depending on whether the code is built with
> sanitizers or fortify. This should probably be streamlined, but in the
> meantime resolve the issues by introducing the IN_BOOT_STRING_C macro,
> similar to the existing IN_ARCH_STRING_C macro.
>
> Signed-off-by: Ilya Leoshkevich <[email protected]>
Reviewed-by: Alexander Potapenko <[email protected]>
On Tue, Nov 21, 2023 at 11:06 PM Ilya Leoshkevich <[email protected]> wrote:
>
> Like for KASAN, it's useful to temporarily disable KMSAN checks around,
> e.g., redzone accesses. Introduce kmsan_disable_current() and
> kmsan_enable_current(), which are similar to their KASAN counterparts.
Initially we used to have this disablement counter in KMSAN, but
adding it uncontrollably can result in KMSAN not functioning properly.
E.g. forgetting to call kmsan_disable_current() or underflowing the
counter will break reporting.
We'd better put this API in include/linux/kmsan.h to indicate it
should be discouraged.
> Even though it's not strictly necessary, make them reentrant, in order
> to match the KASAN behavior.
Until this becomes strictly necessary, I think we'd better
KMSAN_WARN_ON if the counter is re-entered.
On Fri, 2023-12-08 at 16:25 +0100, Alexander Potapenko wrote:
> > A problem with __memset() is that, at least for me, it always ends
> > up being a call. There is a use case where we need to write only 1
> > byte, so I thought that introducing a call there (when compiling
> > without KMSAN) would be unacceptable.
>
> Wonder what happens with that use case if we e.g. build with fortify-
> source.
> Calling memset() for a single byte might be indicating the code is
> not hot.
The original code has a simple assignment. Here is the relevant diff:
if (s->flags & __OBJECT_POISON) {
- memset(p, POISON_FREE, poison_size - 1);
- p[poison_size - 1] = POISON_END;
+ memset_no_sanitize_memory(p, POISON_FREE, poison_size -
1);
+ memset_no_sanitize_memory(p + poison_size - 1,
POISON_END, 1);
}
[...]
> As stated above, I don't think this is more or less working as
> intended.
> If we really want the ability to inline __memset(), we could
> transform
> it into memset() in non-sanitizer builds, but perhaps having a call
> is
> also acceptable?
Thanks for the detailed explanation and analysis. I will post
a version with a __memset() and let the slab maintainers decide if
the additional overhead is acceptable.
On Wed, 2023-12-13 at 02:31 +0100, Ilya Leoshkevich wrote:
> On Fri, 2023-12-08 at 16:25 +0100, Alexander Potapenko wrote:
> > > A problem with __memset() is that, at least for me, it always
> > > ends
> > > up being a call. There is a use case where we need to write only
> > > 1
> > > byte, so I thought that introducing a call there (when compiling
> > > without KMSAN) would be unacceptable.
[...]
> > As stated above, I don't think this is more or less working as
> > intended.
> > If we really want the ability to inline __memset(), we could
> > transform
> > it into memset() in non-sanitizer builds, but perhaps having a call
> > is
> > also acceptable?
>
> Thanks for the detailed explanation and analysis. I will post
> a version with a __memset() and let the slab maintainers decide if
> the additional overhead is acceptable.
I noticed I had the same problem in the get_user()/put_user() and
check_canary() patches.
The annotation being silently ignored is never what a programmer
intends, so what do you think about adding noinline to
__no_kmsan_checks and __no_sanitize_memory?
On Mon, 2023-12-11 at 12:50 +0100, Alexander Potapenko wrote:
> On Tue, Nov 21, 2023 at 11:06 PM Ilya Leoshkevich <[email protected]>
> wrote:
> >
> > Like for KASAN, it's useful to temporarily disable KMSAN checks
> > around,
> > e.g., redzone accesses. Introduce kmsan_disable_current() and
> > kmsan_enable_current(), which are similar to their KASAN
> > counterparts.
>
> Initially we used to have this disablement counter in KMSAN, but
> adding it uncontrollably can result in KMSAN not functioning
> properly.
> E.g. forgetting to call kmsan_disable_current() or underflowing the
> counter will break reporting.
> We'd better put this API in include/linux/kmsan.h to indicate it
> should be discouraged.
>
> > Even though it's not strictly necessary, make them reentrant, in
> > order
> > to match the KASAN behavior.
>
> Until this becomes strictly necessary, I think we'd better
> KMSAN_WARN_ON if the counter is re-entered.
I encountered a case when we are freeing memory from an interrupt
handler:
[ 149.840553] ------------[ cut here ]------------
[ 149.840649] WARNING: CPU: 1 PID: 181 at mm/kmsan/hooks.c:447
kmsan_disable_current+0x2e/0x40
[ 149.840790] Modules linked in:
[ 149.840894] CPU: 1 PID: 181 Comm: (direxec) Tainted: G B W
N 6.7.0-rc5-gd34a4b46f382 #13
[ 149.841003] Hardware name: IBM 3931 A01 704 (KVM/Linux)
[ 149.841094] Krnl PSW : 0404c00180000000 000000000197dbc2
(kmsan_disable_current+0x32/0x40)
[ 149.841276] R:0 T:1 IO:0 EX:0 Key:0 M:1 W:0 P:0 AS:3 CC:0
PM:0 RI:0 EA:3
[ 149.841420] Krnl GPRS: 0000000000000040 0000000096914100
0000000000001000 0000000000000001
[ 149.841518] 0000036d827daee0 0000000007c97008
0000000080096500 0000000092f4f000
[ 149.841617] 0000036d00000000 0000000000000000
0000000000000040 0000000000000000
[ 149.841712] 0000000092f4efc0 00000001ff710f60
000000000193acba 0000037f0008f710
[ 149.841893] Krnl Code: 000000000197dbb6: eb0018640352 mviy
14436(%r1),0
[ 149.841893] 000000000197dbbc: 07fe bcr
15,%r14
[ 149.841893] #000000000197dbbe: af000000 mc
0,0
[ 149.841893] >000000000197dbc2: a7f4fffa brc
15,000000000197dbb6
[ 149.841893] 000000000197dbc6: 0700 bcr
0,%r0
[ 149.841893] 000000000197dbc8: 0700 bcr
0,%r0
[ 149.841893] 000000000197dbca: 0700 bcr
0,%r0
[ 149.841893] 000000000197dbcc: 0700 bcr
0,%r0
[ 149.842438] Call Trace:
15:37:25 [90/1838]
[ 149.842510] [<000000000197dbc2>] kmsan_disable_current+0x32/0x40
[ 149.842631] ([<000000000193ac14>] slab_pad_check+0x1d4/0xac0)
[ 149.842738] [<0000000001949222>] free_to_partial_list+0x1d72/0x3b80
[ 149.842850] [<0000000001947066>] __slab_free+0xd86/0x11d0
[ 149.842956] [<00000000019111e8>] kmem_cache_free+0x15d8/0x25d0
[ 149.843062] [<0000000000229e3a>] __tlb_remove_table+0x20a/0xa50
[ 149.843174] [<00000000016c7f98>] tlb_remove_table_rcu+0x98/0x120
[ 149.843291] [<000000000083e1c6>] rcu_core+0x15b6/0x54b0
[ 149.843406] [<00000000069c3c0e>] __do_softirq+0xa1e/0x2178
[ 149.843514] [<00000000003467b4>] irq_exit_rcu+0x2c4/0x630
[ 149.843623] [<0000000006949f6e>] do_ext_irq+0x9e/0x120
[ 149.843736] [<00000000069c18d4>] ext_int_handler+0xc4/0xf0
[ 149.843841] [<000000000197e428>] kmsan_get_metadata+0x68/0x280
[ 149.843950] [<000000000197e344>]
kmsan_get_shadow_origin_ptr+0x74/0xf0
[ 149.844071] [<000000000197ba3a>]
__msan_metadata_ptr_for_load_8+0x2a/0x40
[ 149.844192] [<0000000000184e4a>]
unwind_get_return_address+0xda/0x150
[ 149.844313] [<000000000018fd12>] arch_stack_walk+0x172/0x2f0
[ 149.844417] [<00000000008f1af0>] stack_trace_save+0x100/0x160
[ 149.844529] [<000000000197af22>]
kmsan_internal_chain_origin+0x62/0xe0
[ 149.844647] [<000000000197c1f0>] __msan_chain_origin+0xd0/0x160
[ 149.844763] [<00000000068b3ba4>] memchr_inv+0x5b4/0xb20
[ 149.844877] [<000000000193e730>] check_bytes_and_report+0xa0/0xd30
[ 149.844986] [<000000000193b920>] check_object+0x420/0x17d0
[ 149.845092] [<000000000194aa8a>] free_to_partial_list+0x35da/0x3b80
[ 149.845202] [<0000000001947066>] __slab_free+0xd86/0x11d0
[ 149.845308] [<00000000019111e8>] kmem_cache_free+0x15d8/0x25d0
[ 149.845414] [<00000000016bc2fe>] exit_mmap+0x87e/0x1200
[ 149.845524] [<00000000002f315c>] mmput+0x13c/0x5b0
[ 149.845632] [<0000000001b9d634>] exec_mmap+0xc34/0x1230
[ 149.845744] [<0000000001b996c2>] begin_new_exec+0xcf2/0x2520
[ 149.845857] [<0000000001f6a084>] load_elf_binary+0x2364/0x67d0
[ 149.845971] [<0000000001ba5ba4>] bprm_execve+0x25b4/0x4010
[ 149.846083] [<0000000001baa7e6>] do_execveat_common+0x2436/0x2600
[ 149.846200] [<0000000001ba78f8>] __s390x_sys_execve+0x108/0x140
[ 149.846314] [<000000000011b192>] do_syscall+0x4c2/0x690
[ 149.846424] [<0000000006949d78>] __do_syscall+0x98/0xe0
[ 149.846536] [<00000000069c1640>] system_call+0x70/0xa0
[ 149.846638] INFO: lockdep is turned off.
[ 149.846846] Last Breaking-Event-Address:
[ 149.846916] [<000000000197dbb2>] kmsan_disable_current+0x22/0x40
[ 149.847057] irq event stamp: 0
[ 149.847128] hardirqs last enabled at (0): [<0000000000000000>] 0x0
[ 149.847227] hardirqs last disabled at (0): [<00000000002f8f46>]
copy_process+0x21f6/0x8b20
[ 149.847344] softirqs last enabled at (0): [<00000000002f8f80>]
copy_process+0x2230/0x8b20
[ 149.847461] softirqs last disabled at (0): [<0000000000000000>] 0x0
[ 149.847559] ---[ end trace 0000000000000000 ]---
[ 149.865485] =====================================================
Using a counter resolves this issue, but, of course, at the expense of
not reporting valid issues in the interrupt handler.
Unfortunately I don't see another easy way to solve this problem. The
possibilities that come to mind are providing uninstrumented
memchr_inv() or disablement flags for each context, but I'm not sure if
we want to go there, especially since KASAN already has this
limitation.