[RFC PATCH] UML: add support for KASAN under x86_64

[Patricia Alfonso]

Make KASAN run on User Mode Linux on x86_64.

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. For this reason, the default
location is 0x100000000000. There is usually enough free space at
this location; however, it is a config option so that it can be
easily changed if needed.

Functions that are used before KASAN is initialized are excluded from
instrumentation. The UML-specific KASAN initializer uses mmap to map
the roughly 2.25TB of shadow memory to the location defined by
KASAN_SHADOW_OFFSET and ensures that the address space used by the
kernel text and the vmalloc region is not poisoned at
initialization.

Signed-off-by: Patricia Alfonso <trisha...@google.com>
---
arch/um/Kconfig | 10 ++++++++++
arch/um/include/asm/dma.h | 1 +
arch/um/include/asm/kasan.h | 32 ++++++++++++++++++++++++++++++++
arch/um/kernel/Makefile | 4 ++++
arch/um/kernel/kasan_init_um.c | 20 ++++++++++++++++++++
arch/um/kernel/skas/Makefile | 6 ++++++
arch/um/kernel/um_arch.c | 3 +++
arch/um/os-Linux/mem.c | 18 ++++++++++++++++++
arch/um/os-Linux/user_syms.c | 4 ++--
arch/x86/um/Makefile | 3 ++-
arch/x86/um/vdso/Makefile | 3 +++
kernel/Makefile | 6 ++++++
lib/Makefile | 10 ++++++++++
13 files changed, 117 insertions(+), 3 deletions(-)
create mode 100644 arch/um/include/asm/kasan.h
create mode 100644 arch/um/kernel/kasan_init_um.c

diff --git a/arch/um/Kconfig b/arch/um/Kconfig
index 6f0edd0c0220..99c68863e7e9 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
select HAVE_ARCH_SECCOMP_FILTER
select HAVE_ASM_MODVERSIONS
select HAVE_UID16
@@ -198,6 +199,15 @@ config UML_TIME_TRAVEL_SUPPORT

It is safe to say Y, but you probably don't need this.

+config KASAN_SHADOW_OFFSET
+ hex
+ depends on KASAN
+ default 0x100000000000
+ help
+ This is the offset at which the ~2.25TB of shadow memory is
+ initialized and used by KASAN for memory debugging. The default
+ is 0x100000000000.
+
endmenu

source "arch/um/drivers/Kconfig"
diff --git a/arch/um/include/asm/dma.h b/arch/um/include/asm/dma.h
index fdc53642c718..8aafd60d62bb 100644
--- a/arch/um/include/asm/dma.h
+++ b/arch/um/include/asm/dma.h
@@ -5,6 +5,7 @@
#include <asm/io.h>

extern unsigned long uml_physmem;
+extern unsigned long long physmem_size;

#define MAX_DMA_ADDRESS (uml_physmem)

diff --git a/arch/um/include/asm/kasan.h b/arch/um/include/asm/kasan.h
new file mode 100644
index 000000000000..ca4c43a35d41
--- /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)
+#ifdef CONFIG_X86_64
+#define KASAN_SHADOW_SIZE 0x100000000000UL
+#else
+#error "KASAN_SHADOW_SIZE is not defined in this sub-architecture"
+#endif
+
+// used in kasan_mem_to_shadow to divide by 8
+#define KASAN_SHADOW_SCALE_SHIFT 3
+
+#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_shadow(void);
+#else
+static inline void kasan_early_init(void) { }
+static inline void kasan_init(void) { }
+#endif /* CONFIG_KASAN */
+
+void kasan_map_memory(void *start, unsigned long len);
+void kasan_unpoison_shadow(const void *address, size_t size);
+
+#endif /* __ASM_UM_KASAN_H */
diff --git a/arch/um/kernel/Makefile b/arch/um/kernel/Makefile
index 5aa882011e04..f783a7dd863c 100644
--- a/arch/um/kernel/Makefile
+++ b/arch/um/kernel/Makefile
@@ -8,6 +8,9 @@
# kernel.
KCOV_INSTRUMENT := n

+# Do not instrument on main.o
+KASAN_SANITIZE := n
+
CPPFLAGS_vmlinux.lds := -DSTART=$(LDS_START) \
-DELF_ARCH=$(LDS_ELF_ARCH) \
-DELF_FORMAT=$(LDS_ELF_FORMAT) \
@@ -24,6 +27,7 @@ obj-$(CONFIG_GPROF) += gprof_syms.o
obj-$(CONFIG_GCOV) += gmon_syms.o
obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
obj-$(CONFIG_STACKTRACE) += stacktrace.o
+obj-$(CONFIG_KASAN) += kasan_init_um.o

USER_OBJS := config.o

diff --git a/arch/um/kernel/kasan_init_um.c b/arch/um/kernel/kasan_init_um.c
new file mode 100644
index 000000000000..2e9a85216fb5
--- /dev/null
+++ b/arch/um/kernel/kasan_init_um.c
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <asm/kasan.h>
+#include <linux/sched.h>
+#include <linux/sched/task.h>
+#include <asm/dma.h>
+#include <as-layout.h>
+
+void kasan_init(void)
+{
+ kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE);
+
+ // unpoison the kernel text which is form uml_physmem -> uml_reserved
+ kasan_unpoison_shadow((void *)uml_physmem, physmem_size);
+
+ // unpoison the vmalloc region, which is start_vm -> end_vm
+ kasan_unpoison_shadow((void *)start_vm, (end_vm - start_vm + 1));
+
+ init_task.kasan_depth = 0;
+ pr_info("KernelAddressSanitizer initialized\n");
+}
diff --git a/arch/um/kernel/skas/Makefile b/arch/um/kernel/skas/Makefile
index f3d494a4fd9b..d68f447274e5 100644
--- a/arch/um/kernel/skas/Makefile
+++ b/arch/um/kernel/skas/Makefile
@@ -5,6 +5,12 @@

obj-y := clone.o mmu.o process.o syscall.o uaccess.o

+ifdef CONFIG_UML
+# Do not instrument until after start_uml() because KASAN is not
+# initialized yet
+KASAN_SANITIZE := n
+endif
+
# clone.o is in the stub, so it can't be built with profiling
# GCC hardened also auto-enables -fpic, but we need %ebx so it can't work ->
# disable it
diff --git a/arch/um/kernel/um_arch.c b/arch/um/kernel/um_arch.c
index 0f40eccbd759..73cd159d28e8 100644
--- a/arch/um/kernel/um_arch.c
+++ b/arch/um/kernel/um_arch.c
@@ -14,6 +14,7 @@
#include <linux/sched/task.h>
#include <linux/kmsg_dump.h>

