linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] UML: add support for KASAN under x86_64
@ 2020-02-26  0:46 Patricia Alfonso
  2020-02-26  1:19 ` Brendan Higgins
                   ` (2 more replies)
  0 siblings, 3 replies; 38+ messages in thread
From: Patricia Alfonso @ 2020-02-26  0:46 UTC (permalink / raw)
  To: jdike, richard, anton.ivanov, aryabinin, dvyukov, brendanhiggins,
	davidgow, johannes
  Cc: kasan-dev, linux-kernel, linux-um, Patricia Alfonso

Make KASAN run on User Mode Linux on x86_64.

Depends on Constructor support in UML - "[RFC PATCH] um:
implement CONFIG_CONSTRUCTORS for modules"
(https://patchwork.ozlabs.org/patch/1234551/) by Johannes Berg.

The location of the KASAN shadow memory, starting at
KASAN_SHADOW_OFFSET, can be configured using the
KASAN_SHADOW_OFFSET option. UML uses roughly 18TB of address
space, and KASAN requires 1/8th of this. The default location of
this offset is 0x7fff8000 as suggested by Dmitry Vyukov. There is
usually enough free space at this location; however, it is a config
option so that it can be easily changed if needed.

The UML-specific KASAN initializer uses mmap to map
the roughly 2.25TB of shadow memory to the location defined by
KASAN_SHADOW_OFFSET. kasan_init() utilizes constructors to initialize
KASAN before main().

Disable stack instrumentation on UML via KASAN_STACK config option to
avoid false positive KASAN reports.

Signed-off-by: Patricia Alfonso <trishalfonso@google.com>
---
 arch/um/Kconfig                  | 13 +++++++++++++
 arch/um/Makefile                 |  6 ++++++
 arch/um/include/asm/common.lds.S |  1 +
 arch/um/include/asm/kasan.h      | 32 ++++++++++++++++++++++++++++++++
 arch/um/kernel/dyn.lds.S         |  5 ++++-
 arch/um/kernel/mem.c             | 18 ++++++++++++++++++
 arch/um/os-Linux/mem.c           | 22 ++++++++++++++++++++++
 arch/um/os-Linux/user_syms.c     |  4 ++--
 arch/x86/um/Makefile             |  3 ++-
 arch/x86/um/vdso/Makefile        |  3 +++
 lib/Kconfig.kasan                |  2 +-
 11 files changed, 104 insertions(+), 5 deletions(-)
 create mode 100644 arch/um/include/asm/kasan.h

diff --git a/arch/um/Kconfig b/arch/um/Kconfig
index 0917f8443c28..fb2ad1fb05fd 100644
--- a/arch/um/Kconfig
+++ b/arch/um/Kconfig
@@ -8,6 +8,7 @@ config UML
 	select ARCH_HAS_KCOV
 	select ARCH_NO_PREEMPT
 	select HAVE_ARCH_AUDITSYSCALL
+	select HAVE_ARCH_KASAN if X86_64
 	select HAVE_ARCH_SECCOMP_FILTER
 	select HAVE_ASM_MODVERSIONS
 	select HAVE_UID16
@@ -200,6 +201,18 @@ config UML_TIME_TRAVEL_SUPPORT
 
 	  It is safe to say Y, but you probably don't need this.
 
+config KASAN_SHADOW_OFFSET
+	hex
+	depends on KASAN
+	default 0x7fff8000
+	help
+	  This is the offset at which the ~2.25TB of shadow memory is
+	  mapped and used by KASAN for memory debugging. This can be any
+	  address that has at least KASAN_SHADOW_SIZE(total address space divided
+	  by 8) amount of space so that the KASAN shadow memory does not conflict
+	  with anything. The default is 0x7fff8000, as it fits into immediate of
+	  most instructions.
+
 endmenu
 
 source "arch/um/drivers/Kconfig"
diff --git a/arch/um/Makefile b/arch/um/Makefile
index d2daa206872d..28fe7a9a1858 100644
--- a/arch/um/Makefile
+++ b/arch/um/Makefile
@@ -75,6 +75,12 @@ USER_CFLAGS = $(patsubst $(KERNEL_DEFINES),,$(patsubst -I%,,$(KBUILD_CFLAGS))) \
 		-D_FILE_OFFSET_BITS=64 -idirafter $(srctree)/include \
 		-idirafter $(objtree)/include -D__KERNEL__ -D__UM_HOST__
 
+# Kernel config options are not included in USER_CFLAGS, but the option for KASAN
+# should be included if the KASAN config option was set.
+ifdef CONFIG_KASAN
+	USER_CFLAGS+=-DCONFIG_KASAN=y
+endif
+
 #This will adjust *FLAGS accordingly to the platform.
 include $(ARCH_DIR)/Makefile-os-$(OS)
 
diff --git a/arch/um/include/asm/common.lds.S b/arch/um/include/asm/common.lds.S
index eca6c452a41b..731f8c8422a2 100644
--- a/arch/um/include/asm/common.lds.S
+++ b/arch/um/include/asm/common.lds.S
@@ -83,6 +83,7 @@
   }
   .init_array : {
 	__init_array_start = .;
+	*(.kasan_init)
 	*(.init_array)
 	__init_array_end = .;
   }
diff --git a/arch/um/include/asm/kasan.h b/arch/um/include/asm/kasan.h
new file mode 100644
index 000000000000..2b81e7bcd4af
--- /dev/null
+++ b/arch/um/include/asm/kasan.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ASM_UM_KASAN_H
+#define __ASM_UM_KASAN_H
+
+#include <linux/init.h>
+#include <linux/const.h>
+
+#define KASAN_SHADOW_OFFSET _AC(CONFIG_KASAN_SHADOW_OFFSET, UL)
+
+/* used in kasan_mem_to_shadow to divide by 8 */
+#define KASAN_SHADOW_SCALE_SHIFT 3
+
+#ifdef CONFIG_X86_64
+#define KASAN_HOST_USER_SPACE_END_ADDR 0x00007fffffffffffUL
+/* KASAN_SHADOW_SIZE is the size of total address space divided by 8 */
+#define KASAN_SHADOW_SIZE ((KASAN_HOST_USER_SPACE_END_ADDR + 1) >> \
+			KASAN_SHADOW_SCALE_SHIFT)
+#else
+#error "KASAN_SHADOW_SIZE is not defined for this sub-architecture"
+#endif /* CONFIG_X86_64 */
+
+#define KASAN_SHADOW_START (KASAN_SHADOW_OFFSET)
+#define KASAN_SHADOW_END (KASAN_SHADOW_START + KASAN_SHADOW_SIZE)
+
+#ifdef CONFIG_KASAN
+void kasan_init(void);
+void kasan_map_memory(void *start, unsigned long len);
+#else
+static inline void kasan_init(void) { }
+#endif /* CONFIG_KASAN */
+
+#endif /* __ASM_UM_KASAN_H */
diff --git a/arch/um/kernel/dyn.lds.S b/arch/um/kernel/dyn.lds.S
index f5001481010c..d91bdb2c3143 100644
--- a/arch/um/kernel/dyn.lds.S
+++ b/arch/um/kernel/dyn.lds.S
@@ -103,7 +103,10 @@ SECTIONS
      be empty, which isn't pretty.  */
   . = ALIGN(32 / 8);
   .preinit_array     : { *(.preinit_array) }
-  .init_array     : { *(.init_array) }
+  .init_array     : {
+    *(.kasan_init)
+    *(.init_array)
+  }
   .fini_array     : { *(.fini_array) }
   .data           : {
     INIT_TASK_DATA(KERNEL_STACK_SIZE)
diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c
index 30885d0b94ac..7b0d028aa079 100644
--- a/arch/um/kernel/mem.c
+++ b/arch/um/kernel/mem.c
@@ -18,6 +18,24 @@
 #include <kern_util.h>
 #include <mem_user.h>
 #include <os.h>
+#include <linux/sched/task.h>
+
+#ifdef CONFIG_KASAN
+void kasan_init(void)
+{
+	/*
+	 * kasan_map_memory will map all of the required address space and
+	 * the host machine will allocate physical memory as necessary.
+	 */
+	kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE);
+	init_task.kasan_depth = 0;
+	os_info("KernelAddressSanitizer initialized\n");
+}
+
+static void (*kasan_init_ptr)(void)
+__section(.kasan_init) __used
+= kasan_init;
+#endif
 
 /* allocated in paging_init, zeroed in mem_init, and unchanged thereafter */
 unsigned long *empty_zero_page = NULL;
diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c
index 3c1b77474d2d..8530b2e08604 100644
--- a/arch/um/os-Linux/mem.c
+++ b/arch/um/os-Linux/mem.c
@@ -17,6 +17,28 @@
 #include <init.h>
 #include <os.h>
 
+/*
+ * kasan_map_memory - maps memory from @start with a size of @len.
+ * The allocated memory is filled with zeroes upon success.
+ * @start: the start address of the memory to be mapped
+ * @len: the length of the memory to be mapped
+ *
+ * This function is used to map shadow memory for KASAN in uml
+ */
+void kasan_map_memory(void *start, size_t len)
+{
+	if (mmap(start,
+		 len,
+		 PROT_READ|PROT_WRITE,
+		 MAP_FIXED|MAP_ANONYMOUS|MAP_PRIVATE|MAP_NORESERVE,
+		 -1,
+		 0) == MAP_FAILED) {
+		os_info("Couldn't allocate shadow memory: %s\n.",
+			strerror(errno));
+		exit(1);
+	}
+}
+
 /* Set by make_tempfile() during early boot. */
 static char *tempdir = NULL;
 
diff --git a/arch/um/os-Linux/user_syms.c b/arch/um/os-Linux/user_syms.c
index 715594fe5719..cb667c9225ab 100644
--- a/arch/um/os-Linux/user_syms.c
+++ b/arch/um/os-Linux/user_syms.c
@@ -27,10 +27,10 @@ EXPORT_SYMBOL(strstr);
 #ifndef __x86_64__
 extern void *memcpy(void *, const void *, size_t);
 EXPORT_SYMBOL(memcpy);
-#endif
-
 EXPORT_SYMBOL(memmove);
 EXPORT_SYMBOL(memset);
