2020-02-26 00:46:50

by Patricia Alfonso

[permalink] [raw]
Subject: [PATCH] UML: add support for KASAN under x86_64

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 <[email protected]>
---
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


2020-02-26 01:20:33

by Brendan Higgins

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

On Tue, Feb 25, 2020 at 4:46 PM Patricia Alfonso
<[email protected]> 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 <[email protected]>

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

Reviewed-by: Brendan Higgins <[email protected]>

> ---
> 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
>

2020-02-26 15:26:01

by Dmitry Vyukov

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

On Wed, Feb 26, 2020 at 1:46 AM Patricia Alfonso
<[email protected]> 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 <[email protected]>

Thanks!

> Signed-off-by: Patricia Alfonso <[email protected]>
> ---
> 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
>

2020-03-06 00:04:31

by Patricia Alfonso

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

On Tue, Feb 25, 2020 at 4:46 PM Patricia Alfonso
<[email protected]> 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 <[email protected]>
> ---

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
>

2020-03-11 10:34:13

by Johannes Berg

[permalink] [raw]
Subject: 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?

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

2020-03-11 10:47:41

by Dmitry Vyukov

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

On Wed, Mar 11, 2020 at 11:32 AM Johannes Berg
<[email protected]> 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
>

2020-03-11 11:19:34

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

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

2020-03-11 11:41:43

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64


> 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

2020-03-11 17:37:11

by Dmitry Vyukov

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

On Wed, Mar 11, 2020 at 12:19 PM Johannes Berg
<[email protected]> 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
>

2020-03-11 22:34:24

by Patricia Alfonso

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

On Wed, Mar 11, 2020 at 3:32 AM Johannes Berg <[email protected]> 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

2020-03-11 22:45:52

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

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

2020-03-20 13:40:51

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

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

2020-03-20 15:19:45

by Dmitry Vyukov

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

On Fri, Mar 20, 2020 at 2:39 PM Johannes Berg <[email protected]> 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...

2020-03-29 20:08:02

by Richard Weinberger

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

----- Ursprüngliche Mail -----
> Von: "Johannes Berg" <[email protected]>
> An: "Patricia Alfonso" <[email protected]>, "Jeff Dike" <[email protected]>, "richard" <[email protected]>, "anton
> ivanov" <[email protected]>, "Andrey Ryabinin" <[email protected]>, "Dmitry Vyukov"
> <[email protected]>, "Brendan Higgins" <[email protected]>, "davidgow" <[email protected]>
> CC: "kasan-dev" <[email protected]>, "linux-kernel" <[email protected]>, "linux-um"
> <[email protected]>
> 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

2020-03-30 07:45:50

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

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


2020-03-30 08:39:45

by Dmitry Vyukov

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

On Mon, Mar 30, 2020 at 9:44 AM Johannes Berg <[email protected]> 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.

2020-03-30 08:42:33

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

On Mon, 2020-03-30 at 10:38 +0200, Dmitry Vyukov wrote:
> On Mon, Mar 30, 2020 at 9:44 AM Johannes Berg <[email protected]> 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

2020-03-31 06:16:22

by David Gow

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

On Mon, Mar 30, 2020 at 1:41 AM Johannes Berg <[email protected]> wrote:
>
> On Mon, 2020-03-30 at 10:38 +0200, Dmitry Vyukov wrote:
> > On Mon, Mar 30, 2020 at 9:44 AM Johannes Berg <[email protected]> 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


Attachments:
smime.p7s (3.76 kB)
S/MIME Cryptographic Signature

2020-03-31 07:45:40

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

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

2020-03-31 16:40:47

by Patricia Alfonso

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

On Mon, Mar 30, 2020 at 1:41 AM Johannes Berg <[email protected]> wrote:
>
> On Mon, 2020-03-30 at 10:38 +0200, Dmitry Vyukov wrote:
> > On Mon, Mar 30, 2020 at 9:44 AM Johannes Berg <[email protected]> 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

2020-03-31 16:55:01

by Richard Weinberger

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

Patricia,

----- Ursprüngliche Mail -----
> Von: "Patricia Alfonso" <[email protected]>
> An: "Johannes Berg" <[email protected]>
> CC: "Dmitry Vyukov" <[email protected]>, "Jeff Dike" <[email protected]>, "richard" <[email protected]>, "anton ivanov"
> <[email protected]>, "Andrey Ryabinin" <[email protected]>, "Brendan Higgins"
> <[email protected]>, "davidgow" <[email protected]>, "linux-um" <[email protected]>,
> "linux-kernel" <[email protected]>, "kasan-dev" <[email protected]>
> 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 <[email protected]> wrote:
>>
>> On Mon, 2020-03-30 at 10:38 +0200, Dmitry Vyukov wrote:
>> > On Mon, Mar 30, 2020 at 9:44 AM Johannes Berg <[email protected]> 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

2022-05-24 20:53:55

by Vincent Whitchurch

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

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/[email protected]/

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));
}

2022-05-25 12:44:21

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

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/[email protected]/

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

2022-05-25 13:04:46

by David Gow

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

+dja in case he has any KASAN_VMALLOC thoughts.

On Tue, May 24, 2022 at 3:34 AM Vincent Whitchurch
<[email protected]> 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/[email protected]/
>

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

2022-05-28 18:34:02

by Vincent Whitchurch

[permalink] [raw]
Subject: Re: [PATCH] UML: add support for KASAN under x86_64

On Tue, May 24, 2022 at 09:35:33PM +0200, David Gow wrote:
> On Tue, May 24, 2022 at 3:34 AM Vincent Whitchurch
> <[email protected]> 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!