+#include <asm/kasan.h>
#include <asm/pgtable.h>
#include <asm/processor.h>
#include <asm/sections.h>
@@ -227,6 +228,8 @@ static struct notifier_block panic_exit_notifier = {

void uml_finishsetup(void)
{
+ kasan_init();
+
atomic_notifier_chain_register(&panic_notifier_list,
&panic_exit_notifier);

diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c
index 3c1b77474d2d..ef282bacc58e 100644
--- a/arch/um/os-Linux/mem.c
+++ b/arch/um/os-Linux/mem.c
@@ -17,6 +17,24 @@
#include <init.h>
#include <os.h>

+/**
+ * kasan_map_memory() - maps memory from @start with a size of @len
+ * @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", strerror(errno));
+}
+
/* 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/kernel/Makefile b/kernel/Makefile
index f2cc0d118a0b..4fbb72cb253f 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -32,6 +32,12 @@ KCOV_INSTRUMENT_kcov.o := n
KASAN_SANITIZE_kcov.o := n
CFLAGS_kcov.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)

+ifdef CONFIG_UML
+# Do not istrument kasan on panic because it can be called before KASAN
+# is initialized
+KASAN_SANITIZE_panic.o := n
+endif
+
# cond_syscall is currently not LTO compatible
CFLAGS_sys_ni.o = $(DISABLE_LTO)

diff --git a/lib/Makefile b/lib/Makefile
index 93217d44237f..e28dc5b06ae2 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -17,6 +17,16 @@ KCOV_INSTRUMENT_list_debug.o := n
KCOV_INSTRUMENT_debugobjects.o := n
KCOV_INSTRUMENT_dynamic_debug.o := n

+# Don't sanatize vsprintf or string functions in UM because they are used
+# before KASAN is initialized from cmdline parsing cmdline and kstrtox are
+# also called during uml initialization before KASAN is instrumented
+ifdef CONFIG_UML
+KASAN_SANITIZE_vsprintf.o := n
+KASAN_SANITIZE_string.o := n
+KASAN_SANITIZE_cmdline.o := n
+KASAN_SANITIZE_kstrtox.o := n
+endif
+
# Early boot use of cmdline, don't instrument it
ifdef CONFIG_AMD_MEM_ENCRYPT
KASAN_SANITIZE_string.o := n
--
2.25.0.rc1.283.g88dfdc4193-goog

[Johannes Berg]

Hi Patricia,

On Wed, 2020-01-15 at 10:28 -0800, Patricia Alfonso wrote:
> Make KASAN run on User Mode Linux on x86_64.

Oh wow, awesome! Just what I always wanted :-)

I tried this before and failed miserably ... mostly I thought we
actually needed CONFIG_CONSTRUCTORS, which doesn't work (at least I hope
my patch for it was reverted?) - do you know what's up with that?

Couple questions, if you don't mind.

> +#ifdef CONFIG_X86_64
> +#define KASAN_SHADOW_SIZE 0x100000000000UL
> +#else
> +#error "KASAN_SHADOW_SIZE is not defined in this sub-architecture"
> +#endif

Is it even possible today to compile ARCH=um on anything but x86_64? If
yes, perhaps the above should be

select HAVE_ARCH_KASAN if X86_64

or so? I assume KASAN itself has some dependencies though, but perhaps
ARM 64-bit or POWERPC 64-bit could possibly run into this, if not X86
32-bit.

> +++ b/arch/um/kernel/skas/Makefile
> @@ -5,6 +5,12 @@
>
> obj-y := clone.o mmu.o process.o syscall.o uaccess.o
>
> +ifdef CONFIG_UML
> +# Do not instrument until after start_uml() because KASAN is not
> +# initialized yet
> +KASAN_SANITIZE := n
> +endif

Not sure I understand this, can anything in this file even get compiled
without CONFIG_UML?

> +++ b/kernel/Makefile
> @@ -32,6 +32,12 @@ KCOV_INSTRUMENT_kcov.o := n
> KASAN_SANITIZE_kcov.o := n
> CFLAGS_kcov.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
>
> +ifdef CONFIG_UML
> +# Do not istrument kasan on panic because it can be called before KASAN

typo there - 'instrument'

> +# is initialized
> +KASAN_SANITIZE_panic.o := n
> +endif

but maybe UML shouldn't call panic() in such contexts, instead of this?
I've had some major trouble with calling into the kernel before things
are ready (or after we've started tearing things down), so that might be
a good thing overall anyway?

Could just do it this way and fix it later too though I guess.

> +++ b/lib/Makefile
> @@ -17,6 +17,16 @@ KCOV_INSTRUMENT_list_debug.o := n
> KCOV_INSTRUMENT_debugobjects.o := n
> KCOV_INSTRUMENT_dynamic_debug.o := n
>
> +# Don't sanatize

typo

> vsprintf or string functions in UM because they are used
> +# before KASAN is initialized from cmdline parsing cmdline and kstrtox are
> +# also called during uml initialization before KASAN is instrumented
> +ifdef CONFIG_UML
> +KASAN_SANITIZE_vsprintf.o := n
> +KASAN_SANITIZE_string.o := n
> +KASAN_SANITIZE_cmdline.o := n
> +KASAN_SANITIZE_kstrtox.o := n
> +endif

I guess this can't be avoided.


Very cool, I look forward to trying this out! :-)

Thanks,
johannes

[Patricia Alfonso]

On Wed, Jan 15, 2020 at 10:48 AM Johannes Berg
<joha...@sipsolutions.net> wrote:
> Couple questions, if you don't mind.
>
> > +#ifdef CONFIG_X86_64
> > +#define KASAN_SHADOW_SIZE 0x100000000000UL
> > +#else
> > +#error "KASAN_SHADOW_SIZE is not defined in this sub-architecture"
> > +#endif
>
> Is it even possible today to compile ARCH=um on anything but x86_64? If
> yes, perhaps the above should be
>
> select HAVE_ARCH_KASAN if X86_64
>
> or so? I assume KASAN itself has some dependencies though, but perhaps
> ARM 64-bit or POWERPC 64-bit could possibly run into this, if not X86
> 32-bit.
>

This seems like a good idea. I'll keep the #ifdef around
KASAN_SHADOW_SIZE, but add "select HAVE_ARCH_KASAN if X86_64" as well.
This will make extending it later easier.

> > +++ b/arch/um/kernel/skas/Makefile
> > @@ -5,6 +5,12 @@
> >
> > obj-y := clone.o mmu.o process.o syscall.o uaccess.o
> >
> > +ifdef CONFIG_UML
> > +# Do not instrument until after start_uml() because KASAN is not
> > +# initialized yet
> > +KASAN_SANITIZE := n
> > +endif
>
> Not sure I understand this, can anything in this file even get compiled
> without CONFIG_UML?
>

You are correct; this #ifdef was unnecessary. I will remove it. Thanks!

> > +++ b/kernel/Makefile
> > @@ -32,6 +32,12 @@ KCOV_INSTRUMENT_kcov.o := n
> > KASAN_SANITIZE_kcov.o := n
> > CFLAGS_kcov.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
> >
> > +ifdef CONFIG_UML
> > +# Do not istrument kasan on panic because it can be called before KASAN
>
> typo there - 'instrument'
>

Thanks for catching that!

> > +++ b/lib/Makefile
> > @@ -17,6 +17,16 @@ KCOV_INSTRUMENT_list_debug.o := n
> > KCOV_INSTRUMENT_debugobjects.o := n
> > KCOV_INSTRUMENT_dynamic_debug.o := n
> >
> > +# Don't sanatize
>
> typo
>

Thanks for catching this, too!

> Very cool, I look forward to trying this out! :-)
>
> Thanks,
> johannes
>

Thank you so much for the comments!

Best,
Patricia


--

Patricia Alfonso
Software Engineer
trisha...@google.com

[Johannes Berg]

Hi,

> This seems like a good idea. I'll keep the #ifdef around
> KASAN_SHADOW_SIZE, but add "select HAVE_ARCH_KASAN if X86_64" as well.
> This will make extending it later easier.

Yeah, that makes a lot of sense.

I think once somebody (Anton? Richard?) start applying patches again,
they will pick up my revert for CONFIG_CONSTRUCTORS:

https://patchwork.ozlabs.org/patch/1204275/

(See there for why I had to revert it)

If I remember correctly, KASAN depends on CONSTRUCTORS, so that revert
will then break your patch here?

And if I remember from looking at KASAN, some of the constructors there
depended on initializing after the KASAN data structures were set up (or
at least allocated)? It may be that you solved that by allocating the
shadow so very early though.

In any case, I think you should pick up that revert of
CONFIG_CONSTRUCTORS and see what you have to do to make it still work,
if that's possible.

If not, then ... tricky, not sure what to do. Maybe then we could
somehow hook in and have our own constructor that's called even before
the compiler-emitted ASAN constructors, to allocate the necessary data
structures.

johannes

[Johannes Berg]

On Thu, 2020-01-16 at 08:57 +0100, Johannes Berg wrote:
>
> And if I remember from looking at KASAN, some of the constructors there
> depended on initializing after the KASAN data structures were set up (or
> at least allocated)? It may be that you solved that by allocating the
> shadow so very early though.

Actually, no ... it's still after main(), and the constructors run
before.

So I _think_ with the CONFIG_CONSTRUCTORS revert, this will no longer
work (but happy to be proven wrong!), if so then I guess we do have to
find a way to initialize the KASAN things from another (somehow
earlier?) constructor ...

Or find a way to fix CONFIG_CONSTRUCTORS and not revert, but I looked at
it quite a bit and didn't.

johannes

[Dmitry Vyukov]

On Wed, Jan 15, 2020 at 11:56 PM Patricia Alfonso
<trisha...@google.com> wrote:
> > > +++ b/kernel/Makefile
> > > @@ -32,6 +32,12 @@ KCOV_INSTRUMENT_kcov.o := n
> > > KASAN_SANITIZE_kcov.o := n
> > > CFLAGS_kcov.o := $(call cc-option, -fno-conserve-stack -fno-stack-protector)
> > >
> > > +ifdef CONFIG_UML
> > > +# Do not istrument kasan on panic because it can be called before KASAN
> >
> > typo there - 'instrument'
> >
>
> Thanks for catching that!

Hi Patricia,

Very cool indeed! And will be a kunit killer feature!

I can't parse this sentence (even with fixed), what is "kasan on panic"?
Did you want to say "Do not instrument panic because it can be called
before KASAN is initialized"?
Or "Do not KASAN-instrument panic because it can be called before
KASAN is initialized"? Though, "KASAN-instrument" looks somewhat
redundant in this context.

[Dmitry Vyukov]

On Wed, Jan 15, 2020 at 7:28 PM Patricia Alfonso
<trisha...@google.com> wrote:
> +config KASAN_SHADOW_OFFSET
> + hex
> + depends on KASAN
> + default 0x100000000000
> + help
> + This is the offset at which the ~2.25TB of shadow memory is
> + initialized and used by KASAN for memory debugging. The default
> + is 0x100000000000.

What are restrictions on this value?
In user-space we use 0x7fff8000 as a base (just below 2GB) and it's
extremely profitable wrt codegen since it fits into immediate of most
instructions.
We can load and add the base with a short instruction:
2d8c: 48 81 c2 00 80 ff 7f add $0x7fff8000,%rdx
Or even add base, load shadow and check it with a single 7-byte instruction:
1e4: 80 b8 00 80 ff 7f 00 cmpb $0x0,0x7fff8000(%rax)

While with the large base, it takes 10 bytes just to load the const
into a register (current x86 KASAN codegen):
ffffffff81001571: 48 b8 00 00 00 00 00 fc ff df movabs
$0xdffffc0000000000,%rax
Most instructions don't have 8-byte immediates, so then we separately
need to add/load/check.

[Dmitry Vyukov]

On Wed, Jan 15, 2020 at 7:28 PM Patricia Alfonso
<trisha...@google.com> wrote:
>

> Make KASAN run on User Mode Linux on x86_64.

> diff --git a/arch/um/kernel/Makefile b/arch/um/kernel/Makefile
> index 5aa882011e04..f783a7dd863c 100644
> --- a/arch/um/kernel/Makefile
> +++ b/arch/um/kernel/Makefile
> @@ -8,6 +8,9 @@
> # kernel.
> KCOV_INSTRUMENT := n
>
> +# Do not instrument on main.o

It's always good to explain why. Otherwise it will be stuck there
forever because nobody really knows why and if they will break
something by removing it.
This comment is also somewhat confusing. We don't instrument the whole
dir, not just main.c? Should we ignore just this single file as
comment says?

> +KASAN_SANITIZE := n
> +

[Dmitry Vyukov]

> +void kasan_init(void)
> +{
> + kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE);
> +
> + // unpoison the kernel text which is form uml_physmem -> uml_reserved
> + kasan_unpoison_shadow((void *)uml_physmem, physmem_size);
> +
> + // unpoison the vmalloc region, which is start_vm -> end_vm
> + kasan_unpoison_shadow((void *)start_vm, (end_vm - start_vm + 1));
> +
> + init_task.kasan_depth = 0;
> + pr_info("KernelAddressSanitizer initialized\n");
> +}

Was this tested with stack instrumentation? Stack instrumentation
changes what shadow is being read/written and when. We don't need to
get it working right now, but if it does not work it would be nice to
restrict the setting and leave some comment traces for future
generations.

[Dmitry Vyukov]

On Wed, Jan 15, 2020 at 7:28 PM Patricia Alfonso
<trisha...@google.com> wrote:

> diff --git a/arch/um/include/asm/kasan.h b/arch/um/include/asm/kasan.h
> new file mode 100644
> index 000000000000..ca4c43a35d41
> --- /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)
> +#ifdef CONFIG_X86_64
> +#define KASAN_SHADOW_SIZE 0x100000000000UL

How was this number computed? Can we replace this with some formula?
I suspect this may be an order of magnitude off. Isn't 0x10000000000 enough?

> +#else
> +#error "KASAN_SHADOW_SIZE is not defined in this sub-architecture"
> +#endif
> +
> +// used in kasan_mem_to_shadow to divide by 8
> +#define KASAN_SHADOW_SCALE_SHIFT 3
> +
> +#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_shadow(void);
> +#else
> +static inline void kasan_early_init(void) { }
> +static inline void kasan_init(void) { }
> +#endif /* CONFIG_KASAN */
> +
> +void kasan_map_memory(void *start, unsigned long len);

This better be moved under #ifdef CONFIG_KASAN, it's not defined
otherwise, right?

> +void kasan_unpoison_shadow(const void *address, size_t size);

This is defined by <linux/kasan.h>. It's better to include that file
where you need this function. Or there are some issues with that?

> +
> +#endif /* __ASM_UM_KASAN_H */

> diff --git a/arch/um/kernel/kasan_init_um.c b/arch/um/kernel/kasan_init_um.c
> new file mode 100644
> index 000000000000..2e9a85216fb5
> --- /dev/null
> +++ b/arch/um/kernel/kasan_init_um.c
> @@ -0,0 +1,20 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <asm/kasan.h>
> +#include <linux/sched.h>
> +#include <linux/sched/task.h>
> +#include <asm/dma.h>
> +#include <as-layout.h>
> +
> +void kasan_init(void)
> +{
> + kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE);
> +
> + // unpoison the kernel text which is form uml_physmem -> uml_reserved

Why do we need to unpoison _text_? Who is accessing shadow for it? Do
you mean data/bss?
But on a more general point, we just allocated it with mmap, mmap
always gives zeroed memory and asan shadow is specifically arranged so
that 0's mean "good". So I don't think we need to unpoison anything
separately.

What may be more useful is to poison (or better mprotect, unmap, not
mmap) regions that kernel is not supposed to ever touch. One such
region is shadow self-mapping (shadow for shadow), in user-space we
mprotect that region. For KASAN we don't map shadow for user-space
part of VM, but I don't know if UML has such separation. We could also
protect other UML-specific regions if there are any, e.g does anybody
read/write text?

[Dmitry Vyukov]

Looking at this problem and at the number of KASAN_SANITIZE := n in
Makefiles (some of which are pretty sad, e.g. ignoring string.c,
kstrtox.c, vsprintf.c -- that's where the bugs are!), I think we
initialize KASAN too late. I think we need to do roughly what we do in
user-space asan (because it is user-space asan!). Constructors run
before main and it's really good, we need to initialize KASAN from
these constructors. Or if that's not enough in all cases, also add own
constructor/.preinit array entry to initialize as early as possible.
All we need to do is to call mmap syscall, there is really no
dependencies on anything kernel-related.
This should resolve the problem with constructors (after they
initialize KASAN, they can proceed to do anything they need) and it
should get rid of most KASAN_SANITIZE (in particular, all of
lib/Makefile and kernel/Makefile) and should fix stack instrumentation
(in case it does not work now). The only tiny bit we should not
instrument is the path from constructor up to mmap call.

[Johannes Berg]

On Thu, 2020-01-16 at 10:18 +0100, Dmitry Vyukov wrote:
>
> Looking at this problem and at the number of KASAN_SANITIZE := n in
> Makefiles (some of which are pretty sad, e.g. ignoring string.c,
> kstrtox.c, vsprintf.c -- that's where the bugs are!), I think we
> initialize KASAN too late. I think we need to do roughly what we do in
> user-space asan (because it is user-space asan!). Constructors run
> before main and it's really good, we need to initialize KASAN from
> these constructors. Or if that's not enough in all cases, also add own
> constructor/.preinit array entry to initialize as early as possible.

We even control the linker in this case, so we can put something into
the .preinit array *first*.

> All we need to do is to call mmap syscall, there is really no
> dependencies on anything kernel-related.

OK. I wasn't really familiar with those details.

> This should resolve the problem with constructors (after they
> initialize KASAN, they can proceed to do anything they need) and it
> should get rid of most KASAN_SANITIZE (in particular, all of
> lib/Makefile and kernel/Makefile) and should fix stack instrumentation
> (in case it does not work now). The only tiny bit we should not
> instrument is the path from constructor up to mmap call.

That'd be great :)

johannes

[Dmitry Vyukov]

On Thu, Jan 16, 2020 at 10:20 AM Johannes Berg
<joha...@sipsolutions.net> wrote:
>
> On Thu, 2020-01-16 at 10:18 +0100, Dmitry Vyukov wrote:
> >
> > Looking at this problem and at the number of KASAN_SANITIZE := n in
> > Makefiles (some of which are pretty sad, e.g. ignoring string.c,
> > kstrtox.c, vsprintf.c -- that's where the bugs are!), I think we
> > initialize KASAN too late. I think we need to do roughly what we do in
> > user-space asan (because it is user-space asan!). Constructors run
> > before main and it's really good, we need to initialize KASAN from
> > these constructors. Or if that's not enough in all cases, also add own
> > constructor/.preinit array entry to initialize as early as possible.
>
> We even control the linker in this case, so we can put something into
> the .preinit array *first*.

Even better! If we can reliably put something before constructors, we
don't even need lazy init in constructors.

> > All we need to do is to call mmap syscall, there is really no
> > dependencies on anything kernel-related.
>
> OK. I wasn't really familiar with those details.
>
> > This should resolve the problem with constructors (after they
> > initialize KASAN, they can proceed to do anything they need) and it
> > should get rid of most KASAN_SANITIZE (in particular, all of
> > lib/Makefile and kernel/Makefile) and should fix stack instrumentation
> > (in case it does not work now). The only tiny bit we should not
> > instrument is the path from constructor up to mmap call.
>
> That'd be great :)
>
> johannes
>

> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/2092169e6dd1f8d15f1db4b3787cc9fe596097b7.camel%40sipsolutions.net.

[Dmitry Vyukov]

On Thu, Jan 16, 2020 at 10:39 PM Patricia Alfonso
<trisha...@google.com> wrote:

>
> On Thu, Jan 16, 2020 at 1:23 AM Dmitry Vyukov <dvy...@google.com> wrote:
> >
> > On Thu, Jan 16, 2020 at 10:20 AM Johannes Berg
> > <joha...@sipsolutions.net> wrote:
> > >
> > > On Thu, 2020-01-16 at 10:18 +0100, Dmitry Vyukov wrote:
> > > >
> > > > Looking at this problem and at the number of KASAN_SANITIZE := n in
> > > > Makefiles (some of which are pretty sad, e.g. ignoring string.c,
> > > > kstrtox.c, vsprintf.c -- that's where the bugs are!), I think we
> > > > initialize KASAN too late. I think we need to do roughly what we do in
> > > > user-space asan (because it is user-space asan!). Constructors run
> > > > before main and it's really good, we need to initialize KASAN from
> > > > these constructors. Or if that's not enough in all cases, also add own
> > > > constructor/.preinit array entry to initialize as early as possible.
> > >
>

> I am not too happy with the number of KASAN_SANITIZE := n's either.
> This sounds like a good idea. Let me look into it; I am not familiar
> with constructors or .preint array.

>
> > > We even control the linker in this case, so we can put something into
> > > the .preinit array *first*.
> >
> > Even better! If we can reliably put something before constructors, we
> > don't even need lazy init in constructors.
> >
> > > > All we need to do is to call mmap syscall, there is really no
> > > > dependencies on anything kernel-related.
> > >
> > > OK. I wasn't really familiar with those details.
> > >
> > > > This should resolve the problem with constructors (after they
> > > > initialize KASAN, they can proceed to do anything they need) and it
> > > > should get rid of most KASAN_SANITIZE (in particular, all of
> > > > lib/Makefile and kernel/Makefile) and should fix stack instrumentation
> > > > (in case it does not work now). The only tiny bit we should not
> > > > instrument is the path from constructor up to mmap call.
>

> This sounds like a great solution. I am getting this KASAN report:
> "BUG: KASAN: stack-out-of-bounds in syscall_stub_data+0x2a5/0x2c7",
> which is probably because of this stack instrumentation problem you
> point out.

[reposting to the list]

If that part of the code I mentioned is instrumented, manifestation
would be different -- stack instrumentation will try to access shadow,
shadow is not mapped yet, so it would crash on the shadow access.

What you are seeing looks like, well, a kernel bug where it does a bad
stack access. Maybe it's KASAN actually _working_? :)

[Dmitry Vyukov]

Though, stack instrumentation may have issues with longjmp-like things.
I would suggest first turning off stack instrumentation and getting
that work. Solving problems one-by-one is always easier.
If you need help debugging this, please post more info: patch, what
you are doing, full kernel output (preferably from start, if it's not
too lengthy).

[Dmitry Vyukov]

I see syscall_stub_data does some weird things with stack (stack
copy?). Maybe we just need to ignore accesses there: individual
accesses, or whole function/file.

[Patricia Alfonso]

I've looked at this quite extensively over the past week or so. I was
able to initialize KASAN as one of the first things that gets executed
in main(), but constructors are, in fact, needed before main(). I
think it might be best to reintroduce constructors in a limited way to
allow KASAN to work in UML. I have done as much testing as I can on my
machine and this limited version seems to work, except when
STATIC_LINK is set. I will send some patches of what I have done so
far and we can talk more about it there. I would like to add your
name, Johannes, as a co-developed-by on that patch. If there is a
better way to give you credit for this, please let me know.


--
Patricia Alfonso

[Patricia Alfonso]

On Fri, Jan 17, 2020 at 2:05 AM Dmitry Vyukov <dvy...@google.com> wrote:
>
> On Fri, Jan 17, 2020 at 11:03 AM Dmitry Vyukov <dvy...@google.com> wrote:
> >
> > On Fri, Jan 17, 2020 at 10:59 AM Dmitry Vyukov <dvy...@google.com> wrote:
> > >
> > > On Thu, Jan 16, 2020 at 10:39 PM Patricia Alfonso
> > > <trisha...@google.com> wrote:
> > > >
> > > > On Thu, Jan 16, 2020 at 1:23 AM Dmitry Vyukov <dvy...@google.com> wrote:
> > > > >
> > > > > On Thu, Jan 16, 2020 at 10:20 AM Johannes Berg
> > > > > <joha...@sipsolutions.net> wrote:
> > > > > >
> > > > > > On Thu, 2020-01-16 at 10:18 +0100, Dmitry Vyukov wrote:
> > > > > > >
> > > > > > > This should resolve the problem with constructors (after they
> > > > > > > initialize KASAN, they can proceed to do anything they need) and it
> > > > > > > should get rid of most KASAN_SANITIZE (in particular, all of
> > > > > > > lib/Makefile and kernel/Makefile) and should fix stack instrumentation
> > > > > > > (in case it does not work now). The only tiny bit we should not
> > > > > > > instrument is the path from constructor up to mmap call.
> > > >

By initializing KASAN as the first thing that executes, I have been
able to get rid of most of the "KASAN_SANITIZE := n" lines and I am
very happy about that. Thanks for the suggestions!

> > > If that part of the code I mentioned is instrumented, manifestation
> > > would be different -- stack instrumentation will try to access shadow,
> > > shadow is not mapped yet, so it would crash on the shadow access.
> > >
> > > What you are seeing looks like, well, a kernel bug where it does a bad
> > > stack access. Maybe it's KASAN actually _working_? :)
> >
> > Though, stack instrumentation may have issues with longjmp-like things.
> > I would suggest first turning off stack instrumentation and getting
> > that work. Solving problems one-by-one is always easier.
> > If you need help debugging this, please post more info: patch, what
> > you are doing, full kernel output (preferably from start, if it's not
> > too lengthy).
>
> I see syscall_stub_data does some weird things with stack (stack
> copy?). Maybe we just need to ignore accesses there: individual
> accesses, or whole function/file.

It is still not clear whether the syscall_stub_data errors are false
positives, but while moving the kasan_init() to be as early as
possible in main(), I ran into a few more stack-related errors like
this(show_stack, dump_trace, and get_wchan). I will be taking your
advice to focus on one thing at a time and temporarily disable stack
instrumentation wherever possible.

--
Patricia Alfonso

[Johannes Berg]

Hi Patricia,

> I've looked at this quite extensively over the past week or so. I was
> able to initialize KASAN as one of the first things that gets executed
> in main(), but constructors are, in fact, needed before main().

They're called before main, by the dynamic loader, or libc, or whatever
magic is built into the binary, right? But what do you mean by "needed"?

> I
> think it might be best to reintroduce constructors in a limited way to
> allow KASAN to work in UML.

I guess I'd have to see that.

> I have done as much testing as I can on my
> machine and this limited version seems to work, except when
> STATIC_LINK is set. I will send some patches of what I have done so
> far and we can talk more about it there. I would like to add your
> name, Johannes, as a co-developed-by on that patch. If there is a
> better way to give you credit for this, please let me know.

I think you give me way too much credit, but I'm not going to complain
either way :-)

I'll post in a minute what I had in mind.

johannes

[Patricia Alfonso]

Make KASAN run on User Mode Linux on x86_64.

Depends on Constructor support in UML and is based off of
"[RFC PATCH] um: implement CONFIG_CONSTRUCTORS for modules"
(https://patchwork.ozlabs.org/patch/1234551/) and "[DEMO] um:
demonstrate super early constructors"
(https://patchwork.ozlabs.org/patch/1234553/) by
Johannes.

The location of the KASAN shadow memory, starting at
KASAN_SHADOW_OFFSET, can be configured using the
KASAN_SHADOW_OFFSET option. UML uses roughly 18TB of address

space, and KASAN requires 1/8th of this. The default location of
this offset is 0x100000000000. 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.

Signed-off-by: Patricia Alfonso <trisha...@google.com>
---

Changes since v1:
- KASAN has been initialized much earlier.
- With the help of Johannes's RFC patch to implement constructors in
UML and Demo showing how kasan_init could take advantage of these
super early constructors, most of the "KASAN_SANITIZE := n" have
been removed.
- Removed extraneous code
- Fixed typos

arch/um/Kconfig | 10 ++++++++++
arch/um/Makefile | 6 ++++++
arch/um/include/asm/dma.h | 1 +
arch/um/include/asm/kasan.h | 30 ++++++++++++++++++++++++++++++
arch/um/kernel/Makefile | 22 ++++++++++++++++++++++
arch/um/kernel/mem.c | 19 +++++++++----------
arch/um/os-Linux/mem.c | 19 +++++++++++++++++++

arch/um/os-Linux/user_syms.c | 4 ++--
arch/x86/um/Makefile | 3 ++-
arch/x86/um/vdso/Makefile | 3 +++

10 files changed, 104 insertions(+), 13 deletions(-)
create mode 100644 arch/um/include/asm/kasan.h

diff --git a/arch/um/Kconfig b/arch/um/Kconfig
index 0917f8443c28..2b76dc273731 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,15 @@ config UML_TIME_TRAVEL_SUPPORT

It is safe to say Y, but you probably don't need this.

+config KASAN_SHADOW_OFFSET
+ hex
+ depends on KASAN
+ default 0x100000000000
+ help
+ This is the offset at which the ~2.25TB of shadow memory is
+ initialized and used by KASAN for memory debugging. The default
+ is 0x100000000000.

+
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/dma.h b/arch/um/include/asm/dma.h
index fdc53642c718..8aafd60d62bb 100644
--- a/arch/um/include/asm/dma.h
+++ b/arch/um/include/asm/dma.h
@@ -5,6 +5,7 @@
#include <asm/io.h>

extern unsigned long uml_physmem;
+extern unsigned long long physmem_size;

#define MAX_DMA_ADDRESS (uml_physmem)

diff --git a/arch/um/include/asm/kasan.h b/arch/um/include/asm/kasan.h
new file mode 100644

index 000000000000..ba08061068cf
--- /dev/null
+++ b/arch/um/include/asm/kasan.h
@@ -0,0 +1,30 @@

+/* 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)
+#ifdef CONFIG_X86_64
+#define KASAN_SHADOW_SIZE 0x100000000000UL

+#else
+#error "KASAN_SHADOW_SIZE is not defined for this sub-architecture"
+#endif /* CONFIG_X86_64 */

+
+// used in kasan_mem_to_shadow to divide by 8
+#define KASAN_SHADOW_SCALE_SHIFT 3
+
+#define KASAN_SHADOW_START (KASAN_SHADOW_OFFSET)
+#define KASAN_SHADOW_END (KASAN_SHADOW_START + KASAN_SHADOW_SIZE)
+
+#ifdef CONFIG_KASAN
+void kasan_init(void);

+#else

+static inline void kasan_init(void) { }
+#endif /* CONFIG_KASAN */
+
+void kasan_map_memory(void *start, unsigned long len);

+void kasan_unpoison_shadow(const void *address, size_t size);

+
+#endif /* __ASM_UM_KASAN_H */

diff --git a/arch/um/kernel/Makefile b/arch/um/kernel/Makefile
index 5aa882011e04..875e1827588b 100644
--- a/arch/um/kernel/Makefile
+++ b/arch/um/kernel/Makefile
@@ -8,6 +8,28 @@

# kernel.
KCOV_INSTRUMENT := n

+# The way UMl deals with the stack causes seemingly false positive KASAN
+# reports such as:
+# BUG: KASAN: stack-out-of-bounds in show_stack+0x15e/0x1fb
+# Read of size 8 at addr 000000006184bbb0 by task swapper/1
+# ==================================================================
+# BUG: KASAN: stack-out-of-bounds in dump_trace+0x141/0x1c5
+# Read of size 8 at addr 0000000071057eb8 by task swapper/1
+# ==================================================================
+# BUG: KASAN: stack-out-of-bounds in get_wchan+0xd7/0x138
+# Read of size 8 at addr 0000000070e8fc80 by task systemd/1
+#
+# With these files removed from instrumentation, those reports are
+# eliminated, but KASAN still repeatedly reports a bug on syscall_stub_data:
+# ==================================================================
+# BUG: KASAN: stack-out-of-bounds in syscall_stub_data+0x299/0x2bf
+# Read of size 128 at addr 0000000071457c50 by task swapper/1
+
+KASAN_SANITIZE_stacktrace.o := n
+KASAN_SANITIZE_sysrq.o := n
+KASAN_SANITIZE_process.o := n
+

+
CPPFLAGS_vmlinux.lds := -DSTART=$(LDS_START) \
-DELF_ARCH=$(LDS_ELF_ARCH) \
-DELF_FORMAT=$(LDS_ELF_FORMAT) \

diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c
index 32fc941c80f7..7b7b8a0ee724 100644
--- a/arch/um/kernel/mem.c
+++ b/arch/um/kernel/mem.c
@@ -18,21 +18,20 @@
#include <kern_util.h>
#include <mem_user.h>
#include <os.h>
+#include <linux/sched/task.h>

-extern int printf(const char *msg, ...);
-static void early_print(void)
+#ifdef CONFIG_KASAN
+void kasan_init(void)
{
- printf("I'm super early, before constructors\n");
+ kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE);
+ init_task.kasan_depth = 0;
+ os_info("KernelAddressSanitizer initialized\n");
}

-static void __attribute__((constructor)) constructor_test(void)
-{
- printf("yes, you can see it\n");
-}
-
-static void (*early_print_ptr)(void)
+static void (*kasan_init_ptr)(void)
__attribute__((section(".kasan_init"), used))
- = early_print;
+= 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..da7039721d35 100644
--- a/arch/um/os-Linux/mem.c
+++ b/arch/um/os-Linux/mem.c
@@ -17,6 +17,25 @@

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

--
2.25.0.341.g760bfbb309-goog

[Johannes Berg]

Hi,

Looks very nice! Some questions/comments below:

> Depends on Constructor support in UML and is based off of
> "[RFC PATCH] um: implement CONFIG_CONSTRUCTORS for modules"
> (https://patchwork.ozlabs.org/patch/1234551/)

I guess I should resend this as a proper patch then. Did you test
modules? I can try (later) too.

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

That also means if I have say 512MB memory allocated for UML, KASAN will
use an *additional* 64, unlike on a "real" system, where KASAN will take
about 1/8th of the available physical memory, right?

> + help
> + This is the offset at which the ~2.25TB of shadow memory is
> + initialized

Maybe that should say "mapped" instead of "initialized", since there are
relatively few machines on which it could actually all all be used?

> +// used in kasan_mem_to_shadow to divide by 8
> +#define KASAN_SHADOW_SCALE_SHIFT 3

nit: use /* */ style comments

So that's actually something to fix still? Just trying to understand,
I'll test it later.

> -extern int printf(const char *msg, ...);
> -static void early_print(void)
> +#ifdef CONFIG_KASAN
> +void kasan_init(void)
> {
> - printf("I'm super early, before constructors\n");
> + kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE);

Heh, you *actually* based it on my patch, in git terms, not just in code
terms. I think you should just pick up the few lines that you need from
that patch and squash them into this one, I just posted that to
demonstrate more clearly what I meant :-)

> +/**
> + * kasan_map_memory() - maps memory from @start with a size of @len.

I think the () shouldn't be there?

> +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", strerror(errno));

If that fails, can we even continue?

johannes

[Dmitry Vyukov]

I started reviewing this, but I am spotting things that I already
commented on, like shadow start and about shadow size const. Please
either address them, or answer why they are not addressed, or add some
kind of TODOs so that I don't write the same comment again.

> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20200210225806.249297-1-trishalfonso%40google.com.

[Patricia Alfonso]

> I started reviewing this, but I am spotting things that I already
> commented on, like shadow start and about shadow size const. Please
> either address them, or answer why they are not addressed, or add some
> kind of TODOs so that I don't write the same comment again.

I'm sorry; They must have gotten lost in all the emails. I'll go
through them all again.
--
Patricia Alfonso

[Patricia Alfonso]

On Thu, Jan 16, 2020 at 12:44 AM Dmitry Vyukov <dvy...@google.com> wrote:
>
> On Wed, Jan 15, 2020 at 7:28 PM Patricia Alfonso
> <trisha...@google.com> wrote:
> > +config KASAN_SHADOW_OFFSET
> > + hex
> > + depends on KASAN
> > + default 0x100000000000
> > + help
> > + This is the offset at which the ~2.25TB of shadow memory is
> > + initialized and used by KASAN for memory debugging. The default
> > + is 0x100000000000.
>
> What are restrictions on this value?

The only restriction is that there is enough space there to map all of
the KASAN shadow memory without conflicting with anything else.

> In user-space we use 0x7fff8000 as a base (just below 2GB) and it's
> extremely profitable wrt codegen since it fits into immediate of most
> instructions.
> We can load and add the base with a short instruction:
> 2d8c: 48 81 c2 00 80 ff 7f add $0x7fff8000,%rdx
> Or even add base, load shadow and check it with a single 7-byte instruction:
> 1e4: 80 b8 00 80 ff 7f 00 cmpb $0x0,0x7fff8000(%rax)
>

I just tested with 0x7fff8000 as the KASAN_SHADOW_OFFSET and it worked
so I can make that the default if it will be more efficient.

--
Patricia Alfonso

[Patricia Alfonso]

If you are referring to KASAN_STACK_ENABLE, I just tested it and it
seems to work fine.

--
Patricia Alfonso

[Dmitry Vyukov]

I think it's the right thing to do if it works.

[Dmitry Vyukov]

I mean stack instrumentation which is enabled with CONFIG_KASAN_STACK.

[Patricia Alfonso]

I believe I was testing with CONFIG_KASAN_STACK set to 1 since that is
the default value when compiling with GCC.The syscall_stub_data error
disappears when the value of CONFIG_KASAN_STACK is 0, though.


--
Patricia Alfonso

[Patricia Alfonso]

On Tue, Feb 11, 2020 at 12:21 AM Johannes Berg
<joha...@sipsolutions.net> wrote:
>
> Hi,
>
> Looks very nice! Some questions/comments below:
>
> > Depends on Constructor support in UML and is based off of
> > "[RFC PATCH] um: implement CONFIG_CONSTRUCTORS for modules"
> > (https://patchwork.ozlabs.org/patch/1234551/)
>
> I guess I should resend this as a proper patch then. Did you test
> modules? I can try (later) too.
>

I have not tested modules - you might want to test modules before
sending it at a proper patch. I just know that it works for the
purposes of this KASAN project.

> > 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.
>
> That also means if I have say 512MB memory allocated for UML, KASAN will
> use an *additional* 64, unlike on a "real" system, where KASAN will take
> about 1/8th of the available physical memory, right?
>

Currently, the amount of shadow memory allocated is a constant based
on the amount of user space address space in x86_64 since this is the
host architecture I have focused on.

> > + help
> > + This is the offset at which the ~2.25TB of shadow memory is
> > + initialized
>
> Maybe that should say "mapped" instead of "initialized", since there are
> relatively few machines on which it could actually all all be used?
>

Valid point!

> > +// used in kasan_mem_to_shadow to divide by 8
> > +#define KASAN_SHADOW_SCALE_SHIFT 3
>
> nit: use /* */ style comments
>

Will do

Yes, I have not found a fix for these issues yet and even with these
few files excluded from instrumentation, the syscall_stub_data error
occurs(unless CONFIG_STACK is disabled, but CONFIG_STACK is enabled by
default when using gcc to compile). It is unclear whether this is a
bug that KASAN has found in UML or it is a mismatch of KASAN error
detection on UML.

> > -extern int printf(const char *msg, ...);
> > -static void early_print(void)
> > +#ifdef CONFIG_KASAN
> > +void kasan_init(void)
> > {
> > - printf("I'm super early, before constructors\n");
> > + kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE);
>
> Heh, you *actually* based it on my patch, in git terms, not just in code
> terms. I think you should just pick up the few lines that you need from
> that patch and squash them into this one, I just posted that to
> demonstrate more clearly what I meant :-)
>

I did base this on your patch. I figured it was more likely to get
merged before this patch anyway. To clarify, do you want me to include
your constructors patch with this one as a patchset?

> > +/**
> > + * kasan_map_memory() - maps memory from @start with a size of @len.
>
> I think the () shouldn't be there?
>

Okay!

> > +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", strerror(errno));
>
> If that fails, can we even continue?
>

Probably not, but with this executing before main(), what is the best
way to have an error occur? Or maybe there's a way we can just
continue without KASAN enabled and print to the console that KASAN
failed to initialize?

> johannes
>

--
Patricia Alfonso

[Dmitry Vyukov]

On Wed, Feb 12, 2020 at 11:25 PM Patricia Alfonso

Then I would either disable it for now for UML, or try to unpoision
stack or ignore accesses.

[Johannes Berg]

On Wed, 2020-02-12 at 16:37 -0800, Patricia Alfonso wrote:
>
> > That also means if I have say 512MB memory allocated for UML, KASAN will
> > use an *additional* 64, unlike on a "real" system, where KASAN will take
> > about 1/8th of the available physical memory, right?
> >
> Currently, the amount of shadow memory allocated is a constant based
> on the amount of user space address space in x86_64 since this is the
> host architecture I have focused on.

Right, but again like below - that's just mapped, not actually used. But
as far as I can tell, once you actually start running and potentially
use all of your mem=1024 (MB), you'll actually also use another 128MB on
the KASAN shadow, right?

Unlike, say, a real x86_64 machine where if you just have 1024 MB
physical memory, the KASAN shadow will have to fit into that as well.

> > > +# With these files removed from instrumentation, those reports are
> > > +# eliminated, but KASAN still repeatedly reports a bug on syscall_stub_data:
> > > +# ==================================================================
> > > +# BUG: KASAN: stack-out-of-bounds in syscall_stub_data+0x299/0x2bf
> > > +# Read of size 128 at addr 0000000071457c50 by task swapper/1
> >
> > So that's actually something to fix still? Just trying to understand,
> > I'll test it later.
> >
> Yes, I have not found a fix for these issues yet and even with these
> few files excluded from instrumentation, the syscall_stub_data error
> occurs(unless CONFIG_STACK is disabled, but CONFIG_STACK is enabled by
> default when using gcc to compile). It is unclear whether this is a
> bug that KASAN has found in UML or it is a mismatch of KASAN error
> detection on UML.

Right, ok, thanks for the explanation. I guess then stack
instrumentation should be disabled for this patch initially.

> > Heh, you *actually* based it on my patch, in git terms, not just in code
> > terms. I think you should just pick up the few lines that you need from
> > that patch and squash them into this one, I just posted that to
> > demonstrate more clearly what I meant :-)
> >
> I did base this on your patch. I figured it was more likely to get
> merged before this patch anyway. To clarify, do you want me to include
> your constructors patch with this one as a patchset?

Well I had two patches:
(1) the module constructors one - I guess we need to test it, but you
can include it here if you like. I'm kinda swamped with other
things right now, no promises I can actually test it soon, though I
really do want to because that's the case I need :)
(2) the [DEMO] patch - you should just take the few lines you need from
that (in the linker script) and stick it into this patch. Don't
even credit me for that, I only wrote it as a patch instead of a
normal text email reply because I couldn't figure out how to word
things in an understandable way...

Then we end up with 2 patches again, the (1) and your KASAN one. There's
no point in keeping the [DEMO] separate, and

> > > + 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", strerror(errno));
> >
> > If that fails, can we even continue?
> >
> Probably not, but with this executing before main(), what is the best
> way to have an error occur? Or maybe there's a way we can just
> continue without KASAN enabled and print to the console that KASAN
> failed to initialize?

You can always "exit(17)" or something.

I'm not sure you can continue without KASAN?

Arguably it's better to fail loudly anyway if something as simple as the
mmap() here fails - after all, that probably means the KASAN offset in
Kconfig needs to be adjusted?

johannes

[Dmitry Vyukov]

On Thu, Feb 13, 2020 at 9:19 AM Johannes Berg <joha...@sipsolutions.net> wrote:
>
> On Wed, 2020-02-12 at 16:37 -0800, Patricia Alfonso wrote:
> >
> > > That also means if I have say 512MB memory allocated for UML, KASAN will
> > > use an *additional* 64, unlike on a "real" system, where KASAN will take
> > > about 1/8th of the available physical memory, right?
> > >
> > Currently, the amount of shadow memory allocated is a constant based
> > on the amount of user space address space in x86_64 since this is the
> > host architecture I have focused on.
>
> Right, but again like below - that's just mapped, not actually used. But
> as far as I can tell, once you actually start running and potentially
> use all of your mem=1024 (MB), you'll actually also use another 128MB on
> the KASAN shadow, right?
>
> Unlike, say, a real x86_64 machine where if you just have 1024 MB
> physical memory, the KASAN shadow will have to fit into that as well.

Depends on what you mean by "real" :)
Real user-space ASAN will also reserve 1/8th of 47-bit VA on start
(16TB). This implementation seems to be much closer to user-space ASAN
rather than to x86_64 KASAN (in particular it seems to be mostly
portable across archs and is not really x86-specific, which is good).
I think it's reasonable and good, but the implementation difference
with other kernel arches may be worth noting somewhere in comments.

[Johannes Berg]

On Thu, 2020-02-13 at 09:44 +0100, Dmitry Vyukov wrote:

> > Right, but again like below - that's just mapped, not actually used. But
> > as far as I can tell, once you actually start running and potentially
> > use all of your mem=1024 (MB), you'll actually also use another 128MB on
> > the KASAN shadow, right?
> >
> > Unlike, say, a real x86_64 machine where if you just have 1024 MB
> > physical memory, the KASAN shadow will have to fit into that as well.
>
> Depends on what you mean by "real" :)

:)

> Real user-space ASAN will also reserve 1/8th of 47-bit VA on start
> (16TB).

Ah, but I was thinking of actual memory *used*, not just VA.

And of KASAN, not user-space, but yeah, good point.

> This implementation seems to be much closer to user-space ASAN
> rather than to x86_64 KASAN (in particular it seems to be mostly
> portable across archs and is not really x86-specific, which is good).

Indeed.

> I think it's reasonable and good, but the implementation difference
> with other kernel arches may be worth noting somewhere in comments.

Right, I guess that's the broader point. I was thinking mostly of the
memory consumption: if you run with UML KASAN, your UML virtual machine
will use around 12.5% more memory than before, unlike if you say have a
KVM virtual machine - whatever you reserve outside will be what it can
use inside, regardless of KASAN being enabled or not.

This is totally fine, I just thought it should be documented somewhere,
perhaps in the Kconfig option, though I guess there isn't a UML specific
one for this... Not sure where then.

johannes

[Patricia Alfonso]

Okay, I'll probably disable it in UML for now.


--
Patricia Alfonso

[Patricia Alfonso]

> Well I had two patches:
> (1) the module constructors one - I guess we need to test it, but you
> can include it here if you like. I'm kinda swamped with other
> things right now, no promises I can actually test it soon, though I
> really do want to because that's the case I need :)
> (2) the [DEMO] patch - you should just take the few lines you need from
> that (in the linker script) and stick it into this patch. Don't
> even credit me for that, I only wrote it as a patch instead of a
> normal text email reply because I couldn't figure out how to word
> things in an understandable way...
>
> Then we end up with 2 patches again, the (1) and your KASAN one. There's
> no point in keeping the [DEMO] separate, and
>

Okay, so I'll rebase onto (1) and just add the lines I need from the
[DEMO]. Are you sure you don't want to be named as a co-developed-by
at least?

>
> > > > + 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", strerror(errno));
> > >
> > > If that fails, can we even continue?
> > >
> > Probably not, but with this executing before main(), what is the best
> > way to have an error occur? Or maybe there's a way we can just
> > continue without KASAN enabled and print to the console that KASAN
> > failed to initialize?
>
> You can always "exit(17)" or something.
>
> I'm not sure you can continue without KASAN?
>
> Arguably it's better to fail loudly anyway if something as simple as the
> mmap() here fails - after all, that probably means the KASAN offset in
> Kconfig needs to be adjusted?
>
> johannes
>

Yeah, failing loudly does seem to be the best option here.

--
Patricia Alfonso

[Johannes Berg]

On Thu, 2020-02-13 at 16:54 -0800, Patricia Alfonso wrote:

> Okay, so I'll rebase onto (1) and just add the lines I need from the
> [DEMO]. Are you sure you don't want to be named as a co-developed-by
> at least?

Yeah ... it's like 3 lines of code? Don't worry about it :)

> Yeah, failing loudly does seem to be the best option here.

I just ran into that with userspace ASAN yesterday for some reason, so
yeah.

Perhaps good to tell people what to do - I couldn't actually solve the
issue I had in userspace yesterday. Here, could tell people to check the
address where it's mapped, or so?

johannes