+#endif
+
 EXPORT_SYMBOL(printf);
 
 /* Here, instead, I can provide a fake prototype. Yes, someone cares: genksyms.
diff --git a/arch/x86/um/Makefile b/arch/x86/um/Makefile
index 33c51c064c77..7dbd76c546fe 100644
--- a/arch/x86/um/Makefile
+++ b/arch/x86/um/Makefile
@@ -26,7 +26,8 @@ else
 
 obj-y += syscalls_64.o vdso/
 
-subarch-y = ../lib/csum-partial_64.o ../lib/memcpy_64.o ../entry/thunk_64.o
+subarch-y = ../lib/csum-partial_64.o ../lib/memcpy_64.o ../entry/thunk_64.o \
+	../lib/memmove_64.o ../lib/memset_64.o
 
 endif
 
diff --git a/arch/x86/um/vdso/Makefile b/arch/x86/um/vdso/Makefile
index 0caddd6acb22..450efa0fb694 100644
--- a/arch/x86/um/vdso/Makefile
+++ b/arch/x86/um/vdso/Makefile
@@ -3,6 +3,9 @@
 # Building vDSO images for x86.
 #
 
+# do not instrument on vdso because KASAN is not compatible with user mode
+KASAN_SANITIZE			:= n
+
 # Prevents link failures: __sanitizer_cov_trace_pc() is not linked in.
 KCOV_INSTRUMENT                := n
 
diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index 81f5464ea9e1..5b54f3c9a741 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -125,7 +125,7 @@ config KASAN_STACK_ENABLE
 
 config KASAN_STACK
 	int
-	default 1 if KASAN_STACK_ENABLE || CC_IS_GCC
+	default 1 if (KASAN_STACK_ENABLE || CC_IS_GCC) && !UML
 	default 0
 
 config KASAN_S390_4_LEVEL_PAGING
-- 
2.25.0.265.gbab2e86ba0-goog


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-02-26  0:46 [PATCH] UML: add support for KASAN under x86_64 Patricia Alfonso
@ 2020-02-26  1:19 ` Brendan Higgins
  2020-02-26 15:24 ` Dmitry Vyukov
  2020-03-06  0:03 ` Patricia Alfonso
  2 siblings, 0 replies; 38+ messages in thread
From: Brendan Higgins @ 2020-02-26  1:19 UTC (permalink / raw)
  To: Patricia Alfonso
  Cc: Jeff Dike, Richard Weinberger, Anton Ivanov, aryabinin,
	Dmitry Vyukov, David Gow, Johannes Berg, kasan-dev,
	Linux Kernel Mailing List, linux-um

On Tue, Feb 25, 2020 at 4:46 PM Patricia Alfonso
<trishalfonso@google.com> wrote:
>
> Make KASAN run on User Mode Linux on x86_64.
>
> Depends on Constructor support in UML - "[RFC PATCH] um:
> implement CONFIG_CONSTRUCTORS for modules"
> (https://patchwork.ozlabs.org/patch/1234551/) by Johannes Berg.
>
> The location of the KASAN shadow memory, starting at
> KASAN_SHADOW_OFFSET, can be configured using the
> KASAN_SHADOW_OFFSET option. UML uses roughly 18TB of address
> space, and KASAN requires 1/8th of this. The default location of
> this offset is 0x7fff8000 as suggested by Dmitry Vyukov. There is
> usually enough free space at this location; however, it is a config
> option so that it can be easily changed if needed.
>
> The UML-specific KASAN initializer uses mmap to map
> the roughly 2.25TB of shadow memory to the location defined by
> KASAN_SHADOW_OFFSET. kasan_init() utilizes constructors to initialize
> KASAN before main().
>
> Disable stack instrumentation on UML via KASAN_STACK config option to
> avoid false positive KASAN reports.
>
> Signed-off-by: Patricia Alfonso <trishalfonso@google.com>

A couple of minor nits (well one nit and one question), but overall
this looks good to me.

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

> ---
>  arch/um/Kconfig                  | 13 +++++++++++++
>  arch/um/Makefile                 |  6 ++++++
>  arch/um/include/asm/common.lds.S |  1 +
>  arch/um/include/asm/kasan.h      | 32 ++++++++++++++++++++++++++++++++
>  arch/um/kernel/dyn.lds.S         |  5 ++++-
>  arch/um/kernel/mem.c             | 18 ++++++++++++++++++
>  arch/um/os-Linux/mem.c           | 22 ++++++++++++++++++++++
>  arch/um/os-Linux/user_syms.c     |  4 ++--
>  arch/x86/um/Makefile             |  3 ++-
>  arch/x86/um/vdso/Makefile        |  3 +++
>  lib/Kconfig.kasan                |  2 +-
>  11 files changed, 104 insertions(+), 5 deletions(-)
>  create mode 100644 arch/um/include/asm/kasan.h
>
> diff --git a/arch/um/Kconfig b/arch/um/Kconfig
> index 0917f8443c28..fb2ad1fb05fd 100644
> --- a/arch/um/Kconfig
> +++ b/arch/um/Kconfig
> @@ -8,6 +8,7 @@ config UML
>         select ARCH_HAS_KCOV
>         select ARCH_NO_PREEMPT
>         select HAVE_ARCH_AUDITSYSCALL
> +       select HAVE_ARCH_KASAN if X86_64
>         select HAVE_ARCH_SECCOMP_FILTER
>         select HAVE_ASM_MODVERSIONS
>         select HAVE_UID16
> @@ -200,6 +201,18 @@ config UML_TIME_TRAVEL_SUPPORT
>
>           It is safe to say Y, but you probably don't need this.
>
> +config KASAN_SHADOW_OFFSET
> +       hex
> +       depends on KASAN
> +       default 0x7fff8000

nit: It looks like you chose the default that Dmitry suggested. Some
explanation of this in the help would probably be good.

> +       help
> +         This is the offset at which the ~2.25TB of shadow memory is
> +         mapped and used by KASAN for memory debugging. This can be any
> +         address that has at least KASAN_SHADOW_SIZE(total address space divided
> +         by 8) amount of space so that the KASAN shadow memory does not conflict
> +         with anything. The default is 0x7fff8000, as it fits into immediate of
> +         most instructions.
> +
>  endmenu

[...]

> diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> index 81f5464ea9e1..5b54f3c9a741 100644
> --- a/lib/Kconfig.kasan
> +++ b/lib/Kconfig.kasan
> @@ -125,7 +125,7 @@ config KASAN_STACK_ENABLE
>
>  config KASAN_STACK
>         int
> -       default 1 if KASAN_STACK_ENABLE || CC_IS_GCC
> +       default 1 if (KASAN_STACK_ENABLE || CC_IS_GCC) && !UML

Up to the KASAN people, but I think you can probably move this to
arch/um/Kconfig. There is some advantage to having all the UML
specific Kconfigery in arch/um/Kconfig, but there are also already a
lot of things that specify !UML outside of arch/um/.

>         default 0
>
>  config KASAN_S390_4_LEVEL_PAGING
> --
> 2.25.0.265.gbab2e86ba0-goog
>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-02-26  0:46 [PATCH] UML: add support for KASAN under x86_64 Patricia Alfonso
  2020-02-26  1:19 ` Brendan Higgins
@ 2020-02-26 15:24 ` Dmitry Vyukov
  2020-03-06  0:03 ` Patricia Alfonso
  2 siblings, 0 replies; 38+ messages in thread
From: Dmitry Vyukov @ 2020-02-26 15:24 UTC (permalink / raw)
  To: Patricia Alfonso
  Cc: Jeff Dike, Richard Weinberger, anton.ivanov, Andrey Ryabinin,
	Brendan Higgins, David Gow, Johannes Berg, kasan-dev, LKML,
	linux-um

On Wed, Feb 26, 2020 at 1:46 AM Patricia Alfonso
<trishalfonso@google.com> wrote:
>
> Make KASAN run on User Mode Linux on x86_64.
>
> Depends on Constructor support in UML - "[RFC PATCH] um:
> implement CONFIG_CONSTRUCTORS for modules"
> (https://patchwork.ozlabs.org/patch/1234551/) by Johannes Berg.
>
> The location of the KASAN shadow memory, starting at
> KASAN_SHADOW_OFFSET, can be configured using the
> KASAN_SHADOW_OFFSET option. UML uses roughly 18TB of address
> space, and KASAN requires 1/8th of this. The default location of
> this offset is 0x7fff8000 as suggested by Dmitry Vyukov. There is
> usually enough free space at this location; however, it is a config
> option so that it can be easily changed if needed.
>
> The UML-specific KASAN initializer uses mmap to map
> the roughly 2.25TB of shadow memory to the location defined by
> KASAN_SHADOW_OFFSET. kasan_init() utilizes constructors to initialize
> KASAN before main().
>
> Disable stack instrumentation on UML via KASAN_STACK config option to
> avoid false positive KASAN reports.

This now looks much better, cleaner, nicer and simpler.
There are few UML-specific things I did not understand, but I will
leave them to UML reviewers.
For KASAN-specifics:

Reviewed-by: Dmitry Vyukov <dvyukov@google.com>

Thanks!

> Signed-off-by: Patricia Alfonso <trishalfonso@google.com>
> ---
>  arch/um/Kconfig                  | 13 +++++++++++++
>  arch/um/Makefile                 |  6 ++++++
>  arch/um/include/asm/common.lds.S |  1 +
>  arch/um/include/asm/kasan.h      | 32 ++++++++++++++++++++++++++++++++
>  arch/um/kernel/dyn.lds.S         |  5 ++++-
>  arch/um/kernel/mem.c             | 18 ++++++++++++++++++
>  arch/um/os-Linux/mem.c           | 22 ++++++++++++++++++++++
>  arch/um/os-Linux/user_syms.c     |  4 ++--
>  arch/x86/um/Makefile             |  3 ++-
>  arch/x86/um/vdso/Makefile        |  3 +++
>  lib/Kconfig.kasan                |  2 +-
>  11 files changed, 104 insertions(+), 5 deletions(-)
>  create mode 100644 arch/um/include/asm/kasan.h
>
> diff --git a/arch/um/Kconfig b/arch/um/Kconfig
> index 0917f8443c28..fb2ad1fb05fd 100644
> --- a/arch/um/Kconfig
> +++ b/arch/um/Kconfig
> @@ -8,6 +8,7 @@ config UML
>         select ARCH_HAS_KCOV
>         select ARCH_NO_PREEMPT
>         select HAVE_ARCH_AUDITSYSCALL
> +       select HAVE_ARCH_KASAN if X86_64
>         select HAVE_ARCH_SECCOMP_FILTER
>         select HAVE_ASM_MODVERSIONS
>         select HAVE_UID16
> @@ -200,6 +201,18 @@ config UML_TIME_TRAVEL_SUPPORT
>
>           It is safe to say Y, but you probably don't need this.
>
> +config KASAN_SHADOW_OFFSET
> +       hex
> +       depends on KASAN
> +       default 0x7fff8000
> +       help
> +         This is the offset at which the ~2.25TB of shadow memory is
> +         mapped and used by KASAN for memory debugging. This can be any
> +         address that has at least KASAN_SHADOW_SIZE(total address space divided
> +         by 8) amount of space so that the KASAN shadow memory does not conflict
> +         with anything. The default is 0x7fff8000, as it fits into immediate of
> +         most instructions.
> +
>  endmenu
>
>  source "arch/um/drivers/Kconfig"
> diff --git a/arch/um/Makefile b/arch/um/Makefile
> index d2daa206872d..28fe7a9a1858 100644
> --- a/arch/um/Makefile
> +++ b/arch/um/Makefile
> @@ -75,6 +75,12 @@ USER_CFLAGS = $(patsubst $(KERNEL_DEFINES),,$(patsubst -I%,,$(KBUILD_CFLAGS))) \
>                 -D_FILE_OFFSET_BITS=64 -idirafter $(srctree)/include \
>                 -idirafter $(objtree)/include -D__KERNEL__ -D__UM_HOST__
>
> +# Kernel config options are not included in USER_CFLAGS, but the option for KASAN
> +# should be included if the KASAN config option was set.
> +ifdef CONFIG_KASAN
> +       USER_CFLAGS+=-DCONFIG_KASAN=y
> +endif
> +
>  #This will adjust *FLAGS accordingly to the platform.
>  include $(ARCH_DIR)/Makefile-os-$(OS)
>
> diff --git a/arch/um/include/asm/common.lds.S b/arch/um/include/asm/common.lds.S
> index eca6c452a41b..731f8c8422a2 100644
> --- a/arch/um/include/asm/common.lds.S
> +++ b/arch/um/include/asm/common.lds.S
> @@ -83,6 +83,7 @@
>    }
>    .init_array : {
>         __init_array_start = .;
> +       *(.kasan_init)
>         *(.init_array)
>         __init_array_end = .;
>    }
> diff --git a/arch/um/include/asm/kasan.h b/arch/um/include/asm/kasan.h
> new file mode 100644
> index 000000000000..2b81e7bcd4af
> --- /dev/null
> +++ b/arch/um/include/asm/kasan.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_UM_KASAN_H
> +#define __ASM_UM_KASAN_H
> +
> +#include <linux/init.h>
> +#include <linux/const.h>
> +
> +#define KASAN_SHADOW_OFFSET _AC(CONFIG_KASAN_SHADOW_OFFSET, UL)
> +
> +/* used in kasan_mem_to_shadow to divide by 8 */
> +#define KASAN_SHADOW_SCALE_SHIFT 3
> +
> +#ifdef CONFIG_X86_64
> +#define KASAN_HOST_USER_SPACE_END_ADDR 0x00007fffffffffffUL
> +/* KASAN_SHADOW_SIZE is the size of total address space divided by 8 */
> +#define KASAN_SHADOW_SIZE ((KASAN_HOST_USER_SPACE_END_ADDR + 1) >> \
> +                       KASAN_SHADOW_SCALE_SHIFT)
> +#else
> +#error "KASAN_SHADOW_SIZE is not defined for this sub-architecture"
> +#endif /* CONFIG_X86_64 */
> +
> +#define KASAN_SHADOW_START (KASAN_SHADOW_OFFSET)
> +#define KASAN_SHADOW_END (KASAN_SHADOW_START + KASAN_SHADOW_SIZE)
> +
> +#ifdef CONFIG_KASAN
> +void kasan_init(void);
> +void kasan_map_memory(void *start, unsigned long len);
> +#else
> +static inline void kasan_init(void) { }
> +#endif /* CONFIG_KASAN */
> +
> +#endif /* __ASM_UM_KASAN_H */
> diff --git a/arch/um/kernel/dyn.lds.S b/arch/um/kernel/dyn.lds.S
> index f5001481010c..d91bdb2c3143 100644
> --- a/arch/um/kernel/dyn.lds.S
> +++ b/arch/um/kernel/dyn.lds.S
> @@ -103,7 +103,10 @@ SECTIONS
>       be empty, which isn't pretty.  */
>    . = ALIGN(32 / 8);
>    .preinit_array     : { *(.preinit_array) }
> -  .init_array     : { *(.init_array) }
> +  .init_array     : {
> +    *(.kasan_init)
> +    *(.init_array)
> +  }
>    .fini_array     : { *(.fini_array) }
>    .data           : {
>      INIT_TASK_DATA(KERNEL_STACK_SIZE)
> diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c
> index 30885d0b94ac..7b0d028aa079 100644
> --- a/arch/um/kernel/mem.c
> +++ b/arch/um/kernel/mem.c
> @@ -18,6 +18,24 @@
>  #include <kern_util.h>
>  #include <mem_user.h>
>  #include <os.h>
> +#include <linux/sched/task.h>
> +
> +#ifdef CONFIG_KASAN
> +void kasan_init(void)
> +{
> +       /*
> +        * kasan_map_memory will map all of the required address space and
> +        * the host machine will allocate physical memory as necessary.
> +        */
> +       kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE);
> +       init_task.kasan_depth = 0;
> +       os_info("KernelAddressSanitizer initialized\n");
> +}
> +
> +static void (*kasan_init_ptr)(void)
> +__section(.kasan_init) __used
> += kasan_init;
> +#endif
>
>  /* allocated in paging_init, zeroed in mem_init, and unchanged thereafter */
>  unsigned long *empty_zero_page = NULL;
> diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c
> index 3c1b77474d2d..8530b2e08604 100644
> --- a/arch/um/os-Linux/mem.c
> +++ b/arch/um/os-Linux/mem.c
> @@ -17,6 +17,28 @@
>  #include <init.h>
>  #include <os.h>
>
> +/*
> + * kasan_map_memory - maps memory from @start with a size of @len.
> + * The allocated memory is filled with zeroes upon success.
> + * @start: the start address of the memory to be mapped
> + * @len: the length of the memory to be mapped
> + *
> + * This function is used to map shadow memory for KASAN in uml
> + */
> +void kasan_map_memory(void *start, size_t len)
> +{
> +       if (mmap(start,
> +                len,
> +                PROT_READ|PROT_WRITE,
> +                MAP_FIXED|MAP_ANONYMOUS|MAP_PRIVATE|MAP_NORESERVE,
> +                -1,
> +                0) == MAP_FAILED) {
> +               os_info("Couldn't allocate shadow memory: %s\n.",
> +                       strerror(errno));
> +               exit(1);
> +       }
> +}
> +
>  /* Set by make_tempfile() during early boot. */
>  static char *tempdir = NULL;
>
> diff --git a/arch/um/os-Linux/user_syms.c b/arch/um/os-Linux/user_syms.c
> index 715594fe5719..cb667c9225ab 100644
> --- a/arch/um/os-Linux/user_syms.c
> +++ b/arch/um/os-Linux/user_syms.c
> @@ -27,10 +27,10 @@ EXPORT_SYMBOL(strstr);
>  #ifndef __x86_64__
>  extern void *memcpy(void *, const void *, size_t);
>  EXPORT_SYMBOL(memcpy);
> -#endif
> -
>  EXPORT_SYMBOL(memmove);
>  EXPORT_SYMBOL(memset);
> +#endif
> +
>  EXPORT_SYMBOL(printf);
>
>  /* Here, instead, I can provide a fake prototype. Yes, someone cares: genksyms.
> diff --git a/arch/x86/um/Makefile b/arch/x86/um/Makefile
> index 33c51c064c77..7dbd76c546fe 100644
> --- a/arch/x86/um/Makefile
> +++ b/arch/x86/um/Makefile
> @@ -26,7 +26,8 @@ else
>
>  obj-y += syscalls_64.o vdso/
>
> -subarch-y = ../lib/csum-partial_64.o ../lib/memcpy_64.o ../entry/thunk_64.o
> +subarch-y = ../lib/csum-partial_64.o ../lib/memcpy_64.o ../entry/thunk_64.o \
> +       ../lib/memmove_64.o ../lib/memset_64.o
>
>  endif
>
> diff --git a/arch/x86/um/vdso/Makefile b/arch/x86/um/vdso/Makefile
> index 0caddd6acb22..450efa0fb694 100644
> --- a/arch/x86/um/vdso/Makefile
> +++ b/arch/x86/um/vdso/Makefile
> @@ -3,6 +3,9 @@
>  # Building vDSO images for x86.
>  #
>
> +# do not instrument on vdso because KASAN is not compatible with user mode
> +KASAN_SANITIZE                 := n
> +
>  # Prevents link failures: __sanitizer_cov_trace_pc() is not linked in.
>  KCOV_INSTRUMENT                := n
>
> diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> index 81f5464ea9e1..5b54f3c9a741 100644
> --- a/lib/Kconfig.kasan
> +++ b/lib/Kconfig.kasan
> @@ -125,7 +125,7 @@ config KASAN_STACK_ENABLE
>
>  config KASAN_STACK
>         int
> -       default 1 if KASAN_STACK_ENABLE || CC_IS_GCC
> +       default 1 if (KASAN_STACK_ENABLE || CC_IS_GCC) && !UML
>         default 0
>
>  config KASAN_S390_4_LEVEL_PAGING
> --
> 2.25.0.265.gbab2e86ba0-goog
>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-02-26  0:46 [PATCH] UML: add support for KASAN under x86_64 Patricia Alfonso
  2020-02-26  1:19 ` Brendan Higgins
  2020-02-26 15:24 ` Dmitry Vyukov
@ 2020-03-06  0:03 ` Patricia Alfonso
  2020-03-11 10:32   ` Johannes Berg
  2 siblings, 1 reply; 38+ messages in thread
From: Patricia Alfonso @ 2020-03-06  0:03 UTC (permalink / raw)
  To: Jeff Dike, Richard Weinberger, anton.ivanov, Andrey Ryabinin,
	Dmitry Vyukov, Brendan Higgins, David Gow, Johannes Berg
  Cc: kasan-dev, LKML, linux-um

On Tue, Feb 25, 2020 at 4:46 PM Patricia Alfonso
<trishalfonso@google.com> wrote:
>
> Make KASAN run on User Mode Linux on x86_64.
>
> Depends on Constructor support in UML - "[RFC PATCH] um:
> implement CONFIG_CONSTRUCTORS for modules"
> (https://patchwork.ozlabs.org/patch/1234551/) by Johannes Berg.
>
> The location of the KASAN shadow memory, starting at
> KASAN_SHADOW_OFFSET, can be configured using the
> KASAN_SHADOW_OFFSET option. UML uses roughly 18TB of address
> space, and KASAN requires 1/8th of this. The default location of
> this offset is 0x7fff8000 as suggested by Dmitry Vyukov. There is
> usually enough free space at this location; however, it is a config
> option so that it can be easily changed if needed.
>
> The UML-specific KASAN initializer uses mmap to map
> the roughly 2.25TB of shadow memory to the location defined by
> KASAN_SHADOW_OFFSET. kasan_init() utilizes constructors to initialize
> KASAN before main().
>
> Disable stack instrumentation on UML via KASAN_STACK config option to
> avoid false positive KASAN reports.
>
> Signed-off-by: Patricia Alfonso <trishalfonso@google.com>
> ---

Hi all, I just want to bump this so we can get all the comments while
this is still fresh in everyone's minds. I would love if some UML
maintainers could give their thoughts!

Thanks,
Patricia

>  arch/um/Kconfig                  | 13 +++++++++++++
>  arch/um/Makefile                 |  6 ++++++
>  arch/um/include/asm/common.lds.S |  1 +
>  arch/um/include/asm/kasan.h      | 32 ++++++++++++++++++++++++++++++++
>  arch/um/kernel/dyn.lds.S         |  5 ++++-
>  arch/um/kernel/mem.c             | 18 ++++++++++++++++++
>  arch/um/os-Linux/mem.c           | 22 ++++++++++++++++++++++
>  arch/um/os-Linux/user_syms.c     |  4 ++--
>  arch/x86/um/Makefile             |  3 ++-
>  arch/x86/um/vdso/Makefile        |  3 +++
>  lib/Kconfig.kasan                |  2 +-
>  11 files changed, 104 insertions(+), 5 deletions(-)
>  create mode 100644 arch/um/include/asm/kasan.h
>
> diff --git a/arch/um/Kconfig b/arch/um/Kconfig
> index 0917f8443c28..fb2ad1fb05fd 100644
> --- a/arch/um/Kconfig
> +++ b/arch/um/Kconfig
> @@ -8,6 +8,7 @@ config UML
>         select ARCH_HAS_KCOV
>         select ARCH_NO_PREEMPT
>         select HAVE_ARCH_AUDITSYSCALL
> +       select HAVE_ARCH_KASAN if X86_64
>         select HAVE_ARCH_SECCOMP_FILTER
>         select HAVE_ASM_MODVERSIONS
>         select HAVE_UID16
> @@ -200,6 +201,18 @@ config UML_TIME_TRAVEL_SUPPORT
>
>           It is safe to say Y, but you probably don't need this.
>
> +config KASAN_SHADOW_OFFSET
> +       hex
> +       depends on KASAN
> +       default 0x7fff8000
> +       help
> +         This is the offset at which the ~2.25TB of shadow memory is
> +         mapped and used by KASAN for memory debugging. This can be any
> +         address that has at least KASAN_SHADOW_SIZE(total address space divided
> +         by 8) amount of space so that the KASAN shadow memory does not conflict
> +         with anything. The default is 0x7fff8000, as it fits into immediate of
> +         most instructions.
> +
>  endmenu
>
>  source "arch/um/drivers/Kconfig"
> diff --git a/arch/um/Makefile b/arch/um/Makefile
> index d2daa206872d..28fe7a9a1858 100644
> --- a/arch/um/Makefile
> +++ b/arch/um/Makefile
> @@ -75,6 +75,12 @@ USER_CFLAGS = $(patsubst $(KERNEL_DEFINES),,$(patsubst -I%,,$(KBUILD_CFLAGS))) \
>                 -D_FILE_OFFSET_BITS=64 -idirafter $(srctree)/include \
>                 -idirafter $(objtree)/include -D__KERNEL__ -D__UM_HOST__
>
> +# Kernel config options are not included in USER_CFLAGS, but the option for KASAN
> +# should be included if the KASAN config option was set.
> +ifdef CONFIG_KASAN
> +       USER_CFLAGS+=-DCONFIG_KASAN=y
> +endif
> +
>  #This will adjust *FLAGS accordingly to the platform.
>  include $(ARCH_DIR)/Makefile-os-$(OS)
>
> diff --git a/arch/um/include/asm/common.lds.S b/arch/um/include/asm/common.lds.S
> index eca6c452a41b..731f8c8422a2 100644
> --- a/arch/um/include/asm/common.lds.S
> +++ b/arch/um/include/asm/common.lds.S
> @@ -83,6 +83,7 @@
>    }
>    .init_array : {
>         __init_array_start = .;
> +       *(.kasan_init)
>         *(.init_array)
>         __init_array_end = .;
>    }
> diff --git a/arch/um/include/asm/kasan.h b/arch/um/include/asm/kasan.h
> new file mode 100644
> index 000000000000..2b81e7bcd4af
> --- /dev/null
> +++ b/arch/um/include/asm/kasan.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_UM_KASAN_H
> +#define __ASM_UM_KASAN_H
> +
> +#include <linux/init.h>
> +#include <linux/const.h>
> +
> +#define KASAN_SHADOW_OFFSET _AC(CONFIG_KASAN_SHADOW_OFFSET, UL)
> +
> +/* used in kasan_mem_to_shadow to divide by 8 */
> +#define KASAN_SHADOW_SCALE_SHIFT 3
> +
> +#ifdef CONFIG_X86_64
> +#define KASAN_HOST_USER_SPACE_END_ADDR 0x00007fffffffffffUL
> +/* KASAN_SHADOW_SIZE is the size of total address space divided by 8 */
> +#define KASAN_SHADOW_SIZE ((KASAN_HOST_USER_SPACE_END_ADDR + 1) >> \
> +                       KASAN_SHADOW_SCALE_SHIFT)
> +#else
> +#error "KASAN_SHADOW_SIZE is not defined for this sub-architecture"
> +#endif /* CONFIG_X86_64 */
> +
> +#define KASAN_SHADOW_START (KASAN_SHADOW_OFFSET)
> +#define KASAN_SHADOW_END (KASAN_SHADOW_START + KASAN_SHADOW_SIZE)
> +
> +#ifdef CONFIG_KASAN
> +void kasan_init(void);
> +void kasan_map_memory(void *start, unsigned long len);
> +#else
> +static inline void kasan_init(void) { }
> +#endif /* CONFIG_KASAN */
> +
> +#endif /* __ASM_UM_KASAN_H */
> diff --git a/arch/um/kernel/dyn.lds.S b/arch/um/kernel/dyn.lds.S
> index f5001481010c..d91bdb2c3143 100644
> --- a/arch/um/kernel/dyn.lds.S
> +++ b/arch/um/kernel/dyn.lds.S
> @@ -103,7 +103,10 @@ SECTIONS
>       be empty, which isn't pretty.  */
>    . = ALIGN(32 / 8);
>    .preinit_array     : { *(.preinit_array) }
> -  .init_array     : { *(.init_array) }
> +  .init_array     : {
> +    *(.kasan_init)
> +    *(.init_array)
> +  }
>    .fini_array     : { *(.fini_array) }
>    .data           : {
>      INIT_TASK_DATA(KERNEL_STACK_SIZE)
> diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c
> index 30885d0b94ac..7b0d028aa079 100644
> --- a/arch/um/kernel/mem.c
> +++ b/arch/um/kernel/mem.c
> @@ -18,6 +18,24 @@
>  #include <kern_util.h>
>  #include <mem_user.h>
>  #include <os.h>
> +#include <linux/sched/task.h>
> +
> +#ifdef CONFIG_KASAN
> +void kasan_init(void)
> +{
> +       /*
> +        * kasan_map_memory will map all of the required address space and
> +        * the host machine will allocate physical memory as necessary.
> +        */
> +       kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE);
> +       init_task.kasan_depth = 0;
> +       os_info("KernelAddressSanitizer initialized\n");
> +}
> +
> +static void (*kasan_init_ptr)(void)
> +__section(.kasan_init) __used
> += kasan_init;
> +#endif
>
>  /* allocated in paging_init, zeroed in mem_init, and unchanged thereafter */
>  unsigned long *empty_zero_page = NULL;
> diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c
> index 3c1b77474d2d..8530b2e08604 100644
> --- a/arch/um/os-Linux/mem.c
> +++ b/arch/um/os-Linux/mem.c
> @@ -17,6 +17,28 @@
>  #include <init.h>
>  #include <os.h>
>
> +/*
> + * kasan_map_memory - maps memory from @start with a size of @len.
> + * The allocated memory is filled with zeroes upon success.
> + * @start: the start address of the memory to be mapped
> + * @len: the length of the memory to be mapped
> + *
> + * This function is used to map shadow memory for KASAN in uml
> + */
> +void kasan_map_memory(void *start, size_t len)
> +{
> +       if (mmap(start,
> +                len,
> +                PROT_READ|PROT_WRITE,
> +                MAP_FIXED|MAP_ANONYMOUS|MAP_PRIVATE|MAP_NORESERVE,
> +                -1,
> +                0) == MAP_FAILED) {
> +               os_info("Couldn't allocate shadow memory: %s\n.",
> +                       strerror(errno));
> +               exit(1);
> +       }
> +}
> +
>  /* Set by make_tempfile() during early boot. */
>  static char *tempdir = NULL;
>
> diff --git a/arch/um/os-Linux/user_syms.c b/arch/um/os-Linux/user_syms.c
> index 715594fe5719..cb667c9225ab 100644
> --- a/arch/um/os-Linux/user_syms.c
> +++ b/arch/um/os-Linux/user_syms.c
> @@ -27,10 +27,10 @@ EXPORT_SYMBOL(strstr);
>  #ifndef __x86_64__
>  extern void *memcpy(void *, const void *, size_t);
>  EXPORT_SYMBOL(memcpy);
> -#endif
> -
>  EXPORT_SYMBOL(memmove);
>  EXPORT_SYMBOL(memset);
> +#endif
> +
>  EXPORT_SYMBOL(printf);
>
>  /* Here, instead, I can provide a fake prototype. Yes, someone cares: genksyms.
> diff --git a/arch/x86/um/Makefile b/arch/x86/um/Makefile
> index 33c51c064c77..7dbd76c546fe 100644
> --- a/arch/x86/um/Makefile
> +++ b/arch/x86/um/Makefile
> @@ -26,7 +26,8 @@ else
>
>  obj-y += syscalls_64.o vdso/
>
> -subarch-y = ../lib/csum-partial_64.o ../lib/memcpy_64.o ../entry/thunk_64.o
> +subarch-y = ../lib/csum-partial_64.o ../lib/memcpy_64.o ../entry/thunk_64.o \
> +       ../lib/memmove_64.o ../lib/memset_64.o
>
>  endif
>
> diff --git a/arch/x86/um/vdso/Makefile b/arch/x86/um/vdso/Makefile
> index 0caddd6acb22..450efa0fb694 100644
> --- a/arch/x86/um/vdso/Makefile
> +++ b/arch/x86/um/vdso/Makefile
> @@ -3,6 +3,9 @@
>  # Building vDSO images for x86.
>  #
>
> +# do not instrument on vdso because KASAN is not compatible with user mode
> +KASAN_SANITIZE                 := n
> +
>  # Prevents link failures: __sanitizer_cov_trace_pc() is not linked in.
>  KCOV_INSTRUMENT                := n
>
> diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> index 81f5464ea9e1..5b54f3c9a741 100644
> --- a/lib/Kconfig.kasan
> +++ b/lib/Kconfig.kasan
> @@ -125,7 +125,7 @@ config KASAN_STACK_ENABLE
>
>  config KASAN_STACK
>         int
> -       default 1 if KASAN_STACK_ENABLE || CC_IS_GCC
> +       default 1 if (KASAN_STACK_ENABLE || CC_IS_GCC) && !UML
>         default 0
>
>  config KASAN_S390_4_LEVEL_PAGING
> --
> 2.25.0.265.gbab2e86ba0-goog
>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-06  0:03 ` Patricia Alfonso
@ 2020-03-11 10:32   ` Johannes Berg
  2020-03-11 10:46     ` Dmitry Vyukov
                       ` (3 more replies)
  0 siblings, 4 replies; 38+ messages in thread
From: Johannes Berg @ 2020-03-11 10:32 UTC (permalink / raw)
  To: Patricia Alfonso, Jeff Dike, Richard Weinberger, anton.ivanov,
	Andrey Ryabinin, Dmitry Vyukov, Brendan Higgins, David Gow
  Cc: kasan-dev, LKML, linux-um

Hi,

> Hi all, I just want to bump this so we can get all the comments while
> this is still fresh in everyone's minds. I would love if some UML
> maintainers could give their thoughts!

I'm not the maintainer, and I don't know where Richard is, but I just
tried with the test_kasan.ko module, and that seems to work. Did you
test that too? I was surprised to see this because you said you didn't
test modules, but surely this would've been the easiest way?

Anyway, as expected, stack (and of course alloca) OOB access is not
detected right now, but otherwise it seems great.

Here's the log:
https://p.sipsolutions.net/ca9b4157776110fe.txt

I'll repost my module init thing as a proper patch then, I guess.


I do see issues with modules though, e.g. 
https://p.sipsolutions.net/1a2df5f65d885937.txt

where we seem to get some real confusion when lockdep is storing the
stack trace??

And https://p.sipsolutions.net/9a97e8f68d8d24b7.txt, where something
convinces ASAN that an address is a user address (it might even be
right?) and it disallows kernel access to it?


Also, do you have any intention to work on the stack later? For me,
enabling that doesn't even report any issues, it just hangs at 'boot'.

johannes


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-11 10:32   ` Johannes Berg
@ 2020-03-11 10:46     ` Dmitry Vyukov
  2020-03-11 11:18     ` Johannes Berg
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 38+ messages in thread
From: Dmitry Vyukov @ 2020-03-11 10:46 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Patricia Alfonso, Jeff Dike, Richard Weinberger, anton.ivanov,
	Andrey Ryabinin, Brendan Higgins, David Gow, kasan-dev, LKML,
	linux-um

On Wed, Mar 11, 2020 at 11:32 AM Johannes Berg
<johannes@sipsolutions.net> wrote:
>
> Hi,
>
> > Hi all, I just want to bump this so we can get all the comments while
> > this is still fresh in everyone's minds. I would love if some UML
> > maintainers could give their thoughts!
>
> I'm not the maintainer, and I don't know where Richard is, but I just
> tried with the test_kasan.ko module, and that seems to work. Did you
> test that too? I was surprised to see this because you said you didn't
> test modules, but surely this would've been the easiest way?
>
> Anyway, as expected, stack (and of course alloca) OOB access is not
> detected right now, but otherwise it seems great.
>
> Here's the log:
> https://p.sipsolutions.net/ca9b4157776110fe.txt
>
> I'll repost my module init thing as a proper patch then, I guess.
>
>
> I do see issues with modules though, e.g.
> https://p.sipsolutions.net/1a2df5f65d885937.txt
>
> where we seem to get some real confusion when lockdep is storing the
> stack trace??
>
> And https://p.sipsolutions.net/9a97e8f68d8d24b7.txt, where something
> convinces ASAN that an address is a user address (it might even be
> right?) and it disallows kernel access to it?

Please pass these reports via scripts/decode_stacktrace.sh to add line
numbers (or any other symbolization script). What is the base
revision?
Hard to analyze without line numbers.

> Also, do you have any intention to work on the stack later? For me,
> enabling that doesn't even report any issues, it just hangs at 'boot'.
>
> johannes
>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-11 10:32   ` Johannes Berg
  2020-03-11 10:46     ` Dmitry Vyukov
@ 2020-03-11 11:18     ` Johannes Berg
  2020-03-11 11:40       ` Johannes Berg
  2020-03-11 17:34       ` Dmitry Vyukov
  2020-03-11 22:32     ` Patricia Alfonso
  2020-03-29 19:06     ` [PATCH] " Richard Weinberger
  3 siblings, 2 replies; 38+ messages in thread
From: Johannes Berg @ 2020-03-11 11:18 UTC (permalink / raw)
  To: Patricia Alfonso, Jeff Dike, Richard Weinberger, anton.ivanov,
	Andrey Ryabinin, Dmitry Vyukov, Brendan Higgins, David Gow
  Cc: linux-um, LKML, kasan-dev

On Wed, 2020-03-11 at 11:32 +0100, Johannes Berg wrote:
> 
> I do see issues with modules though, e.g. 
> https://p.sipsolutions.net/1a2df5f65d885937.txt
> 
> where we seem to get some real confusion when lockdep is storing the
> stack trace??
> 
> And https://p.sipsolutions.net/9a97e8f68d8d24b7.txt, where something
> convinces ASAN that an address is a user address (it might even be
> right?) and it disallows kernel access to it?

I can work around both of these by not freeing the original module copy
in kernel/module.c:

        /* Get rid of temporary copy. */
//      free_copy(info);

but I really have no idea why we get this in the first place?

Another interesting data point is that it never happens on the first
module.

Also, I've managed to get a report like this:

Memory state around the buggy address:
 000000007106cf00: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
 000000007106cf80: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
>000000007106d000: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
                   ^
 000000007106d080: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
 000000007106d100: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b


which indicates that something's _really_ off with the KASAN shadow?


Ohhh ...

$ gdb -p ...
(gdb) p/x task_size
$1 = 0x7fc0000000
(gdb) p/x __end_of_fixed_addresses
$2 = 0x0
(gdb) p/x end_iomem
$3 = 0x70000000
(gdb) p/x __va_space

#define TASK_SIZE (task_size)
#define FIXADDR_TOP        (TASK_SIZE - 2 * PAGE_SIZE)

#define FIXADDR_START      (FIXADDR_TOP - FIXADDR_SIZE)
#define FIXADDR_SIZE       (__end_of_fixed_addresses << PAGE_SHIFT)

#define VMALLOC_END       (FIXADDR_START-2*PAGE_SIZE)

#define MODULES_VADDR   VMALLOC_START
#define MODULES_END       VMALLOC_END
#define VMALLOC_START ((end_iomem + VMALLOC_OFFSET) & ~(VMALLOC_OFFSET-1))
#define VMALLOC_OFFSET  (__va_space)
#define __va_space (8*1024*1024)


So from that, it would look like the UML vmalloc area is from
0x  70800000 all the way to
0x7fbfffc000, which obviously clashes with the KASAN_SHADOW_OFFSET being
just 0x7fff8000.


I'm guessing that basically the module loading overwrote the kasan
shadow then?

I tried changing it

 config KASAN_SHADOW_OFFSET
        hex
        depends on KASAN
-       default 0x7fff8000
+       default 0x8000000000


and also put a check in like this:

+++ b/arch/um/kernel/um_arch.c
@@ -13,6 +13,7 @@
 #include <linux/sched.h>
 #include <linux/sched/task.h>
 #include <linux/kmsg_dump.h>
+#include <linux/kasan.h>
 
 #include <asm/pgtable.h>
 #include <asm/processor.h>
@@ -267,9 +268,11 @@ int __init linux_main(int argc, char **argv)
        /*
         * TASK_SIZE needs to be PGDIR_SIZE aligned or else exit_mmap craps
         * out
         */
        task_size = host_task_size & PGDIR_MASK;
 
+       if (task_size > KASAN_SHADOW_OFFSET)
+               panic("KASAN shadow offset must be bigger than task size");


but now I just crash accessing the shadow even though it was mapped fine?


Pid: 504, comm: modprobe Tainted: G           O      5.5.0-rc6-00009-g09462ab4014b-dirty
RIP:  
RSP: 000000006d68fa90  EFLAGS: 00010202
RAX: 000000800e0210cd RBX: 000000007010866f RCX: 00000000601a9777
RDX: 000000800e0210ce RSI: 0000000000000004 RDI: 000000007010866c
RBP: 000000006d68faa0 R08: 000000800e0210cd R09: 0000000060041432
R10: 000000800e0210ce R11: 0000000000000001 R12: 000000800e0210cd
R13: 0000000000000000 R14: 0000000000000001 R15: 00000000601c2e82
Kernel panic - not syncing: Kernel mode fault at addr 0x800e0210cd, ip 0x601c332b
CPU: 0 PID: 504 Comm: modprobe Tainted: G           O      5.5.0-rc6-00009-g09462ab4014b-dirty #24
Stack:
601c2f89 70108638 6d68fab0 601c1209
6d68fad0 601a9777 6cf2b240 7317f000
6d68fb40 601a2ae9 6f15b118 00000001
Call Trace:
? __asan_load8 (/home/tester/vlab/linux/mm/kasan/generic.c:252) 
__kasan_check_write (/home/tester/vlab/linux/mm/kasan/common.c:102) 
__free_pages (/home/tester/vlab/linux/./arch/x86/include/asm/atomic.h:125 /home/tester/vlab/linux/./include/asm-generic/atomic-instrumented.h:748 /home/tester/vlab/linux/./include/linux/page_ref.h:139 /home/tester/vlab/linux/./include/linux/mm.h:593 /home/tester/vlab/linux/mm/page_alloc.c:4823) 
__vunmap (/home/tester/vlab/linux/mm/vmalloc.c:2303 (discriminator 2)) 
? __asan_load4 (/home/tester/vlab/linux/mm/kasan/generic.c:251) 
? sysfs_create_bin_file (/home/tester/vlab/linux/fs/sysfs/file.c:537) 
__vfree (/home/tester/vlab/linux/mm/vmalloc.c:2356) 
? delete_object_full (/home/tester/vlab/linux/mm/kmemleak.c:693) 
vfree (/home/tester/vlab/linux/mm/vmalloc.c:2386) 
? sysfs_create_bin_file (/home/tester/vlab/linux/fs/sysfs/file.c:537) 
? __asan_load8 (/home/tester/vlab/linux/mm/kasan/generic.c:252) 
load_module (/home/tester/vlab/linux/./include/linux/jump_label.h:254 /home/tester/vlab/linux/./include/linux/jump_label.h:264 /home/tester/vlab/linux/./include/trace/events/module.h:31 /home/tester/vlab/linux/kernel/module.c:3927) 
? kernel_read_file_from_fd (/home/tester/vlab/linux/fs/exec.c:993) 
? __asan_load8 (/home/tester/vlab/linux/mm/kasan/generic.c:252) 
__do_sys_finit_module (/home/tester/vlab/linux/kernel/module.c:4019) 
? sys_finit_module (/home/tester/vlab/linux/kernel/module.c:3995) 
? __asan_store8 (/home/tester/vlab/linux/mm/kasan/generic.c:252) 
sys_finit_module (/home/tester/vlab/linux/kernel/module.c:3995) 
handle_syscall (/home/tester/vlab/linux/arch/um/kernel/skas/syscall.c:44) 
userspace (/home/tester/vlab/linux/arch/um/os-Linux/skas/process.c:173 /home/tester/vlab/linux/arch/um/os-Linux/skas/process.c:416) 
? save_registers (/home/tester/vlab/linux/arch/um/os-Linux/registers.c:18) 
? arch_prctl (/home/tester/vlab/linux/arch/x86/um/syscalls_64.c:65) 
? calculate_sigpending (/home/tester/vlab/linux/kernel/signal.c:200) 
? __asan_load8 (/home/tester/vlab/linux/mm/kasan/generic.c:252) 
? __asan_load8 (/home/tester/vlab/linux/mm/kasan/generic.c:252) 
? __asan_load8 (/home/tester/vlab/linux/mm/kasan/generic.c:252) 
fork_handler (/home/tester/vlab/linux/arch/um/kernel/process.c:154) 

johannes


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-11 11:18     ` Johannes Berg
@ 2020-03-11 11:40       ` Johannes Berg
  2020-03-11 17:34       ` Dmitry Vyukov
  1 sibling, 0 replies; 38+ messages in thread
From: Johannes Berg @ 2020-03-11 11:40 UTC (permalink / raw)
  To: Patricia Alfonso, Jeff Dike, Richard Weinberger, anton.ivanov,
	Andrey Ryabinin, Dmitry Vyukov, Brendan Higgins, David Gow
  Cc: linux-um, LKML, kasan-dev


> Pid: 504, comm: modprobe Tainted: G           O      5.5.0-rc6-00009-g09462ab4014b-dirty
> RIP:  
> RSP: 000000006d68fa90  EFLAGS: 00010202
> RAX: 000000800e0210cd RBX: 000000007010866f RCX: 00000000601a9777
> RDX: 000000800e0210ce RSI: 0000000000000004 RDI: 000000007010866c
> RBP: 000000006d68faa0 R08: 000000800e0210cd R09: 0000000060041432
> R10: 000000800e0210ce R11: 0000000000000001 R12: 000000800e0210cd
> R13: 0000000000000000 R14: 0000000000000001 R15: 00000000601c2e82
> Kernel panic - not syncing: Kernel mode fault at addr 0x800e0210cd, ip 0x601c332b

Same if I move it to the original place from your v2 patch
(0x100000000000):

Kernel panic - not syncing: Kernel mode fault at addr 0x10000e0c7032, ip 0x601c332b

Not sure what to do now?

johannes


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-11 11:18     ` Johannes Berg
  2020-03-11 11:40       ` Johannes Berg
@ 2020-03-11 17:34       ` Dmitry Vyukov
  2020-03-20 13:39         ` Johannes Berg
  1 sibling, 1 reply; 38+ messages in thread
From: Dmitry Vyukov @ 2020-03-11 17:34 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Patricia Alfonso, Jeff Dike, Richard Weinberger, anton.ivanov,
	Andrey Ryabinin, Brendan Higgins, David Gow, linux-um, LKML,
	kasan-dev

On Wed, Mar 11, 2020 at 12:19 PM Johannes Berg
<johannes@sipsolutions.net> wrote:
>
> On Wed, 2020-03-11 at 11:32 +0100, Johannes Berg wrote:
> >
> > I do see issues with modules though, e.g.
> > https://p.sipsolutions.net/1a2df5f65d885937.txt
> >
> > where we seem to get some real confusion when lockdep is storing the
> > stack trace??
> >
> > And https://p.sipsolutions.net/9a97e8f68d8d24b7.txt, where something
> > convinces ASAN that an address is a user address (it might even be
> > right?) and it disallows kernel access to it?
>
> I can work around both of these by not freeing the original module copy
> in kernel/module.c:
>
>         /* Get rid of temporary copy. */
> //      free_copy(info);
>
> but I really have no idea why we get this in the first place?
>
> Another interesting data point is that it never happens on the first
> module.
>
> Also, I've managed to get a report like this:
>
> Memory state around the buggy address:
>  000000007106cf00: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
>  000000007106cf80: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
> >000000007106d000: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
>                    ^
>  000000007106d080: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
>  000000007106d100: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b
>
>
> which indicates that something's _really_ off with the KASAN shadow?
>
>
> Ohhh ...
>
> $ gdb -p ...
> (gdb) p/x task_size
> $1 = 0x7fc0000000
> (gdb) p/x __end_of_fixed_addresses
> $2 = 0x0
> (gdb) p/x end_iomem
> $3 = 0x70000000
> (gdb) p/x __va_space
>
> #define TASK_SIZE (task_size)
> #define FIXADDR_TOP        (TASK_SIZE - 2 * PAGE_SIZE)
>
> #define FIXADDR_START      (FIXADDR_TOP - FIXADDR_SIZE)
> #define FIXADDR_SIZE       (__end_of_fixed_addresses << PAGE_SHIFT)
>
> #define VMALLOC_END       (FIXADDR_START-2*PAGE_SIZE)
>
> #define MODULES_VADDR   VMALLOC_START
> #define MODULES_END       VMALLOC_END
> #define VMALLOC_START ((end_iomem + VMALLOC_OFFSET) & ~(VMALLOC_OFFSET-1))
> #define VMALLOC_OFFSET  (__va_space)
> #define __va_space (8*1024*1024)
>
>
> So from that, it would look like the UML vmalloc area is from
> 0x  70800000 all the way to
> 0x7fbfffc000, which obviously clashes with the KASAN_SHADOW_OFFSET being
> just 0x7fff8000.
>
>
> I'm guessing that basically the module loading overwrote the kasan
> shadow then?

Well, ok, this is definitely not going to fly :)

I don't know if it's easy to move modules to a different location. It
would be nice because 0x7fbfffc000 is the shadow start that's used in
userspace asan and it allows to faster instrumentation (if offset is
within first 2 gigs, the instruction encoding is much more compact,
for >2gigs it will require several instructions).
But if it's not really easy, I guess we go with a large shadow start
(at least initially). A slower but working KASAN is better than fast
non-working KASAN :)

> I tried changing it
>
>  config KASAN_SHADOW_OFFSET
>         hex
>         depends on KASAN
> -       default 0x7fff8000
> +       default 0x8000000000
>
>
> and also put a check in like this:
>
> +++ b/arch/um/kernel/um_arch.c
> @@ -13,6 +13,7 @@
>  #include <linux/sched.h>
>  #include <linux/sched/task.h>
>  #include <linux/kmsg_dump.h>
> +#include <linux/kasan.h>
>
>  #include <asm/pgtable.h>
>  #include <asm/processor.h>
> @@ -267,9 +268,11 @@ int __init linux_main(int argc, char **argv)
>         /*
>          * TASK_SIZE needs to be PGDIR_SIZE aligned or else exit_mmap craps
>          * out
>          */
>         task_size = host_task_size & PGDIR_MASK;
>
> +       if (task_size > KASAN_SHADOW_OFFSET)
> +               panic("KASAN shadow offset must be bigger than task size");
>
>
> but now I just crash accessing the shadow even though it was mapped fine?

Yes, this is puzzling.
I noticed that RIP is the same in both cases and it relates to vmap code.
A support for shadow for vmalloced-memory was added to KASAN recently
and I suspect it may conflict with UML.
See:
https://elixir.bootlin.com/linux/v5.6-rc5/ident/kasan_populate_vmalloc

I think we simply don't need any of that because we already mapped
shadow for all potentially used memory.
A simple thing to try is to disable CONFIG_KASAN_VMALLOC. If it fixes
the problem, we need to either force-disable CONFIG_KASAN_VMALLOC
under UML or return early from these functions.

What does pte-manipulation code even do under UML?

Looking at the code around, kasan_mem_notifier may be a problem too,
or at least excessive and confusing. We already have shadow for
everything, we don't need _any_ of dynamic/lazy shadow mapping.


> Pid: 504, comm: modprobe Tainted: G           O      5.5.0-rc6-00009-g09462ab4014b-dirty
> RIP:
> RSP: 000000006d68fa90  EFLAGS: 00010202
> RAX: 000000800e0210cd RBX: 000000007010866f RCX: 00000000601a9777
> RDX: 000000800e0210ce RSI: 0000000000000004 RDI: 000000007010866c
> RBP: 000000006d68faa0 R08: 000000800e0210cd R09: 0000000060041432
> R10: 000000800e0210ce R11: 0000000000000001 R12: 000000800e0210cd
> R13: 0000000000000000 R14: 0000000000000001 R15: 00000000601c2e82
> Kernel panic - not syncing: Kernel mode fault at addr 0x800e0210cd, ip 0x601c332b
> CPU: 0 PID: 504 Comm: modprobe Tainted: G           O      5.5.0-rc6-00009-g09462ab4014b-dirty #24
> Stack:
> 601c2f89 70108638 6d68fab0 601c1209
> 6d68fad0 601a9777 6cf2b240 7317f000
> 6d68fb40 601a2ae9 6f15b118 00000001
> Call Trace:
> ? __asan_load8 (/home/tester/vlab/linux/mm/kasan/generic.c:252)
> __kasan_check_write (/home/tester/vlab/linux/mm/kasan/common.c:102)
> __free_pages (/home/tester/vlab/linux/./arch/x86/include/asm/atomic.h:125 /home/tester/vlab/linux/./include/asm-generic/atomic-instrumented.h:748 /home/tester/vlab/linux/./include/linux/page_ref.h:139 /home/tester/vlab/linux/./include/linux/mm.h:593 /home/tester/vlab/linux/mm/page_alloc.c:4823)
> __vunmap (/home/tester/vlab/linux/mm/vmalloc.c:2303 (discriminator 2))
> ? __asan_load4 (/home/tester/vlab/linux/mm/kasan/generic.c:251)
> ? sysfs_create_bin_file (/home/tester/vlab/linux/fs/sysfs/file.c:537)
> __vfree (/home/tester/vlab/linux/mm/vmalloc.c:2356)
> ? delete_object_full (/home/tester/vlab/linux/mm/kmemleak.c:693)
> vfree (/home/tester/vlab/linux/mm/vmalloc.c:2386)
> ? sysfs_create_bin_file (/home/tester/vlab/linux/fs/sysfs/file.c:537)
> ? __asan_load8 (/home/tester/vlab/linux/mm/kasan/generic.c:252)
> load_module (/home/tester/vlab/linux/./include/linux/jump_label.h:254 /home/tester/vlab/linux/./include/linux/jump_label.h:264 /home/tester/vlab/linux/./include/trace/events/module.h:31 /home/tester/vlab/linux/kernel/module.c:3927)
> ? kernel_read_file_from_fd (/home/tester/vlab/linux/fs/exec.c:993)
> ? __asan_load8 (/home/tester/vlab/linux/mm/kasan/generic.c:252)
> __do_sys_finit_module (/home/tester/vlab/linux/kernel/module.c:4019)
> ? sys_finit_module (/home/tester/vlab/linux/kernel/module.c:3995)
> ? __asan_store8 (/home/tester/vlab/linux/mm/kasan/generic.c:252)
> sys_finit_module (/home/tester/vlab/linux/kernel/module.c:3995)
> handle_syscall (/home/tester/vlab/linux/arch/um/kernel/skas/syscall.c:44)
> userspace (/home/tester/vlab/linux/arch/um/os-Linux/skas/process.c:173 /home/tester/vlab/linux/arch/um/os-Linux/skas/process.c:416)
> ? save_registers (/home/tester/vlab/linux/arch/um/os-Linux/registers.c:18)
> ? arch_prctl (/home/tester/vlab/linux/arch/x86/um/syscalls_64.c:65)
> ? calculate_sigpending (/home/tester/vlab/linux/kernel/signal.c:200)
> ? __asan_load8 (/home/tester/vlab/linux/mm/kasan/generic.c:252)
> ? __asan_load8 (/home/tester/vlab/linux/mm/kasan/generic.c:252)
> ? __asan_load8 (/home/tester/vlab/linux/mm/kasan/generic.c:252)
> fork_handler (/home/tester/vlab/linux/arch/um/kernel/process.c:154)
>
> johannes
>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-11 10:32   ` Johannes Berg
  2020-03-11 10:46     ` Dmitry Vyukov
  2020-03-11 11:18     ` Johannes Berg
@ 2020-03-11 22:32     ` Patricia Alfonso
  2020-03-11 22:44       ` Johannes Berg
  2020-03-29 19:06     ` [PATCH] " Richard Weinberger
  3 siblings, 1 reply; 38+ messages in thread
From: Patricia Alfonso @ 2020-03-11 22:32 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Jeff Dike, Richard Weinberger, anton.ivanov, Andrey Ryabinin,
	Dmitry Vyukov, Brendan Higgins, David Gow, kasan-dev, LKML,
	linux-um

On Wed, Mar 11, 2020 at 3:32 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> Hi,
>
> > Hi all, I just want to bump this so we can get all the comments while
> > this is still fresh in everyone's minds. I would love if some UML
> > maintainers could give their thoughts!
>
> I'm not the maintainer,
That's okay! I appreciate that you took the time to look at it.

> and I don't know where Richard is, but I just
> tried with the test_kasan.ko module, and that seems to work. Did you
> test that too? I was surprised to see this because you said you didn't
> test modules, but surely this would've been the easiest way?
>
I had not tested with test_kasan.ko. I have been using KUnit to test
KASAN from the beginning so to be completely honest, I hadn't even
learned how to run modules until today.

> Anyway, as expected, stack (and of course alloca) OOB access is not
> detected right now, but otherwise it seems great.
>
Great! Thanks for putting time into this.

> Here's the log:
> https://p.sipsolutions.net/ca9b4157776110fe.txt
>
> I'll repost my module init thing as a proper patch then, I guess.
>
That would be really helpful, thank you!

>
> I do see issues with modules though, e.g.
> https://p.sipsolutions.net/1a2df5f65d885937.txt
>
> where we seem to get some real confusion when lockdep is storing the
> stack trace??
>
> And https://p.sipsolutions.net/9a97e8f68d8d24b7.txt, where something
> convinces ASAN that an address is a user address (it might even be
> right?) and it disallows kernel access to it?
>
>
I'll need some time to investigate these all myself. Having just
gotten my first module to run about an hour ago, any more information
about how you got these errors would be helpful so I can try to
reproduce them on my own.

> Also, do you have any intention to work on the stack later? For me,
> enabling that doesn't even report any issues, it just hangs at 'boot'.
>
I was originally planning on it, but it's not a high priority for me
or my team at this time.

> johannes
>

-- 
Best,
Patricia Alfonso

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-11 22:32     ` Patricia Alfonso
@ 2020-03-11 22:44       ` Johannes Berg
  2022-05-24 10:34         ` Vincent Whitchurch
  0 siblings, 1 reply; 38+ messages in thread
From: Johannes Berg @ 2020-03-11 22:44 UTC (permalink / raw)
  To: Patricia Alfonso
  Cc: Jeff Dike, Richard Weinberger, anton.ivanov, Andrey Ryabinin,
	Dmitry Vyukov, Brendan Higgins, David Gow, kasan-dev, LKML,
	linux-um

On Wed, 2020-03-11 at 15:32 -0700, Patricia Alfonso wrote:

> I'll need some time to investigate these all myself. Having just
> gotten my first module to run about an hour ago, any more information
> about how you got these errors would be helpful so I can try to
> reproduce them on my own.

See the other emails, I was basically just loading random modules. In my
case cfg80211, mac80211, mac80211-hwsim - those are definitely available
without any (virtio) hardware requirements, so you could use them.

Note that doing a bunch of vmalloc would likely result in similar
issues, since the module and vmalloc space is the same on UML.

johannes


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-11 17:34       ` Dmitry Vyukov
@ 2020-03-20 13:39         ` Johannes Berg
  2020-03-20 15:18           ` Dmitry Vyukov
  0 siblings, 1 reply; 38+ messages in thread
From: Johannes Berg @ 2020-03-20 13:39 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Patricia Alfonso, Jeff Dike, Richard Weinberger, anton.ivanov,
	Andrey Ryabinin, Brendan Higgins, David Gow, linux-um, LKML,
	kasan-dev

On Wed, 2020-03-11 at 18:34 +0100, Dmitry Vyukov wrote:

> > $ gdb -p ...
> > (gdb) p/x task_size
> > $1 = 0x7fc0000000
> > (gdb) p/x __end_of_fixed_addresses
> > $2 = 0x0
> > (gdb) p/x end_iomem
> > $3 = 0x70000000
> > (gdb) p/x __va_space
> > 
> > #define TASK_SIZE (task_size)
> > #define FIXADDR_TOP        (TASK_SIZE - 2 * PAGE_SIZE)
> > 
> > #define FIXADDR_START      (FIXADDR_TOP - FIXADDR_SIZE)
> > #define FIXADDR_SIZE       (__end_of_fixed_addresses << PAGE_SHIFT)
> > 
> > #define VMALLOC_END       (FIXADDR_START-2*PAGE_SIZE)
> > 
> > #define MODULES_VADDR   VMALLOC_START
> > #define MODULES_END       VMALLOC_END
> > #define VMALLOC_START ((end_iomem + VMALLOC_OFFSET) & ~(VMALLOC_OFFSET-1))
> > #define VMALLOC_OFFSET  (__va_space)
> > #define __va_space (8*1024*1024)
> > 
> > 
> > So from that, it would look like the UML vmalloc area is from
> > 0x  70800000 all the way to
> > 0x7fbfffc000, which obviously clashes with the KASAN_SHADOW_OFFSET being
> > just 0x7fff8000.
> > 
> > 
> > I'm guessing that basically the module loading overwrote the kasan
> > shadow then?
> 
> Well, ok, this is definitely not going to fly :)

Yeah, not with vmalloc/modules at least, but you can't really prevent
vmalloc :)

> I don't know if it's easy to move modules to a different location.

We'd have to not just move modules, but also vmalloc space. They're one
and the same in UML.

> It
> would be nice because 0x7fbfffc000 is the shadow start that's used in
> userspace asan and it allows to faster instrumentation (if offset is
> within first 2 gigs, the instruction encoding is much more compact,
> for >2gigs it will require several instructions).

Wait ... Now you say 0x7fbfffc000, but that is almost fine? I think you
confused the values - because I see, on userspace, the following:

|| `[0x10007fff8000, 0x7fffffffffff]` || HighMem    ||
|| `[0x02008fff7000, 0x10007fff7fff]` || HighShadow ||
|| `[0x00008fff7000, 0x02008fff6fff]` || ShadowGap  ||
|| `[0x00007fff8000, 0x00008fff6fff]` || LowShadow  ||
|| `[0x000000000000, 0x00007fff7fff]` || LowMem     ||


Now, I also don't really understand what UML is doing here -
os_get_top_address() determines some sort of "top address"? But all that
is only on 32-bit, on 64-bit, that's always 0x7fc0000000.

So basically that means it's just _slightly_ higher than what you
suggested as the KASAN_SHADOW_OFFSET now (even if erroneously?), and
shouldn't actually clash (and we can just change the top address value
to be slightly lower anyway to prevent clashing).

> But if it's not really easy, I guess we go with a large shadow start
> (at least initially). A slower but working KASAN is better than fast
> non-working KASAN :)

Indeed, but I can't even get it to work regardless of the offset.

Note that I have lockdep enabled, and at least some crashes appear to be
because of the stack unwinding code that is called by lockdep in various
situations...

> > I tried changing it
> > 
> >  config KASAN_SHADOW_OFFSET
> >         hex
> >         depends on KASAN
> > -       default 0x7fff8000
> > +       default 0x8000000000
> > 
> > 
> > and also put a check in like this:
> > 
> > +++ b/arch/um/kernel/um_arch.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/sched/task.h>
> >  #include <linux/kmsg_dump.h>
> > +#include <linux/kasan.h>
> > 
> >  #include <asm/pgtable.h>
> >  #include <asm/processor.h>
> > @@ -267,9 +268,11 @@ int __init linux_main(int argc, char **argv)
> >         /*
> >          * TASK_SIZE needs to be PGDIR_SIZE aligned or else exit_mmap craps
> >          * out
> >          */
> >         task_size = host_task_size & PGDIR_MASK;
> > 
> > +       if (task_size > KASAN_SHADOW_OFFSET)
> > +               panic("KASAN shadow offset must be bigger than task size");
> > 
> > 
> > but now I just crash accessing the shadow even though it was mapped fine?
> 
> Yes, this is puzzling.
> I noticed that RIP is the same in both cases and it relates to vmap code.
> A support for shadow for vmalloced-memory was added to KASAN recently
> and I suspect it may conflict with UML.

This can't be it - HAVE_ARCH_KASAN_VMALLOC isn't selected, so
KASAN_VMALLOC isn't set.

> What does pte-manipulation code even do under UML?

No idea.

> Looking at the code around, kasan_mem_notifier may be a problem too,
> or at least excessive and confusing. We already have shadow for
> everything, we don't need _any_ of dynamic/lazy shadow mapping.

CONFIG_MEMORY_HOTPLUG is also not supported in ARCH=um, or at least not
used in my config.

johannes


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-20 13:39         ` Johannes Berg
@ 2020-03-20 15:18           ` Dmitry Vyukov
  2020-03-30  7:43             ` Johannes Berg
  0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Vyukov @ 2020-03-20 15:18 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Patricia Alfonso, Jeff Dike, Richard Weinberger, anton.ivanov,
	Andrey Ryabinin, Brendan Higgins, David Gow, linux-um, LKML,
	kasan-dev

On Fri, Mar 20, 2020 at 2:39 PM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Wed, 2020-03-11 at 18:34 +0100, Dmitry Vyukov wrote:
>
> > > $ gdb -p ...
> > > (gdb) p/x task_size
> > > $1 = 0x7fc0000000
> > > (gdb) p/x __end_of_fixed_addresses
> > > $2 = 0x0
> > > (gdb) p/x end_iomem
> > > $3 = 0x70000000
> > > (gdb) p/x __va_space
> > >
> > > #define TASK_SIZE (task_size)
> > > #define FIXADDR_TOP        (TASK_SIZE - 2 * PAGE_SIZE)
> > >
> > > #define FIXADDR_START      (FIXADDR_TOP - FIXADDR_SIZE)
> > > #define FIXADDR_SIZE       (__end_of_fixed_addresses << PAGE_SHIFT)
> > >
> > > #define VMALLOC_END       (FIXADDR_START-2*PAGE_SIZE)
> > >
> > > #define MODULES_VADDR   VMALLOC_START
> > > #define MODULES_END       VMALLOC_END
> > > #define VMALLOC_START ((end_iomem + VMALLOC_OFFSET) & ~(VMALLOC_OFFSET-1))
> > > #define VMALLOC_OFFSET  (__va_space)
> > > #define __va_space (8*1024*1024)
> > >
> > >
> > > So from that, it would look like the UML vmalloc area is from
> > > 0x  70800000 all the way to
> > > 0x7fbfffc000, which obviously clashes with the KASAN_SHADOW_OFFSET being
> > > just 0x7fff8000.
> > >
> > >
> > > I'm guessing that basically the module loading overwrote the kasan
> > > shadow then?
> >
> > Well, ok, this is definitely not going to fly :)
>
> Yeah, not with vmalloc/modules at least, but you can't really prevent
> vmalloc :)
>
> > I don't know if it's easy to move modules to a different location.
>
> We'd have to not just move modules, but also vmalloc space. They're one
> and the same in UML.
>
> > It
> > would be nice because 0x7fbfffc000 is the shadow start that's used in
> > userspace asan and it allows to faster instrumentation (if offset is
> > within first 2 gigs, the instruction encoding is much more compact,
> > for >2gigs it will require several instructions).
>
> Wait ... Now you say 0x7fbfffc000, but that is almost fine? I think you
> confused the values - because I see, on userspace, the following:

Oh, sorry, I copy-pasted wrong number. I meant 0x7fff8000. Here is the
user-space mapping that uses it:
https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/asan/asan_mapping.h#L25


> || `[0x10007fff8000, 0x7fffffffffff]` || HighMem    ||
> || `[0x02008fff7000, 0x10007fff7fff]` || HighShadow ||
> || `[0x00008fff7000, 0x02008fff6fff]` || ShadowGap  ||
> || `[0x00007fff8000, 0x00008fff6fff]` || LowShadow  ||
> || `[0x000000000000, 0x00007fff7fff]` || LowMem     ||
>
>
> Now, I also don't really understand what UML is doing here -
> os_get_top_address() determines some sort of "top address"? But all that
> is only on 32-bit, on 64-bit, that's always 0x7fc0000000.

Then I would expect 0x1000 0000 0000 to work, but you say it doesn't...

> So basically that means it's just _slightly_ higher than what you
> suggested as the KASAN_SHADOW_OFFSET now (even if erroneously?), and
> shouldn't actually clash (and we can just change the top address value
> to be slightly lower anyway to prevent clashing).
>
> > But if it's not really easy, I guess we go with a large shadow start
> > (at least initially). A slower but working KASAN is better than fast
> > non-working KASAN :)
>
> Indeed, but I can't even get it to work regardless of the offset.
>
> Note that I have lockdep enabled, and at least some crashes appear to be
> because of the stack unwinding code that is called by lockdep in various
> situations...

This is something new, right? The previous stacks you posted did not
mention lockdep.

> > > I tried changing it
> > >
> > >  config KASAN_SHADOW_OFFSET
> > >         hex
> > >         depends on KASAN
> > > -       default 0x7fff8000
> > > +       default 0x8000000000
> > >
> > >
> > > and also put a check in like this:
> > >
> > > +++ b/arch/um/kernel/um_arch.c
> > > @@ -13,6 +13,7 @@
> > >  #include <linux/sched.h>
> > >  #include <linux/sched/task.h>
> > >  #include <linux/kmsg_dump.h>
> > > +#include <linux/kasan.h>
> > >
> > >  #include <asm/pgtable.h>
> > >  #include <asm/processor.h>
> > > @@ -267,9 +268,11 @@ int __init linux_main(int argc, char **argv)
> > >         /*
> > >          * TASK_SIZE needs to be PGDIR_SIZE aligned or else exit_mmap craps
> > >          * out
> > >          */
> > >         task_size = host_task_size & PGDIR_MASK;
> > >
> > > +       if (task_size > KASAN_SHADOW_OFFSET)
> > > +               panic("KASAN shadow offset must be bigger than task size");
> > >
> > >
> > > but now I just crash accessing the shadow even though it was mapped fine?
> >
> > Yes, this is puzzling.
> > I noticed that RIP is the same in both cases and it relates to vmap code.
> > A support for shadow for vmalloced-memory was added to KASAN recently
> > and I suspect it may conflict with UML.
>
> This can't be it - HAVE_ARCH_KASAN_VMALLOC isn't selected, so
> KASAN_VMALLOC isn't set.
>
> > What does pte-manipulation code even do under UML?
>
> No idea.
>
> > Looking at the code around, kasan_mem_notifier may be a problem too,
> > or at least excessive and confusing. We already have shadow for
> > everything, we don't need _any_ of dynamic/lazy shadow mapping.
>
> CONFIG_MEMORY_HOTPLUG is also not supported in ARCH=um, or at least not
> used in my config.

Ack.

Maybe if you dump /proc/self/maps for the process, it will shed some light.
Or is it possible to run it under strace? If we get all
mmap/munmap/mprotect, we will maybe see the offender that messes the
shadow...

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-11 10:32   ` Johannes Berg
                       ` (2 preceding siblings ...)
  2020-03-11 22:32     ` Patricia Alfonso
@ 2020-03-29 19:06     ` Richard Weinberger
  3 siblings, 0 replies; 38+ messages in thread
From: Richard Weinberger @ 2020-03-29 19:06 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Patricia Alfonso, Jeff Dike, anton ivanov, Andrey Ryabinin,
	Dmitry Vyukov, Brendan Higgins, davidgow, kasan-dev,
	linux-kernel, linux-um

----- Ursprüngliche Mail -----
> Von: "Johannes Berg" <johannes@sipsolutions.net>
> An: "Patricia Alfonso" <trishalfonso@google.com>, "Jeff Dike" <jdike@addtoit.com>, "richard" <richard@nod.at>, "anton
> ivanov" <anton.ivanov@cambridgegreys.com>, "Andrey Ryabinin" <aryabinin@virtuozzo.com>, "Dmitry Vyukov"
> <dvyukov@google.com>, "Brendan Higgins" <brendanhiggins@google.com>, "davidgow" <davidgow@google.com>
> CC: "kasan-dev" <kasan-dev@googlegroups.com>, "linux-kernel" <linux-kernel@vger.kernel.org>, "linux-um"
> <linux-um@lists.infradead.org>
> Gesendet: Mittwoch, 11. März 2020 11:32:00
> Betreff: Re: [PATCH] UML: add support for KASAN under x86_64

> Hi,
> 
>> Hi all, I just want to bump this so we can get all the comments while
>> this is still fresh in everyone's minds. I would love if some UML
>> maintainers could give their thoughts!
> 
> I'm not the maintainer, and I don't know where Richard is, but I just
> tried with the test_kasan.ko module, and that seems to work. Did you
> test that too? I was surprised to see this because you said you didn't
> test modules, but surely this would've been the easiest way?

Sorry for vanishing.

I read thought the discussion and it seems like the patch is not ready,
right?

Johannes, thanks a lot for pointing out all these issues.

Thanks,
//richard

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-20 15:18           ` Dmitry Vyukov
@ 2020-03-30  7:43             ` Johannes Berg
  2020-03-30  8:38               ` Dmitry Vyukov
  0 siblings, 1 reply; 38+ messages in thread
From: Johannes Berg @ 2020-03-30  7:43 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Patricia Alfonso, Jeff Dike, Richard Weinberger, anton.ivanov,
	Andrey Ryabinin, Brendan Higgins, David Gow, linux-um, LKML,
	kasan-dev

On Fri, 2020-03-20 at 16:18 +0100, Dmitry Vyukov wrote:
> 
> > Wait ... Now you say 0x7fbfffc000, but that is almost fine? I think you
> > confused the values - because I see, on userspace, the following:
> 
> Oh, sorry, I copy-pasted wrong number. I meant 0x7fff8000. 

Right, ok.

> Then I would expect 0x1000 0000 0000 to work, but you say it doesn't...

So it just occurred to me - as I was mentioning this whole thing to
Richard - that there's probably somewhere some check about whether some
space is userspace or not.

I'm beginning to think that we shouldn't just map this outside of the
kernel memory system, but properly treat it as part of the memory that's
inside. And also use KASAN_VMALLOC.

We can probably still have it at 0x7fff8000, just need to make sure we
actually map it? I tried with vm_area_add_early() but it didn't really
work once you have vmalloc() stuff...

I dunno.

johannes



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-30  7:43             ` Johannes Berg
@ 2020-03-30  8:38               ` Dmitry Vyukov
  2020-03-30  8:41                 ` Johannes Berg
  0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Vyukov @ 2020-03-30  8:38 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Patricia Alfonso, Jeff Dike, Richard Weinberger, anton.ivanov,
	Andrey Ryabinin, Brendan Higgins, David Gow, linux-um, LKML,
	kasan-dev

On Mon, Mar 30, 2020 at 9:44 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Fri, 2020-03-20 at 16:18 +0100, Dmitry Vyukov wrote:
> >
> > > Wait ... Now you say 0x7fbfffc000, but that is almost fine? I think you
> > > confused the values - because I see, on userspace, the following:
> >
> > Oh, sorry, I copy-pasted wrong number. I meant 0x7fff8000.
>
> Right, ok.
>
> > Then I would expect 0x1000 0000 0000 to work, but you say it doesn't...
>
> So it just occurred to me - as I was mentioning this whole thing to
> Richard - that there's probably somewhere some check about whether some
> space is userspace or not.
>
> I'm beginning to think that we shouldn't just map this outside of the
> kernel memory system, but properly treat it as part of the memory that's
> inside. And also use KASAN_VMALLOC.
>
> We can probably still have it at 0x7fff8000, just need to make sure we
> actually map it? I tried with vm_area_add_early() but it didn't really
> work once you have vmalloc() stuff...

But we do mmap it, no? See kasan_init() -> kasan_map_memory() -> mmap.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-30  8:38               ` Dmitry Vyukov
@ 2020-03-30  8:41                 ` Johannes Berg
  2020-03-31  6:14                   ` David Gow
  2020-03-31 16:39                   ` Patricia Alfonso
  0 siblings, 2 replies; 38+ messages in thread
From: Johannes Berg @ 2020-03-30  8:41 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Patricia Alfonso, Jeff Dike, Richard Weinberger, anton.ivanov,
	Andrey Ryabinin, Brendan Higgins, David Gow, linux-um, LKML,
	kasan-dev

On Mon, 2020-03-30 at 10:38 +0200, Dmitry Vyukov wrote:
> On Mon, Mar 30, 2020 at 9:44 AM Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Fri, 2020-03-20 at 16:18 +0100, Dmitry Vyukov wrote:
> > > > Wait ... Now you say 0x7fbfffc000, but that is almost fine? I think you
> > > > confused the values - because I see, on userspace, the following:
> > > 
> > > Oh, sorry, I copy-pasted wrong number. I meant 0x7fff8000.
> > 
> > Right, ok.
> > 
> > > Then I would expect 0x1000 0000 0000 to work, but you say it doesn't...
> > 
> > So it just occurred to me - as I was mentioning this whole thing to
> > Richard - that there's probably somewhere some check about whether some
> > space is userspace or not.
> > 
> > I'm beginning to think that we shouldn't just map this outside of the
> > kernel memory system, but properly treat it as part of the memory that's
> > inside. And also use KASAN_VMALLOC.
> > 
> > We can probably still have it at 0x7fff8000, just need to make sure we
> > actually map it? I tried with vm_area_add_early() but it didn't really
> > work once you have vmalloc() stuff...
> 
> But we do mmap it, no? See kasan_init() -> kasan_map_memory() -> mmap.

Of course. But I meant inside the UML PTE system. We end up *unmapping*
it when loading modules, because it overlaps vmalloc space, and then we
vfree() something again, and unmap it ... because of the overlap.

And if it's *not* in the vmalloc area, then the kernel doesn't consider
it valid, and we seem to often just fault when trying to determine
whether it's valid kernel memory or not ... Though I'm not really sure I
understand the failure part of this case well yet.

johannes


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-30  8:41                 ` Johannes Berg
@ 2020-03-31  6:14                   ` David Gow
  2020-03-31  7:43                     ` Johannes Berg
  2020-03-31 16:39                   ` Patricia Alfonso
  1 sibling, 1 reply; 38+ messages in thread
From: David Gow @ 2020-03-31  6:14 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Dmitry Vyukov, Patricia Alfonso, Jeff Dike, Richard Weinberger,
	Anton Ivanov, Andrey Ryabinin, Brendan Higgins, linux-um, LKML,
	kasan-dev

[-- Attachment #1: Type: text/plain, Size: 2945 bytes --]

On Mon, Mar 30, 2020 at 1:41 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Mon, 2020-03-30 at 10:38 +0200, Dmitry Vyukov wrote:
> > On Mon, Mar 30, 2020 at 9:44 AM Johannes Berg <johannes@sipsolutions.net> wrote:
> > > On Fri, 2020-03-20 at 16:18 +0100, Dmitry Vyukov wrote:
> > > > > Wait ... Now you say 0x7fbfffc000, but that is almost fine? I think you
> > > > > confused the values - because I see, on userspace, the following:
> > > >
> > > > Oh, sorry, I copy-pasted wrong number. I meant 0x7fff8000.
> > >
> > > Right, ok.
> > >
> > > > Then I would expect 0x1000 0000 0000 to work, but you say it doesn't...
> > >
> > > So it just occurred to me - as I was mentioning this whole thing to
> > > Richard - that there's probably somewhere some check about whether some
> > > space is userspace or not.
> > >
> > > I'm beginning to think that we shouldn't just map this outside of the
> > > kernel memory system, but properly treat it as part of the memory that's
> > > inside. And also use KASAN_VMALLOC.
> > >
> > > We can probably still have it at 0x7fff8000, just need to make sure we
> > > actually map it? I tried with vm_area_add_early() but it didn't really
> > > work once you have vmalloc() stuff...
> >
> > But we do mmap it, no? See kasan_init() -> kasan_map_memory() -> mmap.
>
> Of course. But I meant inside the UML PTE system. We end up *unmapping*
> it when loading modules, because it overlaps vmalloc space, and then we
> vfree() something again, and unmap it ... because of the overlap.
>
> And if it's *not* in the vmalloc area, then the kernel doesn't consider
> it valid, and we seem to often just fault when trying to determine
> whether it's valid kernel memory or not ... Though I'm not really sure I
> understand the failure part of this case well yet.
>
> johannes
>

I spent a little time playing around with this, and was able to get
mac80211 loading if I force-enabled CONFIG_KASAN_VMALLOC (alongside
bumping up the shadow memory address).
The test-bpf module was still failing, though — which may or may not
have been related to how bpf uses vmalloc().

Simply adding code to unpoison the region on vmalloc() doesn't seem to
do anything, which lends credence to the idea that the memory is
actually being unmapped or is not considered kernel memory.

I do like the idea of trying to push the shadow memory allocation
through UML's PTE code, but confess to not understanding it
particularly well. I imagine it'd require pushing the KASAN
initialisation back until after init_physmem, and having the shadow
memory be backed by the physmem file? Unless there's a clever way of
allocating the shadow memory early, and then hooking it into the page
tables/etc when those are initialised (akin to how on x86 there's a
separate early shadow memory stage while things are still being set
up, maybe?)

Food for thought, perhaps.

-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3854 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-31  6:14                   ` David Gow
@ 2020-03-31  7:43                     ` Johannes Berg
  0 siblings, 0 replies; 38+ messages in thread
From: Johannes Berg @ 2020-03-31  7:43 UTC (permalink / raw)
  To: David Gow
  Cc: Dmitry Vyukov, Patricia Alfonso, Jeff Dike, Richard Weinberger,
	Anton Ivanov, Andrey Ryabinin, Brendan Higgins, linux-um, LKML,
	kasan-dev

On Mon, 2020-03-30 at 23:14 -0700, David Gow wrote:
> 
> I spent a little time playing around with this, and was able to get
> mac80211 

mac80211, or mac80211-hwsim? I can load a few modules, but then it
crashes on say the third (usually, but who knows what this depends on).

> loading if I force-enabled CONFIG_KASAN_VMALLOC (alongside
> bumping up the shadow memory address).

Not sure I tried that combination though.

> The test-bpf module was still failing, though — which may or may not
> have been related to how bpf uses vmalloc().

I think I got some trouble also with just stack unwinding and other
random things faulting in the vmalloc and/or shadow space ...

> I do like the idea of trying to push the shadow memory allocation
> through UML's PTE code, but confess to not understanding it
> particularly well. 

Me neither. I just noticed that all the vmalloc and kasan-vmalloc do all
the PTE handling, so things might easily clash if you have
CONFIG_KASAN_VMALLOC, which we do want eventually.

> I imagine it'd require pushing the KASAN
> initialisation back until after init_physmem, and having the shadow
> memory be backed by the physmem file? Unless there's a clever way of
> allocating the shadow memory early, and then hooking it into the page
> tables/etc when those are initialised (akin to how on x86 there's a
> separate early shadow memory stage while things are still being set
> up, maybe?)

Pretty sure we should be able to hook it up later, but I haven't really
dug deeply yet.

johannes


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-30  8:41                 ` Johannes Berg
  2020-03-31  6:14                   ` David Gow
@ 2020-03-31 16:39                   ` Patricia Alfonso
  2020-03-31 16:54                     ` Richard Weinberger
  1 sibling, 1 reply; 38+ messages in thread
From: Patricia Alfonso @ 2020-03-31 16:39 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Dmitry Vyukov, Jeff Dike, Richard Weinberger, anton.ivanov,
	Andrey Ryabinin, Brendan Higgins, David Gow, linux-um, LKML,
	kasan-dev

On Mon, Mar 30, 2020 at 1:41 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Mon, 2020-03-30 at 10:38 +0200, Dmitry Vyukov wrote:
> > On Mon, Mar 30, 2020 at 9:44 AM Johannes Berg <johannes@sipsolutions.net> wrote:
> > > On Fri, 2020-03-20 at 16:18 +0100, Dmitry Vyukov wrote:
> > > > > Wait ... Now you say 0x7fbfffc000, but that is almost fine? I think you
> > > > > confused the values - because I see, on userspace, the following:
> > > >
> > > > Oh, sorry, I copy-pasted wrong number. I meant 0x7fff8000.
> > >
> > > Right, ok.
> > >
> > > > Then I would expect 0x1000 0000 0000 to work, but you say it doesn't...
> > >
> > > So it just occurred to me - as I was mentioning this whole thing to
> > > Richard - that there's probably somewhere some check about whether some
> > > space is userspace or not.
> > >

Yeah, it seems the "Kernel panic - not syncing: Segfault with no mm",
"Kernel mode fault at addr...", and "Kernel tried to access user
memory at addr..." errors all come from segv() in
arch/um/kernel/trap.c due to what I think is this type of check
whether the address is
in userspace or not.

> > > I'm beginning to think that we shouldn't just map this outside of the
> > > kernel memory system, but properly treat it as part of the memory that's
> > > inside. And also use KASAN_VMALLOC.
> > >
> > > We can probably still have it at 0x7fff8000, just need to make sure we
> > > actually map it? I tried with vm_area_add_early() but it didn't really
> > > work once you have vmalloc() stuff...
> >

What x86 does when KASAN_VMALLOC is disabled is make all vmalloc
region accesses succeed by default
by using the early shadow memory to have completely unpoisoned and
unpoisonable read-only pages for all of vmalloc (which includes
modules). When KASAN_VMALLOC is enabled in x86, the shadow memory is not
allocated for the vmalloc region at startup. New chunks of shadow
memory are allocated and unpoisoned every time there's a vmalloc()
call. A similar thing might have to be done here by mprotect()ing
the vmalloc space as read only, unpoisoned without KASAN_VMALLOC. This
issue here is that
kasan_init runs so early in the process that the vmalloc region for
uml is not setup yet.


> > But we do mmap it, no? See kasan_init() -> kasan_map_memory() -> mmap.
>
> Of course. But I meant inside the UML PTE system. We end up *unmapping*
> it when loading modules, because it overlaps vmalloc space, and then we
> vfree() something again, and unmap it ... because of the overlap.
>
> And if it's *not* in the vmalloc area, then the kernel doesn't consider
> it valid, and we seem to often just fault when trying to determine
> whether it's valid kernel memory or not ... Though I'm not really sure I
> understand the failure part of this case well yet.
>

I have been testing this issue in a multitude of ways and have only
been getting more confused. It's still very unclear where exactly the
problem occurs, mostly because the errors I found most frequently were
reported in segv(), but the stack traces never contained segv.

Does anyone know if/how UML determines if memory being accessed is
kernel or user memory?

> johannes
>


--
Best,
Patricia

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-31 16:39                   ` Patricia Alfonso
@ 2020-03-31 16:54                     ` Richard Weinberger
  0 siblings, 0 replies; 38+ messages in thread
From: Richard Weinberger @ 2020-03-31 16:54 UTC (permalink / raw)
  To: Patricia Alfonso
  Cc: Johannes Berg, Dmitry Vyukov, Jeff Dike, anton ivanov,
	Andrey Ryabinin, Brendan Higgins, davidgow, linux-um,
	linux-kernel, kasan-dev

Patricia,

----- Ursprüngliche Mail -----
> Von: "Patricia Alfonso" <trishalfonso@google.com>
> An: "Johannes Berg" <johannes@sipsolutions.net>
> CC: "Dmitry Vyukov" <dvyukov@google.com>, "Jeff Dike" <jdike@addtoit.com>, "richard" <richard@nod.at>, "anton ivanov"
> <anton.ivanov@cambridgegreys.com>, "Andrey Ryabinin" <aryabinin@virtuozzo.com>, "Brendan Higgins"
> <brendanhiggins@google.com>, "davidgow" <davidgow@google.com>, "linux-um" <linux-um@lists.infradead.org>,
> "linux-kernel" <linux-kernel@vger.kernel.org>, "kasan-dev" <kasan-dev@googlegroups.com>
> Gesendet: Dienstag, 31. März 2020 18:39:21
> Betreff: Re: [PATCH] UML: add support for KASAN under x86_64

> On Mon, Mar 30, 2020 at 1:41 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>>
>> On Mon, 2020-03-30 at 10:38 +0200, Dmitry Vyukov wrote:
>> > On Mon, Mar 30, 2020 at 9:44 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>> > > On Fri, 2020-03-20 at 16:18 +0100, Dmitry Vyukov wrote:
>> > > > > Wait ... Now you say 0x7fbfffc000, but that is almost fine? I think you
>> > > > > confused the values - because I see, on userspace, the following:
>> > > >
>> > > > Oh, sorry, I copy-pasted wrong number. I meant 0x7fff8000.
>> > >
>> > > Right, ok.
>> > >
>> > > > Then I would expect 0x1000 0000 0000 to work, but you say it doesn't...
>> > >
>> > > So it just occurred to me - as I was mentioning this whole thing to
>> > > Richard - that there's probably somewhere some check about whether some
>> > > space is userspace or not.
>> > >
> 
> Yeah, it seems the "Kernel panic - not syncing: Segfault with no mm",
> "Kernel mode fault at addr...", and "Kernel tried to access user
> memory at addr..." errors all come from segv() in
> arch/um/kernel/trap.c due to what I think is this type of check
> whether the address is
> in userspace or not.

Segfault with no mm means that a (not fixable) pagefault happened while
kernel code ran.

>> > > I'm beginning to think that we shouldn't just map this outside of the
>> > > kernel memory system, but properly treat it as part of the memory that's
>> > > inside. And also use KASAN_VMALLOC.
>> > >
>> > > We can probably still have it at 0x7fff8000, just need to make sure we
>> > > actually map it? I tried with vm_area_add_early() but it didn't really
>> > > work once you have vmalloc() stuff...
>> >
> 
> What x86 does when KASAN_VMALLOC is disabled is make all vmalloc
> region accesses succeed by default
> by using the early shadow memory to have completely unpoisoned and
> unpoisonable read-only pages for all of vmalloc (which includes
> modules). When KASAN_VMALLOC is enabled in x86, the shadow memory is not
> allocated for the vmalloc region at startup. New chunks of shadow
> memory are allocated and unpoisoned every time there's a vmalloc()
> call. A similar thing might have to be done here by mprotect()ing
> the vmalloc space as read only, unpoisoned without KASAN_VMALLOC. This
> issue here is that
> kasan_init runs so early in the process that the vmalloc region for
> uml is not setup yet.
> 
> 
>> > But we do mmap it, no? See kasan_init() -> kasan_map_memory() -> mmap.
>>
>> Of course. But I meant inside the UML PTE system. We end up *unmapping*
>> it when loading modules, because it overlaps vmalloc space, and then we
>> vfree() something again, and unmap it ... because of the overlap.
>>
>> And if it's *not* in the vmalloc area, then the kernel doesn't consider
>> it valid, and we seem to often just fault when trying to determine
>> whether it's valid kernel memory or not ... Though I'm not really sure I
>> understand the failure part of this case well yet.
>>
> 
> I have been testing this issue in a multitude of ways and have only
> been getting more confused. It's still very unclear where exactly the
> problem occurs, mostly because the errors I found most frequently were
> reported in segv(), but the stack traces never contained segv.
> 
> Does anyone know if/how UML determines if memory being accessed is
> kernel or user memory?

In contrast to classic x86, without KPTI and SMAP/SMEP, UML has a strong
separation between user- and kernel-memory. This is also why copy_from/to_user()
is so expensive.

In arch/um/kernel/trap.c segv() you can see the logic.
Also see UPT_IS_USER().

Thanks,
//richard

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2020-03-11 22:44       ` Johannes Berg
@ 2022-05-24 10:34         ` Vincent Whitchurch
  2022-05-24 10:45           ` Johannes Berg
  2022-05-24 19:35           ` David Gow
  0 siblings, 2 replies; 38+ messages in thread
From: Vincent Whitchurch @ 2022-05-24 10:34 UTC (permalink / raw)
  To: Johannes Berg
  Cc: Patricia Alfonso, Jeff Dike, Richard Weinberger, anton.ivanov,
	Andrey Ryabinin, Dmitry Vyukov, Brendan Higgins, David Gow,
	kasan-dev, LKML, linux-um

On Wed, Mar 11, 2020 at 11:44:37PM +0100, Johannes Berg wrote:
> On Wed, 2020-03-11 at 15:32 -0700, Patricia Alfonso wrote:
> > I'll need some time to investigate these all myself. Having just
> > gotten my first module to run about an hour ago, any more information
> > about how you got these errors would be helpful so I can try to
> > reproduce them on my own.
> 
> See the other emails, I was basically just loading random modules. In my
> case cfg80211, mac80211, mac80211-hwsim - those are definitely available
> without any (virtio) hardware requirements, so you could use them.
> 
> Note that doing a bunch of vmalloc would likely result in similar
> issues, since the module and vmalloc space is the same on UML.

Old thread, but I had a look at this the other day and I think I got it
working.  Since the entire shadow area is mapped at init, we don't need
to do any mappings later.

It works both with and without KASAN_VMALLOC.  KASAN_STACK works too
after I disabled sanitization of the stacktrace code.  All kasan kunit
tests pass and the test_kasan.ko module works too.

Delta patch against Patricia's is below.  The CONFIG_UML checks need to
be replaced with something more appropriate (new config? __weak
functions?) and the free functions should probably be hooked up to
madvise(MADV_DONTNEED) so we discard unused pages in the shadow mapping.

Note that there's a KASAN stack-out-of-bounds splat on startup when just
booting UML.  That looks like a real (17-year-old) bug, I've posted a
fix for that:

 https://lore.kernel.org/lkml/20220523140403.2361040-1-vincent.whitchurch@axis.com/

8<-----------
diff --git a/arch/um/Kconfig b/arch/um/Kconfig
index a1bd8c07ce14..5f3a4d25d57e 100644
--- a/arch/um/Kconfig
+++ b/arch/um/Kconfig
@@ -12,6 +12,7 @@ config UML
 	select ARCH_NO_PREEMPT
 	select HAVE_ARCH_AUDITSYSCALL
 	select HAVE_ARCH_KASAN if X86_64
+	select HAVE_ARCH_KASAN_VMALLOC if HAVE_ARCH_KASAN
 	select HAVE_ARCH_SECCOMP_FILTER
 	select HAVE_ASM_MODVERSIONS
 	select HAVE_UID16
@@ -223,7 +224,7 @@ config UML_TIME_TRAVEL_SUPPORT
 config KASAN_SHADOW_OFFSET
 	hex
 	depends on KASAN
-	default 0x7fff8000
+	default 0x100000000000
 	help
 	  This is the offset at which the ~2.25TB of shadow memory is
 	  mapped and used by KASAN for memory debugging. This can be any
diff --git a/arch/um/kernel/Makefile b/arch/um/kernel/Makefile
index 1c2d4b29a3d4..a089217e2f0e 100644
--- a/arch/um/kernel/Makefile
+++ b/arch/um/kernel/Makefile
@@ -27,6 +27,9 @@ obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
 obj-$(CONFIG_STACKTRACE) += stacktrace.o
 obj-$(CONFIG_GENERIC_PCI_IOMAP) += ioport.o
 
+KASAN_SANITIZE_stacktrace.o := n
+KASAN_SANITIZE_sysrq.o := n
+
 USER_OBJS := config.o
 
 include arch/um/scripts/Makefile.rules
diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c
index 7c3196c297f7..a32cfce53efb 100644
--- a/arch/um/kernel/mem.c
+++ b/arch/um/kernel/mem.c
@@ -33,7 +33,7 @@ void kasan_init(void)
 }
 
 static void (*kasan_init_ptr)(void)
-__section(.kasan_init) __used
+__section(".kasan_init") __used
 = kasan_init;
 #endif
 
diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index 1113cf5fea25..1f3e620188a2 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -152,7 +152,7 @@ config KASAN_STACK
 	bool "Enable stack instrumentation (unsafe)" if CC_IS_CLANG && !COMPILE_TEST
 	depends on KASAN_GENERIC || KASAN_SW_TAGS
 	depends on !ARCH_DISABLE_KASAN_INLINE
-	default y if CC_IS_GCC && !UML
+	default y if CC_IS_GCC
 	help
 	  The LLVM stack address sanitizer has a know problem that
 	  causes excessive stack usage in a lot of functions, see
diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c
index a4f07de21771..d8c518bd0e7d 100644
--- a/mm/kasan/shadow.c
+++ b/mm/kasan/shadow.c
@@ -295,8 +295,14 @@ int kasan_populate_vmalloc(unsigned long addr, unsigned long size)
 		return 0;
 
 	shadow_start = (unsigned long)kasan_mem_to_shadow((void *)addr);
-	shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE);
 	shadow_end = (unsigned long)kasan_mem_to_shadow((void *)addr + size);
+
+	if (IS_ENABLED(CONFIG_UML)) {
+		__memset(kasan_mem_to_shadow((void *)addr), KASAN_VMALLOC_INVALID, shadow_end - shadow_start);
+		return 0;
+	}
+
+	shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE);
 	shadow_end = ALIGN(shadow_end, PAGE_SIZE);
 
 	ret = apply_to_page_range(&init_mm, shadow_start,
@@ -466,6 +472,10 @@ void kasan_release_vmalloc(unsigned long start, unsigned long end,
 
 	if (shadow_end > shadow_start) {
 		size = shadow_end - shadow_start;
+		if (IS_ENABLED(CONFIG_UML)) {
+			__memset(shadow_start, KASAN_SHADOW_INIT, shadow_end - shadow_start);
+			return;
+		}
 		apply_to_existing_page_range(&init_mm,
 					     (unsigned long)shadow_start,
 					     size, kasan_depopulate_vmalloc_pte,
@@ -531,6 +541,11 @@ int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask)
 	if (WARN_ON(!PAGE_ALIGNED(shadow_start)))
 		return -EINVAL;
 
+	if (IS_ENABLED(CONFIG_UML)) {
+		__memset((void *)shadow_start, KASAN_SHADOW_INIT, shadow_size);
+		return 0;
+	}
+
 	ret = __vmalloc_node_range(shadow_size, 1, shadow_start,
 			shadow_start + shadow_size,
 			GFP_KERNEL,
@@ -554,6 +569,9 @@ int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask)
 
 void kasan_free_module_shadow(const struct vm_struct *vm)
 {
+	if (IS_ENABLED(CONFIG_UML))
+		return;
+
 	if (vm->flags & VM_KASAN)
 		vfree(kasan_mem_to_shadow(vm->addr));
 }

^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2022-05-24 10:34         ` Vincent Whitchurch
@ 2022-05-24 10:45           ` Johannes Berg
  2022-05-24 19:35           ` David Gow
  1 sibling, 0 replies; 38+ messages in thread
From: Johannes Berg @ 2022-05-24 10:45 UTC (permalink / raw)
  To: Vincent Whitchurch
  Cc: Patricia Alfonso, Jeff Dike, Richard Weinberger, anton.ivanov,
	Andrey Ryabinin, Dmitry Vyukov, Brendan Higgins, David Gow,
	kasan-dev, LKML, linux-um

Hi Vincent,

> Old thread, but I had a look at this the other day and I think I got it
> working.  Since the entire shadow area is mapped at init, we don't need
> to do any mappings later.

Nice!! I've always wanted to get back to this too.

> It works both with and without KASAN_VMALLOC.  KASAN_STACK works too
> after I disabled sanitization of the stacktrace code.  All kasan kunit
> tests pass and the test_kasan.ko module works too.

:-)

> The CONFIG_UML checks need to
> be replaced with something more appropriate (new config? __weak
> functions?) and the free functions should probably be hooked up to
> madvise(MADV_DONTNEED) so we discard unused pages in the shadow
> mapping.

I guess a new config would be most appropriate - that way code can be
compiled out accordingly. But I don't know who maintains the KASAN code,
I guess have to discuss with them.

> Note that there's a KASAN stack-out-of-bounds splat on startup when just
> booting UML.  That looks like a real (17-year-old) bug, I've posted a
> fix for that:
> 
>  https://lore.kernel.org/lkml/20220523140403.2361040-1-vincent.whitchurch@axis.com/

Hah, right, I was wondering how that came up suddenly now... Almost
suprised it's just a single bug so far :)

> --- a/mm/kasan/shadow.c
> +++ b/mm/kasan/shadow.c
> @@ -295,8 +295,14 @@ int kasan_populate_vmalloc(unsigned long addr, unsigned long size)
>  		return 0;
>  
>  	shadow_start = (unsigned long)kasan_mem_to_shadow((void *)addr);
> -	shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE);
>  	shadow_end = (unsigned long)kasan_mem_to_shadow((void *)addr + size);
> +
> +	if (IS_ENABLED(CONFIG_UML)) {
> +		__memset(kasan_mem_to_shadow((void *)addr), KASAN_VMALLOC_INVALID, shadow_end - shadow_start);
> +		return 0;
> +	}
> 

If that were

	if (IS_ENABLED(CONFIG_KASAN_NO_SHADOW_ALLOC)) {
		...
	}

(or so) as discussed above, it might be a little more readable, but
otherwise it doesn't really seem all _that_ intrusive.

I'll give it a spin later.

johannes

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2022-05-24 10:34         ` Vincent Whitchurch
  2022-05-24 10:45           ` Johannes Berg
@ 2022-05-24 19:35           ` David Gow
  2022-05-25 11:17             ` Vincent Whitchurch
  1 sibling, 1 reply; 38+ messages in thread
From: David Gow @ 2022-05-24 19:35 UTC (permalink / raw)
  To: Vincent Whitchurch
  Cc: Johannes Berg, Patricia Alfonso, Jeff Dike, Richard Weinberger,
	Anton Ivanov, Andrey Ryabinin, Dmitry Vyukov, Brendan Higgins,
	kasan-dev, LKML, linux-um, Daniel Axtens

+dja in case he has any KASAN_VMALLOC thoughts.

On Tue, May 24, 2022 at 3:34 AM Vincent Whitchurch
<vincent.whitchurch@axis.com> wrote:
>
> On Wed, Mar 11, 2020 at 11:44:37PM +0100, Johannes Berg wrote:
> > On Wed, 2020-03-11 at 15:32 -0700, Patricia Alfonso wrote:
> > > I'll need some time to investigate these all myself. Having just
> > > gotten my first module to run about an hour ago, any more information
> > > about how you got these errors would be helpful so I can try to
> > > reproduce them on my own.
> >
> > See the other emails, I was basically just loading random modules. In my
> > case cfg80211, mac80211, mac80211-hwsim - those are definitely available
> > without any (virtio) hardware requirements, so you could use them.
> >
> > Note that doing a bunch of vmalloc would likely result in similar
> > issues, since the module and vmalloc space is the same on UML.
>
> Old thread, but I had a look at this the other day and I think I got it
> working.  Since the entire shadow area is mapped at init, we don't need
> to do any mappings later.

Wow -- thanks for looking at this again. It's been on my to-do list
for quite a while, too. I'd somewhat resigned myself to having to
re-implement the shadow memory stuff on top of page allocation
functions, so I'm particularly thrilled to see this working without
needing to do that.

>
> It works both with and without KASAN_VMALLOC.  KASAN_STACK works too
> after I disabled sanitization of the stacktrace code.  All kasan kunit
> tests pass and the test_kasan.ko module works too.

I've got this running myself, and can confirm the kasan tests work
under kunit_tool in most cases, though there are a couple of failures
when built with clang/llvm:
[11:56:30] # kasan_global_oob_right: EXPECTATION FAILED at lib/test_kasan.c:732
[11:56:30] KASAN failure expected in "*(volatile char *)p", but none occurred
[11:56:30] not ok 32 - kasan_global_oob_right
[11:56:30] [FAILED] kasan_global_oob_right
[11:56:30] # kasan_global_oob_left: EXPECTATION FAILED at lib/test_kasan.c:746
[11:56:30] KASAN failure expected in "*(volatile char *)p", but none occurred
[11:56:30] not ok 33 - kasan_global_oob_left
[11:56:30] [FAILED] kasan_global_oob_left

The global_oob_left test doesn't work on gcc either (but fails on all
architectures, so is disabled), but kasan_global_oob_right should work
in theory.

>
> Delta patch against Patricia's is below.  The CONFIG_UML checks need to
> be replaced with something more appropriate (new config? __weak
> functions?) and the free functions should probably be hooked up to
> madvise(MADV_DONTNEED) so we discard unused pages in the shadow mapping.

I'd probably go with a new config here, rather than using __weak
functions. Either have a "shadow already allocated" config like the
CONFIG_KASAN_NO_SHADOW_ALLOC Johannes suggests, or something like
CONFIG_KASAN_HAS_ARCH_SHADOW_ALLOC, and call into an
architecture-specific "shadow allocator", which would just do the
__memset(). The latter would make adding the madvise(MADV_DONTNEED)
easier, I think, though it's more work in general. Ultimately a
question for the KASAN folks, though.

> Note that there's a KASAN stack-out-of-bounds splat on startup when just
> booting UML.  That looks like a real (17-year-old) bug, I've posted a
> fix for that:
>
>  https://lore.kernel.org/lkml/20220523140403.2361040-1-vincent.whitchurch@axis.com/
>

Wow, that's a good catch. And also explains a bit why I was so
confused trying to understand that code when we were originally
looking at this.

> 8<-----------
> diff --git a/arch/um/Kconfig b/arch/um/Kconfig
> index a1bd8c07ce14..5f3a4d25d57e 100644
> --- a/arch/um/Kconfig
> +++ b/arch/um/Kconfig
> @@ -12,6 +12,7 @@ config UML
>         select ARCH_NO_PREEMPT
>         select HAVE_ARCH_AUDITSYSCALL
>         select HAVE_ARCH_KASAN if X86_64
> +       select HAVE_ARCH_KASAN_VMALLOC if HAVE_ARCH_KASAN
>         select HAVE_ARCH_SECCOMP_FILTER
>         select HAVE_ASM_MODVERSIONS
>         select HAVE_UID16
> @@ -223,7 +224,7 @@ config UML_TIME_TRAVEL_SUPPORT
>  config KASAN_SHADOW_OFFSET
>         hex
>         depends on KASAN
> -       default 0x7fff8000
> +       default 0x100000000000
>         help
>           This is the offset at which the ~2.25TB of shadow memory is
>           mapped and used by KASAN for memory debugging. This can be any
> diff --git a/arch/um/kernel/Makefile b/arch/um/kernel/Makefile
> index 1c2d4b29a3d4..a089217e2f0e 100644
> --- a/arch/um/kernel/Makefile
> +++ b/arch/um/kernel/Makefile
> @@ -27,6 +27,9 @@ obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
>  obj-$(CONFIG_STACKTRACE) += stacktrace.o
>  obj-$(CONFIG_GENERIC_PCI_IOMAP) += ioport.o
>
> +KASAN_SANITIZE_stacktrace.o := n
> +KASAN_SANITIZE_sysrq.o := n
> +
>  USER_OBJS := config.o
>
>  include arch/um/scripts/Makefile.rules
> diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c
> index 7c3196c297f7..a32cfce53efb 100644
> --- a/arch/um/kernel/mem.c
> +++ b/arch/um/kernel/mem.c
> @@ -33,7 +33,7 @@ void kasan_init(void)
>  }
>
>  static void (*kasan_init_ptr)(void)
> -__section(.kasan_init) __used
> +__section(".kasan_init") __used
>  = kasan_init;
>  #endif
>
> diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> index 1113cf5fea25..1f3e620188a2 100644
> --- a/lib/Kconfig.kasan
> +++ b/lib/Kconfig.kasan
> @@ -152,7 +152,7 @@ config KASAN_STACK
>         bool "Enable stack instrumentation (unsafe)" if CC_IS_CLANG && !COMPILE_TEST
>         depends on KASAN_GENERIC || KASAN_SW_TAGS
>         depends on !ARCH_DISABLE_KASAN_INLINE
> -       default y if CC_IS_GCC && !UML
> +       default y if CC_IS_GCC
>         help
>           The LLVM stack address sanitizer has a know problem that
>           causes excessive stack usage in a lot of functions, see
> diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c
> index a4f07de21771..d8c518bd0e7d 100644
> --- a/mm/kasan/shadow.c
> +++ b/mm/kasan/shadow.c
> @@ -295,8 +295,14 @@ int kasan_populate_vmalloc(unsigned long addr, unsigned long size)
>                 return 0;
>
>         shadow_start = (unsigned long)kasan_mem_to_shadow((void *)addr);
> -       shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE);
>         shadow_end = (unsigned long)kasan_mem_to_shadow((void *)addr + size);
> +
> +       if (IS_ENABLED(CONFIG_UML)) {
> +               __memset(kasan_mem_to_shadow((void *)addr), KASAN_VMALLOC_INVALID, shadow_end - shadow_start);
> +               return 0;
> +       }
> +
> +       shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE);
>         shadow_end = ALIGN(shadow_end, PAGE_SIZE);

Is there a particular reason we're not doing the rounding under UML,
particularly since I think it's happening anyway in
kasan_release_vmalloc() below. (I get that it's not really necessary,
but is there an actual bug you've noticed with it?)

>
>         ret = apply_to_page_range(&init_mm, shadow_start,
> @@ -466,6 +472,10 @@ void kasan_release_vmalloc(unsigned long start, unsigned long end,
>
>         if (shadow_end > shadow_start) {
>                 size = shadow_end - shadow_start;
> +               if (IS_ENABLED(CONFIG_UML)) {
> +                       __memset(shadow_start, KASAN_SHADOW_INIT, shadow_end - shadow_start);
> +                       return;
> +               }
>                 apply_to_existing_page_range(&init_mm,
>                                              (unsigned long)shadow_start,
>                                              size, kasan_depopulate_vmalloc_pte,
> @@ -531,6 +541,11 @@ int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask)
>         if (WARN_ON(!PAGE_ALIGNED(shadow_start)))
>                 return -EINVAL;
>
> +       if (IS_ENABLED(CONFIG_UML)) {
> +               __memset((void *)shadow_start, KASAN_SHADOW_INIT, shadow_size);
> +               return 0;
> +       }
> +
>         ret = __vmalloc_node_range(shadow_size, 1, shadow_start,
>                         shadow_start + shadow_size,
>                         GFP_KERNEL,
> @@ -554,6 +569,9 @@ int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask)
>
>  void kasan_free_module_shadow(const struct vm_struct *vm)
>  {
> +       if (IS_ENABLED(CONFIG_UML))
> +               return;
> +
>         if (vm->flags & VM_KASAN)
>                 vfree(kasan_mem_to_shadow(vm->addr));
>  }

In any case, this looks pretty great to me. I still definitely want to
play with it a bit more, particularly with various module loads -- and
it'd be great to track down why those global_oob tests are failing --
but I'm definitely hopeful that we can finish this off and get it
upstream.

It's probably worth sending a new rebased/combined patch out which has
your fixes and applies more cleanly on recent kernels. (I've got a
working tree here, so I can do that if you'd prefer.)

Cheers,
-- David

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH] UML: add support for KASAN under x86_64
  2022-05-24 19:35           ` David Gow
@ 2022-05-25 11:17             ` Vincent Whitchurch
  2022-05-26  1:01               ` [RFC PATCH v3] " David Gow
  0 siblings, 1 reply; 38+ messages in thread
From: Vincent Whitchurch @ 2022-05-25 11:17 UTC (permalink / raw)
  To: David Gow
  Cc: Johannes Berg, Patricia Alfonso, Jeff Dike, Richard Weinberger,
	Anton Ivanov, Andrey Ryabinin, Dmitry Vyukov, Brendan Higgins,
	kasan-dev, LKML, linux-um, Daniel Axtens

On Tue, May 24, 2022 at 09:35:33PM +0200, David Gow wrote:
> On Tue, May 24, 2022 at 3:34 AM Vincent Whitchurch
> <vincent.whitchurch@axis.com> wrote:
> > It works both with and without KASAN_VMALLOC.  KASAN_STACK works too
> > after I disabled sanitization of the stacktrace code.  All kasan kunit
> > tests pass and the test_kasan.ko module works too.
> 
> I've got this running myself, and can confirm the kasan tests work
> under kunit_tool in most cases, though there are a couple of failures
> when built with clang/llvm:
> [11:56:30] # kasan_global_oob_right: EXPECTATION FAILED at lib/test_kasan.c:732
> [11:56:30] KASAN failure expected in "*(volatile char *)p", but none occurred
> [11:56:30] not ok 32 - kasan_global_oob_right
> [11:56:30] [FAILED] kasan_global_oob_right
> [11:56:30] # kasan_global_oob_left: EXPECTATION FAILED at lib/test_kasan.c:746
> [11:56:30] KASAN failure expected in "*(volatile char *)p", but none occurred
> [11:56:30] not ok 33 - kasan_global_oob_left
> [11:56:30] [FAILED] kasan_global_oob_left
> 
> The global_oob_left test doesn't work on gcc either (but fails on all
> architectures, so is disabled), but kasan_global_oob_right should work
> in theory.

kasan_global_oob_right works for me with GCC, but it looks like
__asan_register_globals() never gets called when built with clang.  This
fixes it:

diff --git a/arch/um/include/asm/common.lds.S b/arch/um/include/asm/common.lds.S
index 731f8c8422a2..fd481ac371de 100644
--- a/arch/um/include/asm/common.lds.S
+++ b/arch/um/include/asm/common.lds.S
@@ -84,6 +84,7 @@
   .init_array : {
 	__init_array_start = .;
 	*(.kasan_init)
+	*(.init_array.*)
 	*(.init_array)
 	__init_array_end = .;
   }

With that:

[13:12:15] =================== kasan (55 subtests) ====================
[13:12:15] [PASSED] kmalloc_oob_right
[13:12:15] [PASSED] kmalloc_oob_left
[13:12:15] [PASSED] kmalloc_node_oob_right
[13:12:15] [PASSED] kmalloc_pagealloc_oob_right
[13:12:15] [PASSED] kmalloc_pagealloc_uaf
[13:12:15] [PASSED] kmalloc_pagealloc_invalid_free
[13:12:15] [SKIPPED] pagealloc_oob_right
[13:12:15] [PASSED] pagealloc_uaf
[13:12:15] [PASSED] kmalloc_large_oob_right
[13:12:15] [PASSED] krealloc_more_oob
[13:12:15] [PASSED] krealloc_less_oob
[13:12:15] [PASSED] krealloc_pagealloc_more_oob
[13:12:15] [PASSED] krealloc_pagealloc_less_oob
[13:12:15] [PASSED] krealloc_uaf
[13:12:15] [PASSED] kmalloc_oob_16
[13:12:15] [PASSED] kmalloc_uaf_16
[13:12:15] [PASSED] kmalloc_oob_in_memset
[13:12:15] [PASSED] kmalloc_oob_memset_2
[13:12:15] [PASSED] kmalloc_oob_memset_4
[13:12:15] [PASSED] kmalloc_oob_memset_8
[13:12:15] [PASSED] kmalloc_oob_memset_16
[13:12:15] [PASSED] kmalloc_memmove_negative_size
[13:12:15] [PASSED] kmalloc_memmove_invalid_size
[13:12:15] [PASSED] kmalloc_uaf
[13:12:15] [PASSED] kmalloc_uaf_memset
[13:12:15] [PASSED] kmalloc_uaf2
[13:12:15] [PASSED] kfree_via_page
[13:12:15] [PASSED] kfree_via_phys
[13:12:15] [PASSED] kmem_cache_oob
[13:12:15] [PASSED] kmem_cache_accounted
[13:12:15] [PASSED] kmem_cache_bulk
[13:12:15] [PASSED] kasan_global_oob_right
[13:12:15] [PASSED] kasan_global_oob_left
[13:12:15] [PASSED] kasan_stack_oob
[13:12:15] [PASSED] kasan_alloca_oob_left
[13:12:15] [PASSED] kasan_alloca_oob_right
[13:12:15] [PASSED] ksize_unpoisons_memory
[13:12:15] [PASSED] ksize_uaf
[13:12:15] [PASSED] kmem_cache_double_free
[13:12:15] [PASSED] kmem_cache_invalid_free
[13:12:15] [PASSED] kmem_cache_double_destroy
[13:12:15] [PASSED] kasan_memchr
[13:12:15] [PASSED] kasan_memcmp
[13:12:15] [PASSED] kasan_strings
[13:12:15] [PASSED] kasan_bitops_generic
[13:12:15] [SKIPPED] kasan_bitops_tags
[13:12:15] [PASSED] kmalloc_double_kzfree
[13:12:15] [SKIPPED] vmalloc_helpers_tags
[13:12:15] [PASSED] vmalloc_oob
[13:12:15] [SKIPPED] vmap_tags
[13:12:15] [SKIPPED] vm_map_ram_tags
[13:12:15] [SKIPPED] vmalloc_percpu
[13:12:15] [SKIPPED] match_all_not_assigned
[13:12:15] [SKIPPED] match_all_ptr_tag
[13:12:15] [SKIPPED] match_all_mem_tag
[13:12:15] ====================== [PASSED] kasan ======================
[13:12:15] ============================================================
[13:12:15] Testing complete. Passed: 46, Failed: 0, Crashed: 0, Skipped: 9, Errors: 0

> > diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c
> > index a4f07de21771..d8c518bd0e7d 100644
> > --- a/mm/kasan/shadow.c
> > +++ b/mm/kasan/shadow.c
> > @@ -295,8 +295,14 @@ int kasan_populate_vmalloc(unsigned long addr, unsigned long size)
> >                 return 0;
> >
> >         shadow_start = (unsigned long)kasan_mem_to_shadow((void *)addr);
> > -       shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE);
> >         shadow_end = (unsigned long)kasan_mem_to_shadow((void *)addr + size);
> > +
> > +       if (IS_ENABLED(CONFIG_UML)) {
> > +               __memset(kasan_mem_to_shadow((void *)addr), KASAN_VMALLOC_INVALID, shadow_end - shadow_start);
> > +               return 0;
> > +       }
> > +
> > +       shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE);
> >         shadow_end = ALIGN(shadow_end, PAGE_SIZE);
> 
> Is there a particular reason we're not doing the rounding under UML,
> particularly since I think it's happening anyway in
> kasan_release_vmalloc() below. (I get that it's not really necessary,
> but is there an actual bug you've noticed with it?)

No, I didn't notice any bug.

> >         ret = apply_to_page_range(&init_mm, shadow_start,
> > @@ -466,6 +472,10 @@ void kasan_release_vmalloc(unsigned long start, unsigned long end,
> >
> >         if (shadow_end > shadow_start) {
> >                 size = shadow_end - shadow_start;
> > +               if (IS_ENABLED(CONFIG_UML)) {
> > +                       __memset(shadow_start, KASAN_SHADOW_INIT, shadow_end - shadow_start);
> > +                       return;
> > +               }
> >                 apply_to_existing_page_range(&init_mm,
> >                                              (unsigned long)shadow_start,
> >                                              size, kasan_depopulate_vmalloc_pte,
> > @@ -531,6 +541,11 @@ int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask)
> >         if (WARN_ON(!PAGE_ALIGNED(shadow_start)))
> >                 return -EINVAL;
> >
> > +       if (IS_ENABLED(CONFIG_UML)) {
> > +               __memset((void *)shadow_start, KASAN_SHADOW_INIT, shadow_size);
> > +               return 0;
> > +       }
> > +
> >         ret = __vmalloc_node_range(shadow_size, 1, shadow_start,
> >                         shadow_start + shadow_size,
> >                         GFP_KERNEL,
> > @@ -554,6 +569,9 @@ int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask)
> >
> >  void kasan_free_module_shadow(const struct vm_struct *vm)
> >  {
> > +       if (IS_ENABLED(CONFIG_UML))
> > +               return;
> > +
> >         if (vm->flags & VM_KASAN)
> >                 vfree(kasan_mem_to_shadow(vm->addr));
> >  }
> 
> In any case, this looks pretty great to me. I still definitely want to
> play with it a bit more, particularly with various module loads -- and
> it'd be great to track down why those global_oob tests are failing --
> but I'm definitely hopeful that we can finish this off and get it
> upstream.
> 
> It's probably worth sending a new rebased/combined patch out which has
> your fixes and applies more cleanly on recent kernels. (I've got a
> working tree here, so I can do that if you'd prefer.)

Please feel free to do so.  Thanks!

^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [RFC PATCH v3] UML: add support for KASAN under x86_64
  2022-05-25 11:17             ` Vincent Whitchurch
@ 2022-05-26  1:01               ` David Gow
  2022-05-26  9:29                 ` Johannes Berg
                                   ` (3 more replies)
  0 siblings, 4 replies; 38+ messages in thread
From: David Gow @ 2022-05-26  1:01 UTC (permalink / raw)
  To: Vincent Whitchurch, Johannes Berg, Patricia Alfonso, Jeff Dike,
	Richard Weinberger, anton.ivanov, Dmitry Vyukov, Brendan Higgins
  Cc: kasan-dev, linux-um, LKML, Daniel Latypov, David Gow

From: Patricia Alfonso <trishalfonso@google.com>

Make KASAN run on User Mode Linux on x86_64.

The UML-specific KASAN initializer uses mmap to map the roughly 2.25TB
of shadow memory to the location defined by KASAN_SHADOW_OFFSET.
kasan_init() utilizes constructors to initialize KASAN before main().

The location of the KASAN shadow memory, starting at
KASAN_SHADOW_OFFSET, can be configured using the KASAN_SHADOW_OFFSET
option. UML uses roughly 18TB of address space, and KASAN requires 1/8th
of this. The default location of this offset is 0x100000000000, which
keeps it out-of-the-way even on UML setups with more "physical" memory.

For low-memory setups, 0x7fff8000 can be used instead, which fits in an
immediate and is therefore faster, as suggested by Dmitry Vyukov. There
is usually enough free space at this location; however, it is a config
option so that it can be easily changed if needed.

Note that, unlike KASAN on other architectures, vmalloc allocations
still use the shadow memory allocated upfront, rather than allocating
and free-ing it per-vmalloc allocation.

Signed-off-by: Patricia Alfonso <trishalfonso@google.com>
Co-developed-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
Signed-off-by: David Gow <davidgow@google.com>
---

This is a new RFC for the KASAN/UML port, based on the patch v1:
https://lore.kernel.org/all/20200226004608.8128-1-trishalfonso@google.com/

With several fixes by Vincent Whitchurch:
https://lore.kernel.org/all/20220525111756.GA15955@axis.com/

That thread describes the differences from the v1 (and hence the
previous RFCs better than I can here), but the gist of it is:
- Support for KASAN_VMALLOC, by changing the way
  kasan_{populate,release}_vmalloc work to update existing shadow
  memory, rather than allocating anything new.
- A similar fix for modules' shadow memory.
- Support for KASAN_STACK
  - This requires the bugfix here:
https://lore.kernel.org/lkml/20220523140403.2361040-1-vincent.whitchurch@axis.com/
  - Plus a couple of files excluded from KASAN.
- Revert the default shadow offset to 0x100000000000
  - This was breaking when mem=1G for me, at least.
- A few minor fixes to linker sections and scripts.
  - I've added one to dyn.lds.S on top of the ones Vincent added.

There are still a few things to be sorted out before this is ready to go
upstream, in particular:
- We've got a bunch of checks for CONFIG_UML, where a more specific
  config option might be better. For example: CONFIG_KASAN_NO_SHADOW_ALLOC.
- Alternatively, the vmalloc (and module) shadow memory allocators could
  support per-architecture replacements.
- Do we want to the alignment before or after the __memset() in
  kasan_populate_vmalloc()?
- This doesn't seem to work when CONFIG_STATIC_LINK is enabled (because
  libc crt0 code calls memory functions, which expect the shadow memory
  to already exist, due to multiple symbols being resolved.
  - I think we should just make this depend on dynamic UML.
  - For that matter, I think static UML is actually broken at the
    moment. I'll send a patch out tomorrow.
- And there's a checkpatch complaint about a long __memset() line.

Thanks again to everyone who's contributed and looked at these patches!
Note that I removed the Reviewed-by tags, as I think this version has
enough changes to warrant a re-review.

-- David

---
 arch/um/Kconfig                  | 15 +++++++++++++++
 arch/um/Makefile                 |  6 ++++++
 arch/um/include/asm/common.lds.S |  2 ++
 arch/um/kernel/Makefile          |  3 +++
 arch/um/kernel/dyn.lds.S         |  6 +++++-
 arch/um/kernel/mem.c             | 18 ++++++++++++++++++
 arch/um/os-Linux/mem.c           | 22 ++++++++++++++++++++++
 arch/um/os-Linux/user_syms.c     |  4 ++--
 arch/x86/um/Makefile             |  3 ++-
 arch/x86/um/vdso/Makefile        |  3 +++
 mm/kasan/shadow.c                | 20 +++++++++++++++++++-
 11 files changed, 97 insertions(+), 5 deletions(-)

diff --git a/arch/um/Kconfig b/arch/um/Kconfig
index 4d398b80aea8..c28ea5c89381 100644
--- a/arch/um/Kconfig
+++ b/arch/um/Kconfig
@@ -11,6 +11,8 @@ config UML
 	select ARCH_HAS_STRNLEN_USER
 	select ARCH_NO_PREEMPT
 	select HAVE_ARCH_AUDITSYSCALL
+	select HAVE_ARCH_KASAN if X86_64
+	select HAVE_ARCH_KASAN_VMALLOC if HAVE_ARCH_KASAN
 	select HAVE_ARCH_SECCOMP_FILTER
 	select HAVE_ASM_MODVERSIONS
 	select HAVE_UID16
@@ -219,6 +221,19 @@ config UML_TIME_TRAVEL_SUPPORT
 
 	  It is safe to say Y, but you probably don't need this.
 
+config KASAN_SHADOW_OFFSET
+	hex
+	depends on KASAN
+	default 0x100000000000
+	help
+	  This is the offset at which the ~2.25TB of shadow memory is
+	  mapped and used by KASAN for memory debugging. This can be any
+	  address that has at least KASAN_SHADOW_SIZE(total address space divided
+	  by 8) amount of space so that the KASAN shadow memory does not conflict
+	  with anything. The default is 0x100000000000, which works even if mem is
+	  set to a large value. On low-memory systems, try 0x7fff8000, as it fits
+	  into the immediate of most instructions, improving performance.
+
 endmenu
 
 source "arch/um/drivers/Kconfig"
diff --git a/arch/um/Makefile b/arch/um/Makefile
index f2fe63bfd819..a98405f4ecb8 100644
--- a/arch/um/Makefile
+++ b/arch/um/Makefile
@@ -75,6 +75,12 @@ USER_CFLAGS = $(patsubst $(KERNEL_DEFINES),,$(patsubst -I%,,$(KBUILD_CFLAGS))) \
 		-D_FILE_OFFSET_BITS=64 -idirafter $(srctree)/include \
 		-idirafter $(objtree)/include -D__KERNEL__ -D__UM_HOST__
 
+# Kernel config options are not included in USER_CFLAGS, but the option for KASAN
+# should be included if the KASAN config option was set.
+ifdef CONFIG_KASAN
+	USER_CFLAGS+=-DCONFIG_KASAN=y
+endif
+
 #This will adjust *FLAGS accordingly to the platform.
 include $(srctree)/$(ARCH_DIR)/Makefile-os-$(OS)
 
diff --git a/arch/um/include/asm/common.lds.S b/arch/um/include/asm/common.lds.S
index eca6c452a41b..fd481ac371de 100644
--- a/arch/um/include/asm/common.lds.S
+++ b/arch/um/include/asm/common.lds.S
@@ -83,6 +83,8 @@
   }
   .init_array : {
 	__init_array_start = .;
+	*(.kasan_init)
+	*(.init_array.*)
 	*(.init_array)
 	__init_array_end = .;
   }
diff --git a/arch/um/kernel/Makefile b/arch/um/kernel/Makefile
index 1c2d4b29a3d4..a089217e2f0e 100644
--- a/arch/um/kernel/Makefile
+++ b/arch/um/kernel/Makefile
@@ -27,6 +27,9 @@ obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
 obj-$(CONFIG_STACKTRACE) += stacktrace.o
 obj-$(CONFIG_GENERIC_PCI_IOMAP) += ioport.o
 
+KASAN_SANITIZE_stacktrace.o := n
+KASAN_SANITIZE_sysrq.o := n
+
 USER_OBJS := config.o
 
 include arch/um/scripts/Makefile.rules
diff --git a/arch/um/kernel/dyn.lds.S b/arch/um/kernel/dyn.lds.S
index 2f2a8ce92f1e..2b7fc5b54164 100644
--- a/arch/um/kernel/dyn.lds.S
+++ b/arch/um/kernel/dyn.lds.S
@@ -109,7 +109,11 @@ SECTIONS
      be empty, which isn't pretty.  */
   . = ALIGN(32 / 8);
   .preinit_array     : { *(.preinit_array) }
-  .init_array     : { *(.init_array) }
+  .init_array     : {
+    *(.kasan_init)
+    *(.init_array.*)
+    *(.init_array)
+  }
   .fini_array     : { *(.fini_array) }
   .data           : {
     INIT_TASK_DATA(KERNEL_STACK_SIZE)
diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c
index 15295c3237a0..a32cfce53efb 100644
--- a/arch/um/kernel/mem.c
+++ b/arch/um/kernel/mem.c
@@ -18,6 +18,24 @@
 #include <kern_util.h>
 #include <mem_user.h>
 #include <os.h>
+#include <linux/sched/task.h>
+
+#ifdef CONFIG_KASAN
+void kasan_init(void)
+{
+	/*
+	 * kasan_map_memory will map all of the required address space and
+	 * the host machine will allocate physical memory as necessary.
+	 */
+	kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE);
+	init_task.kasan_depth = 0;
+	os_info("KernelAddressSanitizer initialized\n");
+}
+
+static void (*kasan_init_ptr)(void)
+__section(".kasan_init") __used
+= kasan_init;
+#endif
 
 /* allocated in paging_init, zeroed in mem_init, and unchanged thereafter */
 unsigned long *empty_zero_page = NULL;
diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c
index 3c1b77474d2d..8530b2e08604 100644
--- a/arch/um/os-Linux/mem.c
+++ b/arch/um/os-Linux/mem.c
@@ -17,6 +17,28 @@
 #include <init.h>
 #include <os.h>
 
+/*
+ * kasan_map_memory - maps memory from @start with a size of @len.
+ * The allocated memory is filled with zeroes upon success.
+ * @start: the start address of the memory to be mapped
+ * @len: the length of the memory to be mapped
+ *
+ * This function is used to map shadow memory for KASAN in uml
+ */
+void kasan_map_memory(void *start, size_t len)
+{
+	if (mmap(start,
+		 len,
+		 PROT_READ|PROT_WRITE,
+		 MAP_FIXED|MAP_ANONYMOUS|MAP_PRIVATE|MAP_NORESERVE,
+		 -1,
+		 0) == MAP_FAILED) {
+		os_info("Couldn't allocate shadow memory: %s\n.",
+			strerror(errno));
+		exit(1);
+	}
+}
+
 /* Set by make_tempfile() during early boot. */
 static char *tempdir = NULL;
 
diff --git a/arch/um/os-Linux/user_syms.c b/arch/um/os-Linux/user_syms.c
index 715594fe5719..cb667c9225ab 100644
--- a/arch/um/os-Linux/user_syms.c
+++ b/arch/um/os-Linux/user_syms.c
@@ -27,10 +27,10 @@ EXPORT_SYMBOL(strstr);
 #ifndef __x86_64__
 extern void *memcpy(void *, const void *, size_t);
 EXPORT_SYMBOL(memcpy);
-#endif
-
 EXPORT_SYMBOL(memmove);
 EXPORT_SYMBOL(memset);
+#endif
+
 EXPORT_SYMBOL(printf);
 
 /* Here, instead, I can provide a fake prototype. Yes, someone cares: genksyms.
diff --git a/arch/x86/um/Makefile b/arch/x86/um/Makefile
index ba5789c35809..f778e37494ba 100644
--- a/arch/x86/um/Makefile
+++ b/arch/x86/um/Makefile
@@ -28,7 +28,8 @@ else
 
 obj-y += syscalls_64.o vdso/
 
-subarch-y = ../lib/csum-partial_64.o ../lib/memcpy_64.o ../entry/thunk_64.o
+subarch-y = ../lib/csum-partial_64.o ../lib/memcpy_64.o ../entry/thunk_64.o \
+	../lib/memmove_64.o ../lib/memset_64.o
 
 endif
 
diff --git a/arch/x86/um/vdso/Makefile b/arch/x86/um/vdso/Makefile
index 5943387e3f35..8c0396fd0e6f 100644
--- a/arch/x86/um/vdso/Makefile
+++ b/arch/x86/um/vdso/Makefile
@@ -3,6 +3,9 @@
 # Building vDSO images for x86.
 #
 
+# do not instrument on vdso because KASAN is not compatible with user mode
+KASAN_SANITIZE			:= n
+
 # Prevents link failures: __sanitizer_cov_trace_pc() is not linked in.
 KCOV_INSTRUMENT                := n
 
diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c
index a4f07de21771..d8c518bd0e7d 100644
--- a/mm/kasan/shadow.c
+++ b/mm/kasan/shadow.c
@@ -295,8 +295,14 @@ int kasan_populate_vmalloc(unsigned long addr, unsigned long size)
 		return 0;
 
 	shadow_start = (unsigned long)kasan_mem_to_shadow((void *)addr);
-	shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE);
 	shadow_end = (unsigned long)kasan_mem_to_shadow((void *)addr + size);
+
+	if (IS_ENABLED(CONFIG_UML)) {
+		__memset(kasan_mem_to_shadow((void *)addr), KASAN_VMALLOC_INVALID, shadow_end - shadow_start);
+		return 0;
+	}
+
+	shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE);
 	shadow_end = ALIGN(shadow_end, PAGE_SIZE);
 
 	ret = apply_to_page_range(&init_mm, shadow_start,
@@ -466,6 +472,10 @@ void kasan_release_vmalloc(unsigned long start, unsigned long end,
 
 	if (shadow_end > shadow_start) {
 		size = shadow_end - shadow_start;
+		if (IS_ENABLED(CONFIG_UML)) {
+			__memset(shadow_start, KASAN_SHADOW_INIT, shadow_end - shadow_start);
+			return;
+		}
 		apply_to_existing_page_range(&init_mm,
 					     (unsigned long)shadow_start,
 					     size, kasan_depopulate_vmalloc_pte,
@@ -531,6 +541,11 @@ int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask)
 	if (WARN_ON(!PAGE_ALIGNED(shadow_start)))
 		return -EINVAL;
 
+	if (IS_ENABLED(CONFIG_UML)) {
+		__memset((void *)shadow_start, KASAN_SHADOW_INIT, shadow_size);
+		return 0;
+	}
+
 	ret = __vmalloc_node_range(shadow_size, 1, shadow_start,
 			shadow_start + shadow_size,
 			GFP_KERNEL,
@@ -554,6 +569,9 @@ int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask)
 
 void kasan_free_module_shadow(const struct vm_struct *vm)
 {
+	if (IS_ENABLED(CONFIG_UML))
+		return;
+
 	if (vm->flags & VM_KASAN)
 		vfree(kasan_mem_to_shadow(vm->addr));
 }
-- 
2.36.1.124.g0e6072fb45-goog


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [RFC PATCH v3] UML: add support for KASAN under x86_64
  2022-05-26  1:01               ` [RFC PATCH v3] " David Gow
@ 2022-05-26  9:29                 ` Johannes Berg
  2022-05-27  5:31                 ` Dmitry Vyukov
                                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 38+ messages in thread
From: Johannes Berg @ 2022-05-26  9:29 UTC (permalink / raw)
  To: David Gow, Vincent Whitchurch, Patricia Alfonso, Jeff Dike,
	Richard Weinberger, anton.ivanov, Dmitry Vyukov, Brendan Higgins
  Cc: kasan-dev, linux-um, LKML, Daniel Latypov

On Wed, 2022-05-25 at 18:01 -0700, David Gow wrote:
> 
> +#ifdef CONFIG_KASAN
> +void kasan_init(void)
> +{
> +	/*
> +	 * kasan_map_memory will map all of the required address space and
> +	 * the host machine will allocate physical memory as necessary.
> +	 */
> +	kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE);
> +	init_task.kasan_depth = 0;
> +	os_info("KernelAddressSanitizer initialized\n");
> 

Can we remove this? Or maybe print it later somehow, when the other
KASAN machinery initializes?

As it is, this gets printed even if you run just "./linux --version" or
"--help", which is a bit strange.

johannes

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC PATCH v3] UML: add support for KASAN under x86_64
  2022-05-26  1:01               ` [RFC PATCH v3] " David Gow
  2022-05-26  9:29                 ` Johannes Berg
@ 2022-05-27  5:31                 ` Dmitry Vyukov
  2022-05-27  7:32                   ` Johannes Berg
  2022-05-27 10:36                 ` Johannes Berg
  2022-05-27 13:05                 ` Johannes Berg
  3 siblings, 1 reply; 38+ messages in thread
From: Dmitry Vyukov @ 2022-05-27  5:31 UTC (permalink / raw)
  To: David Gow
  Cc: Vincent Whitchurch, Johannes Berg, Patricia Alfonso, Jeff Dike,
	Richard Weinberger, anton.ivanov, Brendan Higgins, kasan-dev,
	linux-um, LKML, Daniel Latypov

On Thu, 26 May 2022 at 03:02, David Gow <davidgow@google.com> wrote:
>
> From: Patricia Alfonso <trishalfonso@google.com>
>
> Make KASAN run on User Mode Linux on x86_64.
>
> The UML-specific KASAN initializer uses mmap to map the roughly 2.25TB
> of shadow memory to the location defined by KASAN_SHADOW_OFFSET.
> kasan_init() utilizes constructors to initialize KASAN before main().
>
> The location of the KASAN shadow memory, starting at
> KASAN_SHADOW_OFFSET, can be configured using the KASAN_SHADOW_OFFSET
> option. UML uses roughly 18TB of address space, and KASAN requires 1/8th
> of this. The default location of this offset is 0x100000000000, which
> keeps it out-of-the-way even on UML setups with more "physical" memory.
>
> For low-memory setups, 0x7fff8000 can be used instead, which fits in an
> immediate and is therefore faster, as suggested by Dmitry Vyukov. There
> is usually enough free space at this location; however, it is a config
> option so that it can be easily changed if needed.
>
> Note that, unlike KASAN on other architectures, vmalloc allocations
> still use the shadow memory allocated upfront, rather than allocating
> and free-ing it per-vmalloc allocation.
>
> Signed-off-by: Patricia Alfonso <trishalfonso@google.com>
> Co-developed-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> Signed-off-by: David Gow <davidgow@google.com>
> ---
>
> This is a new RFC for the KASAN/UML port, based on the patch v1:
> https://lore.kernel.org/all/20200226004608.8128-1-trishalfonso@google.com/
>
> With several fixes by Vincent Whitchurch:
> https://lore.kernel.org/all/20220525111756.GA15955@axis.com/
>
> That thread describes the differences from the v1 (and hence the
> previous RFCs better than I can here), but the gist of it is:
> - Support for KASAN_VMALLOC, by changing the way
>   kasan_{populate,release}_vmalloc work to update existing shadow
>   memory, rather than allocating anything new.
> - A similar fix for modules' shadow memory.
> - Support for KASAN_STACK
>   - This requires the bugfix here:
> https://lore.kernel.org/lkml/20220523140403.2361040-1-vincent.whitchurch@axis.com/
>   - Plus a couple of files excluded from KASAN.
> - Revert the default shadow offset to 0x100000000000
>   - This was breaking when mem=1G for me, at least.
> - A few minor fixes to linker sections and scripts.
>   - I've added one to dyn.lds.S on top of the ones Vincent added.

Excited to see this revived!

> There are still a few things to be sorted out before this is ready to go
> upstream, in particular:
> - We've got a bunch of checks for CONFIG_UML, where a more specific
>   config option might be better. For example: CONFIG_KASAN_NO_SHADOW_ALLOC.

Probably. But with 1 arch setting it, I am fine either way.

> - Alternatively, the vmalloc (and module) shadow memory allocators could
>   support per-architecture replacements.

Humm... again hard to say while we have only 1 arch doing this.
Another option: leave a comment on the first CONFIG_UML check listing
these alternatives. When another arch needs something similar, then we
can switch to one of these options.

> - Do we want to the alignment before or after the __memset() in
>   kasan_populate_vmalloc()?

I think you did it correctly (alignment after).
8 normal pages map to 1 shadow page. For the purposes of mapping pages
lazily on other arches, we want to over-map. But for the memset, we
want to clear only the shadow that relates to the current region.


> - This doesn't seem to work when CONFIG_STATIC_LINK is enabled (because
>   libc crt0 code calls memory functions, which expect the shadow memory
>   to already exist, due to multiple symbols being resolved.
>   - I think we should just make this depend on dynamic UML.
>   - For that matter, I think static UML is actually broken at the
>     moment. I'll send a patch out tomorrow.

I don't know how important the static build is for UML.
Generally I prefer to build things statically b/c e.g. if a testing
system builds on one machine but runs tests on another, dynamic link
may be a problem. Or, say, if a testing system provides binary
artifacts, and then nobody can run it locally.

One potential way to fix it is to require outline KASAN
instrumentation for static build and then make kasan_arch_is_ready()
return false until the shadow is mapped. I see kasan_arch_is_ready()
is checked at the beginning of all KASAN runtime entry points.
But it would be nice if the dynamic build also supports inline and
does not add kasan_arch_is_ready() check overhead.

> - And there's a checkpatch complaint about a long __memset() line.
>
> Thanks again to everyone who's contributed and looked at these patches!
> Note that I removed the Reviewed-by tags, as I think this version has
> enough changes to warrant a re-review.
>
> -- David
>
> ---
>  arch/um/Kconfig                  | 15 +++++++++++++++
>  arch/um/Makefile                 |  6 ++++++
>  arch/um/include/asm/common.lds.S |  2 ++
>  arch/um/kernel/Makefile          |  3 +++
>  arch/um/kernel/dyn.lds.S         |  6 +++++-
>  arch/um/kernel/mem.c             | 18 ++++++++++++++++++
>  arch/um/os-Linux/mem.c           | 22 ++++++++++++++++++++++
>  arch/um/os-Linux/user_syms.c     |  4 ++--
>  arch/x86/um/Makefile             |  3 ++-
>  arch/x86/um/vdso/Makefile        |  3 +++
>  mm/kasan/shadow.c                | 20 +++++++++++++++++++-
>  11 files changed, 97 insertions(+), 5 deletions(-)
>
> diff --git a/arch/um/Kconfig b/arch/um/Kconfig
> index 4d398b80aea8..c28ea5c89381 100644
> --- a/arch/um/Kconfig
> +++ b/arch/um/Kconfig
> @@ -11,6 +11,8 @@ config UML
>         select ARCH_HAS_STRNLEN_USER
>         select ARCH_NO_PREEMPT
>         select HAVE_ARCH_AUDITSYSCALL
> +       select HAVE_ARCH_KASAN if X86_64
> +       select HAVE_ARCH_KASAN_VMALLOC if HAVE_ARCH_KASAN
>         select HAVE_ARCH_SECCOMP_FILTER
>         select HAVE_ASM_MODVERSIONS
>         select HAVE_UID16
> @@ -219,6 +221,19 @@ config UML_TIME_TRAVEL_SUPPORT
>
>           It is safe to say Y, but you probably don't need this.
>
> +config KASAN_SHADOW_OFFSET
> +       hex
> +       depends on KASAN
> +       default 0x100000000000
> +       help
> +         This is the offset at which the ~2.25TB of shadow memory is
> +         mapped and used by KASAN for memory debugging. This can be any
> +         address that has at least KASAN_SHADOW_SIZE(total address space divided
> +         by 8) amount of space so that the KASAN shadow memory does not conflict
> +         with anything. The default is 0x100000000000, which works even if mem is
> +         set to a large value. On low-memory systems, try 0x7fff8000, as it fits
> +         into the immediate of most instructions, improving performance.
> +
>  endmenu
>
>  source "arch/um/drivers/Kconfig"
> diff --git a/arch/um/Makefile b/arch/um/Makefile
> index f2fe63bfd819..a98405f4ecb8 100644
> --- a/arch/um/Makefile
> +++ b/arch/um/Makefile
> @@ -75,6 +75,12 @@ USER_CFLAGS = $(patsubst $(KERNEL_DEFINES),,$(patsubst -I%,,$(KBUILD_CFLAGS))) \
>                 -D_FILE_OFFSET_BITS=64 -idirafter $(srctree)/include \
>                 -idirafter $(objtree)/include -D__KERNEL__ -D__UM_HOST__
>
> +# Kernel config options are not included in USER_CFLAGS, but the option for KASAN
> +# should be included if the KASAN config option was set.
> +ifdef CONFIG_KASAN
> +       USER_CFLAGS+=-DCONFIG_KASAN=y
> +endif
> +
>  #This will adjust *FLAGS accordingly to the platform.
>  include $(srctree)/$(ARCH_DIR)/Makefile-os-$(OS)
>
> diff --git a/arch/um/include/asm/common.lds.S b/arch/um/include/asm/common.lds.S
> index eca6c452a41b..fd481ac371de 100644
> --- a/arch/um/include/asm/common.lds.S
> +++ b/arch/um/include/asm/common.lds.S
> @@ -83,6 +83,8 @@
>    }
>    .init_array : {
>         __init_array_start = .;
> +       *(.kasan_init)
> +       *(.init_array.*)
>         *(.init_array)
>         __init_array_end = .;
>    }
> diff --git a/arch/um/kernel/Makefile b/arch/um/kernel/Makefile
> index 1c2d4b29a3d4..a089217e2f0e 100644
> --- a/arch/um/kernel/Makefile
> +++ b/arch/um/kernel/Makefile
> @@ -27,6 +27,9 @@ obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
>  obj-$(CONFIG_STACKTRACE) += stacktrace.o
>  obj-$(CONFIG_GENERIC_PCI_IOMAP) += ioport.o
>
> +KASAN_SANITIZE_stacktrace.o := n
> +KASAN_SANITIZE_sysrq.o := n
> +
>  USER_OBJS := config.o
>
>  include arch/um/scripts/Makefile.rules
> diff --git a/arch/um/kernel/dyn.lds.S b/arch/um/kernel/dyn.lds.S
> index 2f2a8ce92f1e..2b7fc5b54164 100644
> --- a/arch/um/kernel/dyn.lds.S
> +++ b/arch/um/kernel/dyn.lds.S
> @@ -109,7 +109,11 @@ SECTIONS
>       be empty, which isn't pretty.  */
>    . = ALIGN(32 / 8);
>    .preinit_array     : { *(.preinit_array) }
> -  .init_array     : { *(.init_array) }
> +  .init_array     : {
> +    *(.kasan_init)
> +    *(.init_array.*)
> +    *(.init_array)
> +  }
>    .fini_array     : { *(.fini_array) }
>    .data           : {
>      INIT_TASK_DATA(KERNEL_STACK_SIZE)
> diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c
> index 15295c3237a0..a32cfce53efb 100644
> --- a/arch/um/kernel/mem.c
> +++ b/arch/um/kernel/mem.c
> @@ -18,6 +18,24 @@
>  #include <kern_util.h>
>  #include <mem_user.h>
>  #include <os.h>
> +#include <linux/sched/task.h>
> +
> +#ifdef CONFIG_KASAN
> +void kasan_init(void)
> +{
> +       /*
> +        * kasan_map_memory will map all of the required address space and
> +        * the host machine will allocate physical memory as necessary.
> +        */
> +       kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE);
> +       init_task.kasan_depth = 0;
> +       os_info("KernelAddressSanitizer initialized\n");
> +}
> +
> +static void (*kasan_init_ptr)(void)
> +__section(".kasan_init") __used
> += kasan_init;
> +#endif
>
>  /* allocated in paging_init, zeroed in mem_init, and unchanged thereafter */
>  unsigned long *empty_zero_page = NULL;
> diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c
> index 3c1b77474d2d..8530b2e08604 100644
> --- a/arch/um/os-Linux/mem.c
> +++ b/arch/um/os-Linux/mem.c
> @@ -17,6 +17,28 @@
>  #include <init.h>
>  #include <os.h>
>
> +/*
> + * kasan_map_memory - maps memory from @start with a size of @len.
> + * The allocated memory is filled with zeroes upon success.
> + * @start: the start address of the memory to be mapped
> + * @len: the length of the memory to be mapped
> + *
> + * This function is used to map shadow memory for KASAN in uml
> + */
> +void kasan_map_memory(void *start, size_t len)
> +{
> +       if (mmap(start,
> +                len,
> +                PROT_READ|PROT_WRITE,
> +                MAP_FIXED|MAP_ANONYMOUS|MAP_PRIVATE|MAP_NORESERVE,
> +                -1,
> +                0) == MAP_FAILED) {
> +               os_info("Couldn't allocate shadow memory: %s\n.",
> +                       strerror(errno));
> +               exit(1);
> +       }
> +}
> +
>  /* Set by make_tempfile() during early boot. */
>  static char *tempdir = NULL;
>
> diff --git a/arch/um/os-Linux/user_syms.c b/arch/um/os-Linux/user_syms.c
> index 715594fe5719..cb667c9225ab 100644
> --- a/arch/um/os-Linux/user_syms.c
> +++ b/arch/um/os-Linux/user_syms.c
> @@ -27,10 +27,10 @@ EXPORT_SYMBOL(strstr);
>  #ifndef __x86_64__
>  extern void *memcpy(void *, const void *, size_t);
>  EXPORT_SYMBOL(memcpy);
> -#endif
> -
>  EXPORT_SYMBOL(memmove);
>  EXPORT_SYMBOL(memset);
> +#endif
> +
>  EXPORT_SYMBOL(printf);
>
>  /* Here, instead, I can provide a fake prototype. Yes, someone cares: genksyms.
> diff --git a/arch/x86/um/Makefile b/arch/x86/um/Makefile
> index ba5789c35809..f778e37494ba 100644
> --- a/arch/x86/um/Makefile
> +++ b/arch/x86/um/Makefile
> @@ -28,7 +28,8 @@ else
>
>  obj-y += syscalls_64.o vdso/
>
> -subarch-y = ../lib/csum-partial_64.o ../lib/memcpy_64.o ../entry/thunk_64.o
> +subarch-y = ../lib/csum-partial_64.o ../lib/memcpy_64.o ../entry/thunk_64.o \
> +       ../lib/memmove_64.o ../lib/memset_64.o
>
>  endif
>
> diff --git a/arch/x86/um/vdso/Makefile b/arch/x86/um/vdso/Makefile
> index 5943387e3f35..8c0396fd0e6f 100644
> --- a/arch/x86/um/vdso/Makefile
> +++ b/arch/x86/um/vdso/Makefile
> @@ -3,6 +3,9 @@
>  # Building vDSO images for x86.
>  #
>
> +# do not instrument on vdso because KASAN is not compatible with user mode
> +KASAN_SANITIZE                 := n
> +
>  # Prevents link failures: __sanitizer_cov_trace_pc() is not linked in.
>  KCOV_INSTRUMENT                := n
>
> diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c
> index a4f07de21771..d8c518bd0e7d 100644
> --- a/mm/kasan/shadow.c
> +++ b/mm/kasan/shadow.c
> @@ -295,8 +295,14 @@ int kasan_populate_vmalloc(unsigned long addr, unsigned long size)
>                 return 0;
>
>         shadow_start = (unsigned long)kasan_mem_to_shadow((void *)addr);
> -       shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE);
>         shadow_end = (unsigned long)kasan_mem_to_shadow((void *)addr + size);
> +
> +       if (IS_ENABLED(CONFIG_UML)) {
> +               __memset(kasan_mem_to_shadow((void *)addr), KASAN_VMALLOC_INVALID, shadow_end - shadow_start);

"kasan_mem_to_shadow((void *)addr)" can be replaced with shadow_start.


> +               return 0;
> +       }
> +
> +       shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE);
>         shadow_end = ALIGN(shadow_end, PAGE_SIZE);

There is no new fancy PAGE_ALIGN macro for this. And I've seen people
sending clean up patches with replacements.
But unfortunately no PAGE_ALIGN_DOWN :(



>
>         ret = apply_to_page_range(&init_mm, shadow_start,
> @@ -466,6 +472,10 @@ void kasan_release_vmalloc(unsigned long start, unsigned long end,
>
>         if (shadow_end > shadow_start) {
>                 size = shadow_end - shadow_start;
> +               if (IS_ENABLED(CONFIG_UML)) {
> +                       __memset(shadow_start, KASAN_SHADOW_INIT, shadow_end - shadow_start);
> +                       return;
> +               }
>                 apply_to_existing_page_range(&init_mm,
>                                              (unsigned long)shadow_start,
>                                              size, kasan_depopulate_vmalloc_pte,
> @@ -531,6 +541,11 @@ int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask)
>         if (WARN_ON(!PAGE_ALIGNED(shadow_start)))
>                 return -EINVAL;
>
> +       if (IS_ENABLED(CONFIG_UML)) {
> +               __memset((void *)shadow_start, KASAN_SHADOW_INIT, shadow_size);
> +               return 0;
> +       }
> +
>         ret = __vmalloc_node_range(shadow_size, 1, shadow_start,
>                         shadow_start + shadow_size,
>                         GFP_KERNEL,
> @@ -554,6 +569,9 @@ int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask)
>
>  void kasan_free_module_shadow(const struct vm_struct *vm)
>  {
> +       if (IS_ENABLED(CONFIG_UML))
> +               return;
> +
>         if (vm->flags & VM_KASAN)
>                 vfree(kasan_mem_to_shadow(vm->addr));
>  }
> --
> 2.36.1.124.g0e6072fb45-goog
>

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC PATCH v3] UML: add support for KASAN under x86_64
  2022-05-27  5:31                 ` Dmitry Vyukov
@ 2022-05-27  7:32                   ` Johannes Berg
  0 siblings, 0 replies; 38+ messages in thread
From: Johannes Berg @ 2022-05-27  7:32 UTC (permalink / raw)
  To: Dmitry Vyukov, David Gow
  Cc: Vincent Whitchurch, Patricia Alfonso, Jeff Dike,
	Richard Weinberger, anton.ivanov, Brendan Higgins, kasan-dev,
	linux-um, LKML, Daniel Latypov

On Fri, 2022-05-27 at 07:31 +0200, Dmitry Vyukov wrote:
> > - This doesn't seem to work when CONFIG_STATIC_LINK is enabled (because
> >   libc crt0 code calls memory functions, which expect the shadow memory
> >   to already exist, due to multiple symbols being resolved.
> >   - I think we should just make this depend on dynamic UML.
> >   - For that matter, I think static UML is actually broken at the
> >     moment. I'll send a patch out tomorrow.
> 
> I don't know how important the static build is for UML.

Depends who you ask, I guess.

IMHO just making KASAN depend on !STATIC_LINK is fine, until somebody
actually wants to do what you describe:

> Generally I prefer to build things statically b/c e.g. if a testing
> system builds on one machine but runs tests on another, dynamic link
> may be a problem. Or, say, if a testing system provides binary
> artifacts, and then nobody can run it locally.
> 
> One potential way to fix it is to require outline KASAN
> instrumentation for static build and then make kasan_arch_is_ready()
> return false until the shadow is mapped. I see kasan_arch_is_ready()
> is checked at the beginning of all KASAN runtime entry points.
> But it would be nice if the dynamic build also supports inline and
> does not add kasan_arch_is_ready() check overhead.

which sounds fine too, but ... trade-offs.

> > +       if (IS_ENABLED(CONFIG_UML)) {
> > +               __memset(kasan_mem_to_shadow((void *)addr), KASAN_VMALLOC_INVALID, shadow_end - shadow_start);
> 
> "kasan_mem_to_shadow((void *)addr)" can be replaced with shadow_start.

and then the memset line isn't so long anymore :)

> 
> 
> > +               return 0;
> > +       }
> > +
> > +       shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE);
> >         shadow_end = ALIGN(shadow_end, PAGE_SIZE);
> 
> There is no new fancy PAGE_ALIGN macro for this. And I've seen people

s/no/now the/ I guess, but it's also existing code.

johannes

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC PATCH v3] UML: add support for KASAN under x86_64
  2022-05-26  1:01               ` [RFC PATCH v3] " David Gow
  2022-05-26  9:29                 ` Johannes Berg
  2022-05-27  5:31                 ` Dmitry Vyukov
@ 2022-05-27 10:36                 ` Johannes Berg
  2022-05-27 13:05                 ` Johannes Berg
  3 siblings, 0 replies; 38+ messages in thread
From: Johannes Berg @ 2022-05-27 10:36 UTC (permalink / raw)
  To: David Gow, Vincent Whitchurch, Patricia Alfonso, Jeff Dike,
	Richard Weinberger, anton.ivanov, Dmitry Vyukov, Brendan Higgins
  Cc: kasan-dev, linux-um, LKML, Daniel Latypov

On Wed, 2022-05-25 at 18:01 -0700, David Gow wrote:
> 
> ---
>  arch/um/Kconfig                  | 15 +++++++++++++++
>  arch/um/Makefile                 |  6 ++++++
>  arch/um/include/asm/common.lds.S |  2 ++
>  arch/um/kernel/Makefile          |  3 +++
>  arch/um/kernel/dyn.lds.S         |  6 +++++-
>  arch/um/kernel/mem.c             | 18 ++++++++++++++++++
>  arch/um/os-Linux/mem.c           | 22 ++++++++++++++++++++++
>  arch/um/os-Linux/user_syms.c     |  4 ++--
>  arch/x86/um/Makefile             |  3 ++-
>  arch/x86/um/vdso/Makefile        |  3 +++
>  mm/kasan/shadow.c                | 20 +++++++++++++++++++-
> 

Btw, it looks like you also forgot to git add the (new) file
arch/um/include/asm/kasan.h from Patricia's patch?

johannes

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC PATCH v3] UML: add support for KASAN under x86_64
  2022-05-26  1:01               ` [RFC PATCH v3] " David Gow
                                   ` (2 preceding siblings ...)
  2022-05-27 10:36                 ` Johannes Berg
@ 2022-05-27 13:05                 ` Johannes Berg
  2022-05-27 13:09                   ` Dmitry Vyukov
  3 siblings, 1 reply; 38+ messages in thread
From: Johannes Berg @ 2022-05-27 13:05 UTC (permalink / raw)
  To: David Gow, Vincent Whitchurch, Patricia Alfonso, Jeff Dike,
	Richard Weinberger, anton.ivanov, Dmitry Vyukov, Brendan Higgins
  Cc: kasan-dev, linux-um, LKML, Daniel Latypov

On Wed, 2022-05-25 at 18:01 -0700, David Gow wrote:
> From: Patricia Alfonso <trishalfonso@google.com>
> 
> Make KASAN run on User Mode Linux on x86_64.

FWIW, I just added this to my virtual lab which I use as CI tests, and
it immediately found a use-after-free bug in mac80211!

I did note (this is more for kasan-dev@) that the "freed by" is fairly
much useless when using kfree_rcu(), it might be worthwhile to annotate
that somehow, so the stack trace is recorded by kfree_rcu() already,
rather than just showing the RCU callback used for that.

johannes

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC PATCH v3] UML: add support for KASAN under x86_64
  2022-05-27 13:05                 ` Johannes Berg
@ 2022-05-27 13:09                   ` Dmitry Vyukov
  2022-05-27 13:15                     ` Johannes Berg
  0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Vyukov @ 2022-05-27 13:09 UTC (permalink / raw)
  To: Johannes Berg
  Cc: David Gow, Vincent Whitchurch, Patricia Alfonso, Jeff Dike,
	Richard Weinberger, anton.ivanov, Brendan Higgins, kasan-dev,
	linux-um, LKML, Daniel Latypov

On Fri, 27 May 2022 at 15:05, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Wed, 2022-05-25 at 18:01 -0700, David Gow wrote:
> > From: Patricia Alfonso <trishalfonso@google.com>
> >
> > Make KASAN run on User Mode Linux on x86_64.
>
> FWIW, I just added this to my virtual lab which I use as CI tests, and
> it immediately found a use-after-free bug in mac80211!
>
> I did note (this is more for kasan-dev@) that the "freed by" is fairly
> much useless when using kfree_rcu(), it might be worthwhile to annotate
> that somehow, so the stack trace is recorded by kfree_rcu() already,
> rather than just showing the RCU callback used for that.

KASAN is doing it for several years now, see e.g.:
https://groups.google.com/g/syzkaller-bugs/c/eTW9zom4O2o/m/_v7cOo2RFwAJ

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC PATCH v3] UML: add support for KASAN under x86_64
  2022-05-27 13:09                   ` Dmitry Vyukov
@ 2022-05-27 13:15                     ` Johannes Berg
  2022-05-27 13:18                       ` Dmitry Vyukov
  0 siblings, 1 reply; 38+ messages in thread
From: Johannes Berg @ 2022-05-27 13:15 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: David Gow, Vincent Whitchurch, Patricia Alfonso, Jeff Dike,
	Richard Weinberger, anton.ivanov, Brendan Higgins, kasan-dev,
	linux-um, LKML, Daniel Latypov

On Fri, 2022-05-27 at 15:09 +0200, Dmitry Vyukov wrote:
> > I did note (this is more for kasan-dev@) that the "freed by" is fairly
> > much useless when using kfree_rcu(), it might be worthwhile to annotate
> > that somehow, so the stack trace is recorded by kfree_rcu() already,
> > rather than just showing the RCU callback used for that.
> 
> KASAN is doing it for several years now, see e.g.:
> https://groups.google.com/g/syzkaller-bugs/c/eTW9zom4O2o/m/_v7cOo2RFwAJ
> 

Hm. It didn't for me:

> BUG: KASAN: use-after-free in ieee80211_vif_use_reserved_context+0x32d/0x40f [mac80211]
> Read of size 4 at addr 0000000065c73340 by task kworker/u2:1/17

Yes.

> CPU: 0 PID: 17 Comm: kworker/u2:1 Tainted: G           O      5.18.0-rc1 #5
> Workqueue: phy0 ieee80211_chswitch_work [mac80211]
> Stack:
>  60ba783f 00000000 10000c268f4e 60ba783f
>  60e60847 70dc9928 719f6e99 00000000
>  71883b20 60bb0b42 60bb0b19 65c73340
> Call Trace:
>  [<600447ea>] show_stack+0x13e/0x14d
>  [<60bb0b42>] dump_stack_lvl+0x29/0x2e
>  [<602ef7c0>] print_report+0x15d/0x60b
>  [<602efdc0>] kasan_report+0x98/0xbd
>  [<602f0cc2>] __asan_report_load4_noabort+0x1b/0x1d
>  [<719f6e99>] ieee80211_vif_use_reserved_context+0x32d/0x40f [mac80211]

This is the user, it just got freed during a function call a few lines
up.

> Allocated by task 16:
>  save_stack_trace+0x2e/0x30
>  stack_trace_save+0x81/0x9b
>  kasan_save_stack+0x2d/0x54
>  kasan_set_track+0x34/0x3e
>  ____kasan_kmalloc+0x8d/0x99
>  __kasan_kmalloc+0x10/0x12
>  __kmalloc+0x1f6/0x20b
>  ieee80211_alloc_chanctx+0xdc/0x35f [mac80211]

This makes sense too.

> Freed by task 8:
>  save_stack_trace+0x2e/0x30
>  stack_trace_save+0x81/0x9b
>  kasan_save_stack+0x2d/0x54
>  kasan_set_track+0x34/0x3e
>  kasan_set_free_info+0x33/0x44
>  ____kasan_slab_free+0x12b/0x149
>  __kasan_slab_free+0x19/0x1b
>  slab_free_freelist_hook+0x10b/0x16a
>  kfree+0x10d/0x1fa
>  kvfree+0x38/0x3a
>  rcu_process_callbacks+0x2c5/0x349

This is the RCU callback.

> Last potentially related work creation:
>  save_stack_trace+0x2e/0x30
>  stack_trace_save+0x81/0x9b
>  kasan_save_stack+0x2d/0x54
>  __kasan_record_aux_stack+0xd5/0xe2
>  kasan_record_aux_stack_noalloc+0x12/0x14
>  insert_work+0x50/0xd7
>  __queue_work+0x805/0x95c
>  queue_work_on+0xba/0x131
>  call_usermodehelper_exec+0x242/0x361
>  kobject_uevent_env+0xe46/0xeaf
>  kobject_uevent+0x12/0x14
>  driver_register+0x37e/0x38d
>  pcie_port_service_register+0x19d/0x1a5

This stuff is completely unrelated.

> The buggy address belongs to the object at 0000000065c73300
>  which belongs to the cache kmalloc-192 of size 192
> The buggy address is located 64 bytes inside of
>  192-byte region [0000000065c73300, 0000000065c733c0)
> 

and that's it?

johannes

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC PATCH v3] UML: add support for KASAN under x86_64
  2022-05-27 13:15                     ` Johannes Berg
@ 2022-05-27 13:18                       ` Dmitry Vyukov
  2022-05-27 13:27                         ` Johannes Berg
  0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Vyukov @ 2022-05-27 13:18 UTC (permalink / raw)
  To: Johannes Berg
  Cc: David Gow, Vincent Whitchurch, Patricia Alfonso, Jeff Dike,
	Richard Weinberger, anton.ivanov, Brendan Higgins, kasan-dev,
	linux-um, LKML, Daniel Latypov

On Fri, 27 May 2022 at 15:15, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Fri, 2022-05-27 at 15:09 +0200, Dmitry Vyukov wrote:
> > > I did note (this is more for kasan-dev@) that the "freed by" is fairly
> > > much useless when using kfree_rcu(), it might be worthwhile to annotate
> > > that somehow, so the stack trace is recorded by kfree_rcu() already,
> > > rather than just showing the RCU callback used for that.
> >
> > KASAN is doing it for several years now, see e.g.:
> > https://groups.google.com/g/syzkaller-bugs/c/eTW9zom4O2o/m/_v7cOo2RFwAJ
> >
>
> Hm. It didn't for me:

Please post a full report with line numbers and kernel version.

> > BUG: KASAN: use-after-free in ieee80211_vif_use_reserved_context+0x32d/0x40f [mac80211]
> > Read of size 4 at addr 0000000065c73340 by task kworker/u2:1/17
>
> Yes.
>
> > CPU: 0 PID: 17 Comm: kworker/u2:1 Tainted: G           O      5.18.0-rc1 #5
> > Workqueue: phy0 ieee80211_chswitch_work [mac80211]
> > Stack:
> >  60ba783f 00000000 10000c268f4e 60ba783f
> >  60e60847 70dc9928 719f6e99 00000000
> >  71883b20 60bb0b42 60bb0b19 65c73340
> > Call Trace:
> >  [<600447ea>] show_stack+0x13e/0x14d
> >  [<60bb0b42>] dump_stack_lvl+0x29/0x2e
> >  [<602ef7c0>] print_report+0x15d/0x60b
> >  [<602efdc0>] kasan_report+0x98/0xbd
> >  [<602f0cc2>] __asan_report_load4_noabort+0x1b/0x1d
> >  [<719f6e99>] ieee80211_vif_use_reserved_context+0x32d/0x40f [mac80211]
>
> This is the user, it just got freed during a function call a few lines
> up.
>
> > Allocated by task 16:
> >  save_stack_trace+0x2e/0x30
> >  stack_trace_save+0x81/0x9b
> >  kasan_save_stack+0x2d/0x54
> >  kasan_set_track+0x34/0x3e
> >  ____kasan_kmalloc+0x8d/0x99
> >  __kasan_kmalloc+0x10/0x12
> >  __kmalloc+0x1f6/0x20b
> >  ieee80211_alloc_chanctx+0xdc/0x35f [mac80211]
>
> This makes sense too.
>
> > Freed by task 8:
> >  save_stack_trace+0x2e/0x30
> >  stack_trace_save+0x81/0x9b
> >  kasan_save_stack+0x2d/0x54
> >  kasan_set_track+0x34/0x3e
> >  kasan_set_free_info+0x33/0x44
> >  ____kasan_slab_free+0x12b/0x149
> >  __kasan_slab_free+0x19/0x1b
> >  slab_free_freelist_hook+0x10b/0x16a
> >  kfree+0x10d/0x1fa
> >  kvfree+0x38/0x3a
> >  rcu_process_callbacks+0x2c5/0x349
>
> This is the RCU callback.
>
> > Last potentially related work creation:
> >  save_stack_trace+0x2e/0x30
> >  stack_trace_save+0x81/0x9b
> >  kasan_save_stack+0x2d/0x54
> >  __kasan_record_aux_stack+0xd5/0xe2
> >  kasan_record_aux_stack_noalloc+0x12/0x14
> >  insert_work+0x50/0xd7
> >  __queue_work+0x805/0x95c
> >  queue_work_on+0xba/0x131
> >  call_usermodehelper_exec+0x242/0x361
> >  kobject_uevent_env+0xe46/0xeaf
> >  kobject_uevent+0x12/0x14
> >  driver_register+0x37e/0x38d
> >  pcie_port_service_register+0x19d/0x1a5
>
> This stuff is completely unrelated.
>
> > The buggy address belongs to the object at 0000000065c73300
> >  which belongs to the cache kmalloc-192 of size 192
> > The buggy address is located 64 bytes inside of
> >  192-byte region [0000000065c73300, 0000000065c733c0)
> >
>
> and that's it?
>
> johannes
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/6fa1ebe49b8d574fb1c82aefeeb54439d9c98750.camel%40sipsolutions.net.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC PATCH v3] UML: add support for KASAN under x86_64
  2022-05-27 13:18                       ` Dmitry Vyukov
@ 2022-05-27 13:27                         ` Johannes Berg
  2022-05-27 13:52                           ` Dmitry Vyukov
  0 siblings, 1 reply; 38+ messages in thread
From: Johannes Berg @ 2022-05-27 13:27 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: David Gow, Vincent Whitchurch, Patricia Alfonso, Jeff Dike,
	Richard Weinberger, anton.ivanov, Brendan Higgins, kasan-dev,
	linux-um, LKML, Daniel Latypov

On Fri, 2022-05-27 at 15:18 +0200, Dmitry Vyukov wrote:
> On Fri, 27 May 2022 at 15:15, Johannes Berg <johannes@sipsolutions.net> wrote:
> > 
> > On Fri, 2022-05-27 at 15:09 +0200, Dmitry Vyukov wrote:
> > > > I did note (this is more for kasan-dev@) that the "freed by" is fairly
> > > > much useless when using kfree_rcu(), it might be worthwhile to annotate
> > > > that somehow, so the stack trace is recorded by kfree_rcu() already,
> > > > rather than just showing the RCU callback used for that.
> > > 
> > > KASAN is doing it for several years now, see e.g.:
> > > https://groups.google.com/g/syzkaller-bugs/c/eTW9zom4O2o/m/_v7cOo2RFwAJ
> > > 
> > 
> > Hm. It didn't for me:
> 
> Please post a full report with line numbers and kernel version.

That was basically it, apart from a few lines snipped from the stack
traces. Kernel version was admittedly a little older - 5.18.0-rc1 + a
few UML fixes + this KASAN patch (+ the fixes I pointed out earlier)

I guess it doesn't really matter that much, just had to dig a bit to
understand why it was freed.

johannes



^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC PATCH v3] UML: add support for KASAN under x86_64
  2022-05-27 13:27                         ` Johannes Berg
@ 2022-05-27 13:52                           ` Dmitry Vyukov
  2022-05-27 14:27                             ` Johannes Berg
  0 siblings, 1 reply; 38+ messages in thread
From: Dmitry Vyukov @ 2022-05-27 13:52 UTC (permalink / raw)
  To: Johannes Berg
  Cc: David Gow, Vincent Whitchurch, Patricia Alfonso, Jeff Dike,
	Richard Weinberger, anton.ivanov, Brendan Higgins, kasan-dev,
	linux-um, LKML, Daniel Latypov

On Fri, 27 May 2022 at 15:27, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Fri, 2022-05-27 at 15:18 +0200, Dmitry Vyukov wrote:
> > On Fri, 27 May 2022 at 15:15, Johannes Berg <johannes@sipsolutions.net> wrote:
> > >
> > > On Fri, 2022-05-27 at 15:09 +0200, Dmitry Vyukov wrote:
> > > > > I did note (this is more for kasan-dev@) that the "freed by" is fairly
> > > > > much useless when using kfree_rcu(), it might be worthwhile to annotate
> > > > > that somehow, so the stack trace is recorded by kfree_rcu() already,
> > > > > rather than just showing the RCU callback used for that.
> > > >
> > > > KASAN is doing it for several years now, see e.g.:
> > > > https://groups.google.com/g/syzkaller-bugs/c/eTW9zom4O2o/m/_v7cOo2RFwAJ
> > > >
> > >
> > > Hm. It didn't for me:
> >
> > Please post a full report with line numbers and kernel version.
>
> That was basically it, apart from a few lines snipped from the stack
> traces. Kernel version was admittedly a little older - 5.18.0-rc1 + a
> few UML fixes + this KASAN patch (+ the fixes I pointed out earlier)
>
> I guess it doesn't really matter that much, just had to dig a bit to
> understand why it was freed.

Humm... I don't have any explanation based only on this info.
Generally call_rcu stacks are memorized and I see the call is still there:
https://elixir.bootlin.com/linux/v5.18/source/kernel/rcu/tree.c#L3595
It may be caused by some narrow races, depleted reserve memory in
stackdepot, or race with quarantine eviction.

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [RFC PATCH v3] UML: add support for KASAN under x86_64
  2022-05-27 13:52                           ` Dmitry Vyukov
@ 2022-05-27 14:27                             ` Johannes Berg
  2022-05-27 15:46                               ` Dmitry Vyukov
  0 siblings, 1 reply; 38+ messages in thread
From: Johannes Berg @ 2022-05-27 14:27 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: David Gow, Vincent Whitchurch, Patricia Alfonso, Jeff Dike,
	Richard Weinberger, anton.ivanov, Brendan Higgins, kasan-dev,
	linux-um, LKML, Daniel Latypov

On Fri, 2022-05-27 at 15:52 +0200, Dmitry Vyukov wrote:
> On Fri, 27 May 2022 at 15:27, Johannes Berg <johannes@sipsolutions.net> wrote:
> > 
> > On Fri, 2022-05-27 at 15:18 +0200, Dmitry Vyukov wrote:
> > > On Fri, 27 May 2022 at 15:15, Johannes Berg <johannes@sipsolutions.net> wrote:
> > > > 
> > > > On Fri, 2022-05-27 at 15:09 +0200, Dmitry Vyukov wrote:
> > > > > > I did note (this is more for kasan-dev@) that the "freed by" is fairly
> > > > > > much useless when using kfree_rcu(), it might be worthwhile to annotate
> > > > > > that somehow, so the stack trace is recorded by kfree_rcu() already,
> > > > > > rather than just showing the RCU callback used for that.
[...]
> Humm... I don't have any explanation based only on this info.
> Generally call_rcu stacks are memorized and I see the call is still there:
> https://elixir.bootlin.com/linux/v5.18/source/kernel/rcu/tree.c#L3595

Oh, that's simple then, UML is !SMP && !PREEMPT so it gets TINY_RCU
instead of TREE_RCU.

Unfortunately, it's not entirely trivial to fix, something like this,
mostly because of header maze (cannot include kasan.h in rcutiny.h):

diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h
index 5fed476f977f..d84e13f2c384 100644
--- a/include/linux/rcutiny.h
+++ b/include/linux/rcutiny.h
@@ -38,7 +38,7 @@ static inline void synchronize_rcu_expedited(void)
  */
 extern void kvfree(const void *addr);
 
-static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
+static inline void __kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 {
 	if (head) {
 		call_rcu(head, func);
@@ -51,6 +51,15 @@ static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
 	kvfree((void *) func);
 }
 
+#ifdef CONFIG_KASAN_GENERIC
+void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func);
+#else
+static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
+{
+	__kvfree_call_rcu(head, func);
+}
+#endif
+
 void rcu_qs(void);
 
 static inline void rcu_softirq_qs(void)
diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c
index 340b3f8b090d..aa235f0332ba 100644
--- a/kernel/rcu/tiny.c
+++ b/kernel/rcu/tiny.c
@@ -217,6 +217,18 @@ bool poll_state_synchronize_rcu(unsigned long oldstate)
 }
 EXPORT_SYMBOL_GPL(poll_state_synchronize_rcu);
 
+void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
+{
+	if (head) {
+		void *ptr = (void *) head - (unsigned long) func;
+
+		kasan_record_aux_stack_noalloc(ptr);
+	}
+
+	__kvfree_call_rcu(head, func);
+}
+EXPORT_SYMBOL_GPL(kvfree_call_rcu);
+
 void __init rcu_init(void)
 {
 	open_softirq(RCU_SOFTIRQ, rcu_process_callbacks);




Or I guess I could copy/paste

#ifdef CONFIG_KASAN_GENERIC
void kasan_record_aux_stack_noalloc(void *ptr);
#else /* CONFIG_KASAN_GENERIC */
static inline void kasan_record_aux_stack_noalloc(void *ptr) {}
#endif /* CONFIG_KASAN_GENERIC */


into rcutiny.h, that'd be smaller, and export the symbol ...

johannes

^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [RFC PATCH v3] UML: add support for KASAN under x86_64
  2022-05-27 14:27                             ` Johannes Berg
@ 2022-05-27 15:46                               ` Dmitry Vyukov
  0 siblings, 0 replies; 38+ messages in thread
From: Dmitry Vyukov @ 2022-05-27 15:46 UTC (permalink / raw)
  To: Johannes Berg
  Cc: David Gow, Vincent Whitchurch, Patricia Alfonso, Jeff Dike,
	Richard Weinberger, anton.ivanov, Brendan Higgins, kasan-dev,
	linux-um, LKML, Daniel Latypov

On Fri, 27 May 2022 at 16:28, Johannes Berg <johannes@sipsolutions.net> wrote:
> > > > > On Fri, 2022-05-27 at 15:09 +0200, Dmitry Vyukov wrote:
> > > > > > > I did note (this is more for kasan-dev@) that the "freed by" is fairly
> > > > > > > much useless when using kfree_rcu(), it might be worthwhile to annotate
> > > > > > > that somehow, so the stack trace is recorded by kfree_rcu() already,
> > > > > > > rather than just showing the RCU callback used for that.
> [...]
> > Humm... I don't have any explanation based only on this info.
> > Generally call_rcu stacks are memorized and I see the call is still there:
> > https://elixir.bootlin.com/linux/v5.18/source/kernel/rcu/tree.c#L3595
>
> Oh, that's simple then, UML is !SMP && !PREEMPT so it gets TINY_RCU
> instead of TREE_RCU.

Nice!

> Unfortunately, it's not entirely trivial to fix, something like this,
> mostly because of header maze (cannot include kasan.h in rcutiny.h):
>
> diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h
> index 5fed476f977f..d84e13f2c384 100644
> --- a/include/linux/rcutiny.h
> +++ b/include/linux/rcutiny.h
> @@ -38,7 +38,7 @@ static inline void synchronize_rcu_expedited(void)
>   */
>  extern void kvfree(const void *addr);
>
> -static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> +static inline void __kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
>  {
>         if (head) {
>                 call_rcu(head, func);
> @@ -51,6 +51,15 @@ static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
>         kvfree((void *) func);
>  }
>
> +#ifdef CONFIG_KASAN_GENERIC
> +void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func);
> +#else
> +static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> +{
> +       __kvfree_call_rcu(head, func);
> +}
> +#endif
> +
>  void rcu_qs(void);
>
>  static inline void rcu_softirq_qs(void)
> diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c
> index 340b3f8b090d..aa235f0332ba 100644
> --- a/kernel/rcu/tiny.c
> +++ b/kernel/rcu/tiny.c
> @@ -217,6 +217,18 @@ bool poll_state_synchronize_rcu(unsigned long oldstate)
>  }
>  EXPORT_SYMBOL_GPL(poll_state_synchronize_rcu);
>
> +void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func)
> +{
> +       if (head) {
> +               void *ptr = (void *) head - (unsigned long) func;
> +
> +               kasan_record_aux_stack_noalloc(ptr);
> +       }
> +
> +       __kvfree_call_rcu(head, func);
> +}
> +EXPORT_SYMBOL_GPL(kvfree_call_rcu);
> +
>  void __init rcu_init(void)
>  {
>         open_softirq(RCU_SOFTIRQ, rcu_process_callbacks);
>
>
>
>
> Or I guess I could copy/paste
>
> #ifdef CONFIG_KASAN_GENERIC
> void kasan_record_aux_stack_noalloc(void *ptr);
> #else /* CONFIG_KASAN_GENERIC */
> static inline void kasan_record_aux_stack_noalloc(void *ptr) {}
> #endif /* CONFIG_KASAN_GENERIC */
>
>
> into rcutiny.h, that'd be smaller, and export the symbol ...
>
> johannes
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/5eef2f1b43c25447ccca2f50f4964fd77a719b08.camel%40sipsolutions.net.

^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2022-05-27 15:46 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-26  0:46 [PATCH] UML: add support for KASAN under x86_64 Patricia Alfonso
2020-02-26  1:19 ` Brendan Higgins
2020-02-26 15:24 ` Dmitry Vyukov
2020-03-06  0:03 ` Patricia Alfonso
2020-03-11 10:32   ` Johannes Berg
2020-03-11 10:46     ` Dmitry Vyukov
2020-03-11 11:18     ` Johannes Berg
2020-03-11 11:40       ` Johannes Berg
2020-03-11 17:34       ` Dmitry Vyukov
2020-03-20 13:39         ` Johannes Berg
2020-03-20 15:18           ` Dmitry Vyukov
2020-03-30  7:43             ` Johannes Berg
2020-03-30  8:38               ` Dmitry Vyukov
2020-03-30  8:41                 ` Johannes Berg
2020-03-31  6:14                   ` David Gow
2020-03-31  7:43                     ` Johannes Berg
2020-03-31 16:39                   ` Patricia Alfonso
2020-03-31 16:54                     ` Richard Weinberger
2020-03-11 22:32     ` Patricia Alfonso
2020-03-11 22:44       ` Johannes Berg
2022-05-24 10:34         ` Vincent Whitchurch
2022-05-24 10:45           ` Johannes Berg
2022-05-24 19:35           ` David Gow
2022-05-25 11:17             ` Vincent Whitchurch
2022-05-26  1:01               ` [RFC PATCH v3] " David Gow
2022-05-26  9:29                 ` Johannes Berg
2022-05-27  5:31                 ` Dmitry Vyukov
2022-05-27  7:32                   ` Johannes Berg
2022-05-27 10:36                 ` Johannes Berg
2022-05-27 13:05                 ` Johannes Berg
2022-05-27 13:09                   ` Dmitry Vyukov
2022-05-27 13:15                     ` Johannes Berg
2022-05-27 13:18                       ` Dmitry Vyukov
2022-05-27 13:27                         ` Johannes Berg
2022-05-27 13:52                           ` Dmitry Vyukov
2022-05-27 14:27                             ` Johannes Berg
2022-05-27 15:46                               ` Dmitry Vyukov
2020-03-29 19:06     ` [PATCH] " Richard Weinberger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).