| Message ID | 20220526010111.755166-1-davidgow@google.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | [RFC,v3] UML: add support for KASAN under x86_64 | expand |
On Wed, 2022-05-25 at 18:01 -0700, David Gow wrote: > > +#ifdef CONFIG_KASAN > +void kasan_init(void) > +{ > + /* > + * kasan_map_memory will map all of the required address space and > + * the host machine will allocate physical memory as necessary. > + */ > + kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE); > + init_task.kasan_depth = 0; > + os_info("KernelAddressSanitizer initialized\n"); > Can we remove this? Or maybe print it later somehow, when the other KASAN machinery initializes? As it is, this gets printed even if you run just "./linux --version" or "--help", which is a bit strange. johannes
On Thu, 26 May 2022 at 03:02, David Gow <davidgow@google.com> wrote: > > From: Patricia Alfonso <trishalfonso@google.com> > > Make KASAN run on User Mode Linux on x86_64. > > The UML-specific KASAN initializer uses mmap to map the roughly 2.25TB > of shadow memory to the location defined by KASAN_SHADOW_OFFSET. > kasan_init() utilizes constructors to initialize KASAN before main(). > > The location of the KASAN shadow memory, starting at > KASAN_SHADOW_OFFSET, can be configured using the KASAN_SHADOW_OFFSET > option. UML uses roughly 18TB of address space, and KASAN requires 1/8th > of this. The default location of this offset is 0x100000000000, which > keeps it out-of-the-way even on UML setups with more "physical" memory. > > For low-memory setups, 0x7fff8000 can be used instead, which fits in an > immediate and is therefore faster, as suggested by Dmitry Vyukov. There > is usually enough free space at this location; however, it is a config > option so that it can be easily changed if needed. > > Note that, unlike KASAN on other architectures, vmalloc allocations > still use the shadow memory allocated upfront, rather than allocating > and free-ing it per-vmalloc allocation. > > Signed-off-by: Patricia Alfonso <trishalfonso@google.com> > Co-developed-by: Vincent Whitchurch <vincent.whitchurch@axis.com> > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> > Signed-off-by: David Gow <davidgow@google.com> > --- > > This is a new RFC for the KASAN/UML port, based on the patch v1: > https://lore.kernel.org/all/20200226004608.8128-1-trishalfonso@google.com/ > > With several fixes by Vincent Whitchurch: > https://lore.kernel.org/all/20220525111756.GA15955@axis.com/ > > That thread describes the differences from the v1 (and hence the > previous RFCs better than I can here), but the gist of it is: > - Support for KASAN_VMALLOC, by changing the way > kasan_{populate,release}_vmalloc work to update existing shadow > memory, rather than allocating anything new. > - A similar fix for modules' shadow memory. > - Support for KASAN_STACK > - This requires the bugfix here: > https://lore.kernel.org/lkml/20220523140403.2361040-1-vincent.whitchurch@axis.com/ > - Plus a couple of files excluded from KASAN. > - Revert the default shadow offset to 0x100000000000 > - This was breaking when mem=1G for me, at least. > - A few minor fixes to linker sections and scripts. > - I've added one to dyn.lds.S on top of the ones Vincent added. Excited to see this revived! > There are still a few things to be sorted out before this is ready to go > upstream, in particular: > - We've got a bunch of checks for CONFIG_UML, where a more specific > config option might be better. For example: CONFIG_KASAN_NO_SHADOW_ALLOC. Probably. But with 1 arch setting it, I am fine either way. > - Alternatively, the vmalloc (and module) shadow memory allocators could > support per-architecture replacements. Humm... again hard to say while we have only 1 arch doing this. Another option: leave a comment on the first CONFIG_UML check listing these alternatives. When another arch needs something similar, then we can switch to one of these options. > - Do we want to the alignment before or after the __memset() in > kasan_populate_vmalloc()? I think you did it correctly (alignment after). 8 normal pages map to 1 shadow page. For the purposes of mapping pages lazily on other arches, we want to over-map. But for the memset, we want to clear only the shadow that relates to the current region. > - This doesn't seem to work when CONFIG_STATIC_LINK is enabled (because > libc crt0 code calls memory functions, which expect the shadow memory > to already exist, due to multiple symbols being resolved. > - I think we should just make this depend on dynamic UML. > - For that matter, I think static UML is actually broken at the > moment. I'll send a patch out tomorrow. I don't know how important the static build is for UML. Generally I prefer to build things statically b/c e.g. if a testing system builds on one machine but runs tests on another, dynamic link may be a problem. Or, say, if a testing system provides binary artifacts, and then nobody can run it locally. One potential way to fix it is to require outline KASAN instrumentation for static build and then make kasan_arch_is_ready() return false until the shadow is mapped. I see kasan_arch_is_ready() is checked at the beginning of all KASAN runtime entry points. But it would be nice if the dynamic build also supports inline and does not add kasan_arch_is_ready() check overhead. > - And there's a checkpatch complaint about a long __memset() line. > > Thanks again to everyone who's contributed and looked at these patches! > Note that I removed the Reviewed-by tags, as I think this version has > enough changes to warrant a re-review. > > -- David > > --- > arch/um/Kconfig | 15 +++++++++++++++ > arch/um/Makefile | 6 ++++++ > arch/um/include/asm/common.lds.S | 2 ++ > arch/um/kernel/Makefile | 3 +++ > arch/um/kernel/dyn.lds.S | 6 +++++- > arch/um/kernel/mem.c | 18 ++++++++++++++++++ > arch/um/os-Linux/mem.c | 22 ++++++++++++++++++++++ > arch/um/os-Linux/user_syms.c | 4 ++-- > arch/x86/um/Makefile | 3 ++- > arch/x86/um/vdso/Makefile | 3 +++ > mm/kasan/shadow.c | 20 +++++++++++++++++++- > 11 files changed, 97 insertions(+), 5 deletions(-) > > diff --git a/arch/um/Kconfig b/arch/um/Kconfig > index 4d398b80aea8..c28ea5c89381 100644 > --- a/arch/um/Kconfig > +++ b/arch/um/Kconfig > @@ -11,6 +11,8 @@ config UML > select ARCH_HAS_STRNLEN_USER > select ARCH_NO_PREEMPT > select HAVE_ARCH_AUDITSYSCALL > + select HAVE_ARCH_KASAN if X86_64 > + select HAVE_ARCH_KASAN_VMALLOC if HAVE_ARCH_KASAN > select HAVE_ARCH_SECCOMP_FILTER > select HAVE_ASM_MODVERSIONS > select HAVE_UID16 > @@ -219,6 +221,19 @@ config UML_TIME_TRAVEL_SUPPORT > > It is safe to say Y, but you probably don't need this. > > +config KASAN_SHADOW_OFFSET > + hex > + depends on KASAN > + default 0x100000000000 > + help > + This is the offset at which the ~2.25TB of shadow memory is > + mapped and used by KASAN for memory debugging. This can be any > + address that has at least KASAN_SHADOW_SIZE(total address space divided > + by 8) amount of space so that the KASAN shadow memory does not conflict > + with anything. The default is 0x100000000000, which works even if mem is > + set to a large value. On low-memory systems, try 0x7fff8000, as it fits > + into the immediate of most instructions, improving performance. > + > endmenu > > source "arch/um/drivers/Kconfig" > diff --git a/arch/um/Makefile b/arch/um/Makefile > index f2fe63bfd819..a98405f4ecb8 100644 > --- a/arch/um/Makefile > +++ b/arch/um/Makefile > @@ -75,6 +75,12 @@ USER_CFLAGS = $(patsubst $(KERNEL_DEFINES),,$(patsubst -I%,,$(KBUILD_CFLAGS))) \ > -D_FILE_OFFSET_BITS=64 -idirafter $(srctree)/include \ > -idirafter $(objtree)/include -D__KERNEL__ -D__UM_HOST__ > > +# Kernel config options are not included in USER_CFLAGS, but the option for KASAN > +# should be included if the KASAN config option was set. > +ifdef CONFIG_KASAN > + USER_CFLAGS+=-DCONFIG_KASAN=y > +endif > + > #This will adjust *FLAGS accordingly to the platform. > include $(srctree)/$(ARCH_DIR)/Makefile-os-$(OS) > > diff --git a/arch/um/include/asm/common.lds.S b/arch/um/include/asm/common.lds.S > index eca6c452a41b..fd481ac371de 100644 > --- a/arch/um/include/asm/common.lds.S > +++ b/arch/um/include/asm/common.lds.S > @@ -83,6 +83,8 @@ > } > .init_array : { > __init_array_start = .; > + *(.kasan_init) > + *(.init_array.*) > *(.init_array) > __init_array_end = .; > } > diff --git a/arch/um/kernel/Makefile b/arch/um/kernel/Makefile > index 1c2d4b29a3d4..a089217e2f0e 100644 > --- a/arch/um/kernel/Makefile > +++ b/arch/um/kernel/Makefile > @@ -27,6 +27,9 @@ obj-$(CONFIG_EARLY_PRINTK) += early_printk.o > obj-$(CONFIG_STACKTRACE) += stacktrace.o > obj-$(CONFIG_GENERIC_PCI_IOMAP) += ioport.o > > +KASAN_SANITIZE_stacktrace.o := n > +KASAN_SANITIZE_sysrq.o := n > + > USER_OBJS := config.o > > include arch/um/scripts/Makefile.rules > diff --git a/arch/um/kernel/dyn.lds.S b/arch/um/kernel/dyn.lds.S > index 2f2a8ce92f1e..2b7fc5b54164 100644 > --- a/arch/um/kernel/dyn.lds.S > +++ b/arch/um/kernel/dyn.lds.S > @@ -109,7 +109,11 @@ SECTIONS > be empty, which isn't pretty. */ > . = ALIGN(32 / 8); > .preinit_array : { *(.preinit_array) } > - .init_array : { *(.init_array) } > + .init_array : { > + *(.kasan_init) > + *(.init_array.*) > + *(.init_array) > + } > .fini_array : { *(.fini_array) } > .data : { > INIT_TASK_DATA(KERNEL_STACK_SIZE) > diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c > index 15295c3237a0..a32cfce53efb 100644 > --- a/arch/um/kernel/mem.c > +++ b/arch/um/kernel/mem.c > @@ -18,6 +18,24 @@ > #include <kern_util.h> > #include <mem_user.h> > #include <os.h> > +#include <linux/sched/task.h> > + > +#ifdef CONFIG_KASAN > +void kasan_init(void) > +{ > + /* > + * kasan_map_memory will map all of the required address space and > + * the host machine will allocate physical memory as necessary. > + */ > + kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE); > + init_task.kasan_depth = 0; > + os_info("KernelAddressSanitizer initialized\n"); > +} > + > +static void (*kasan_init_ptr)(void) > +__section(".kasan_init") __used > += kasan_init; > +#endif > > /* allocated in paging_init, zeroed in mem_init, and unchanged thereafter */ > unsigned long *empty_zero_page = NULL; > diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c > index 3c1b77474d2d..8530b2e08604 100644 > --- a/arch/um/os-Linux/mem.c > +++ b/arch/um/os-Linux/mem.c > @@ -17,6 +17,28 @@ > #include <init.h> > #include <os.h> > > +/* > + * kasan_map_memory - maps memory from @start with a size of @len. > + * The allocated memory is filled with zeroes upon success. > + * @start: the start address of the memory to be mapped > + * @len: the length of the memory to be mapped > + * > + * This function is used to map shadow memory for KASAN in uml > + */ > +void kasan_map_memory(void *start, size_t len) > +{ > + if (mmap(start, > + len, > + PROT_READ|PROT_WRITE, > + MAP_FIXED|MAP_ANONYMOUS|MAP_PRIVATE|MAP_NORESERVE, > + -1, > + 0) == MAP_FAILED) { > + os_info("Couldn't allocate shadow memory: %s\n.", > + strerror(errno)); > + exit(1); > + } > +} > + > /* Set by make_tempfile() during early boot. */ > static char *tempdir = NULL; > > diff --git a/arch/um/os-Linux/user_syms.c b/arch/um/os-Linux/user_syms.c > index 715594fe5719..cb667c9225ab 100644 > --- a/arch/um/os-Linux/user_syms.c > +++ b/arch/um/os-Linux/user_syms.c > @@ -27,10 +27,10 @@ EXPORT_SYMBOL(strstr); > #ifndef __x86_64__ > extern void *memcpy(void *, const void *, size_t); > EXPORT_SYMBOL(memcpy); > -#endif > - > EXPORT_SYMBOL(memmove); > EXPORT_SYMBOL(memset); > +#endif > + > EXPORT_SYMBOL(printf); > > /* Here, instead, I can provide a fake prototype. Yes, someone cares: genksyms. > diff --git a/arch/x86/um/Makefile b/arch/x86/um/Makefile > index ba5789c35809..f778e37494ba 100644 > --- a/arch/x86/um/Makefile > +++ b/arch/x86/um/Makefile > @@ -28,7 +28,8 @@ else > > obj-y += syscalls_64.o vdso/ > > -subarch-y = ../lib/csum-partial_64.o ../lib/memcpy_64.o ../entry/thunk_64.o > +subarch-y = ../lib/csum-partial_64.o ../lib/memcpy_64.o ../entry/thunk_64.o \ > + ../lib/memmove_64.o ../lib/memset_64.o > > endif > > diff --git a/arch/x86/um/vdso/Makefile b/arch/x86/um/vdso/Makefile > index 5943387e3f35..8c0396fd0e6f 100644 > --- a/arch/x86/um/vdso/Makefile > +++ b/arch/x86/um/vdso/Makefile > @@ -3,6 +3,9 @@ > # Building vDSO images for x86. > # > > +# do not instrument on vdso because KASAN is not compatible with user mode > +KASAN_SANITIZE := n > + > # Prevents link failures: __sanitizer_cov_trace_pc() is not linked in. > KCOV_INSTRUMENT := n > > diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c > index a4f07de21771..d8c518bd0e7d 100644 > --- a/mm/kasan/shadow.c > +++ b/mm/kasan/shadow.c > @@ -295,8 +295,14 @@ int kasan_populate_vmalloc(unsigned long addr, unsigned long size) > return 0; > > shadow_start = (unsigned long)kasan_mem_to_shadow((void *)addr); > - shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE); > shadow_end = (unsigned long)kasan_mem_to_shadow((void *)addr + size); > + > + if (IS_ENABLED(CONFIG_UML)) { > + __memset(kasan_mem_to_shadow((void *)addr), KASAN_VMALLOC_INVALID, shadow_end - shadow_start); "kasan_mem_to_shadow((void *)addr)" can be replaced with shadow_start. > + return 0; > + } > + > + shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE); > shadow_end = ALIGN(shadow_end, PAGE_SIZE); There is no new fancy PAGE_ALIGN macro for this. And I've seen people sending clean up patches with replacements. But unfortunately no PAGE_ALIGN_DOWN :( > > ret = apply_to_page_range(&init_mm, shadow_start, > @@ -466,6 +472,10 @@ void kasan_release_vmalloc(unsigned long start, unsigned long end, > > if (shadow_end > shadow_start) { > size = shadow_end - shadow_start; > + if (IS_ENABLED(CONFIG_UML)) { > + __memset(shadow_start, KASAN_SHADOW_INIT, shadow_end - shadow_start); > + return; > + } > apply_to_existing_page_range(&init_mm, > (unsigned long)shadow_start, > size, kasan_depopulate_vmalloc_pte, > @@ -531,6 +541,11 @@ int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask) > if (WARN_ON(!PAGE_ALIGNED(shadow_start))) > return -EINVAL; > > + if (IS_ENABLED(CONFIG_UML)) { > + __memset((void *)shadow_start, KASAN_SHADOW_INIT, shadow_size); > + return 0; > + } > + > ret = __vmalloc_node_range(shadow_size, 1, shadow_start, > shadow_start + shadow_size, > GFP_KERNEL, > @@ -554,6 +569,9 @@ int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask) > > void kasan_free_module_shadow(const struct vm_struct *vm) > { > + if (IS_ENABLED(CONFIG_UML)) > + return; > + > if (vm->flags & VM_KASAN) > vfree(kasan_mem_to_shadow(vm->addr)); > } > -- > 2.36.1.124.g0e6072fb45-goog >
On Fri, 2022-05-27 at 07:31 +0200, Dmitry Vyukov wrote: > > - This doesn't seem to work when CONFIG_STATIC_LINK is enabled (because > > libc crt0 code calls memory functions, which expect the shadow memory > > to already exist, due to multiple symbols being resolved. > > - I think we should just make this depend on dynamic UML. > > - For that matter, I think static UML is actually broken at the > > moment. I'll send a patch out tomorrow. > > I don't know how important the static build is for UML. Depends who you ask, I guess. IMHO just making KASAN depend on !STATIC_LINK is fine, until somebody actually wants to do what you describe: > Generally I prefer to build things statically b/c e.g. if a testing > system builds on one machine but runs tests on another, dynamic link > may be a problem. Or, say, if a testing system provides binary > artifacts, and then nobody can run it locally. > > One potential way to fix it is to require outline KASAN > instrumentation for static build and then make kasan_arch_is_ready() > return false until the shadow is mapped. I see kasan_arch_is_ready() > is checked at the beginning of all KASAN runtime entry points. > But it would be nice if the dynamic build also supports inline and > does not add kasan_arch_is_ready() check overhead. which sounds fine too, but ... trade-offs. > > + if (IS_ENABLED(CONFIG_UML)) { > > + __memset(kasan_mem_to_shadow((void *)addr), KASAN_VMALLOC_INVALID, shadow_end - shadow_start); > > "kasan_mem_to_shadow((void *)addr)" can be replaced with shadow_start. and then the memset line isn't so long anymore :) > > > > + return 0; > > + } > > + > > + shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE); > > shadow_end = ALIGN(shadow_end, PAGE_SIZE); > > There is no new fancy PAGE_ALIGN macro for this. And I've seen people s/no/now the/ I guess, but it's also existing code. johannes
On Wed, 2022-05-25 at 18:01 -0700, David Gow wrote: > > --- > arch/um/Kconfig | 15 +++++++++++++++ > arch/um/Makefile | 6 ++++++ > arch/um/include/asm/common.lds.S | 2 ++ > arch/um/kernel/Makefile | 3 +++ > arch/um/kernel/dyn.lds.S | 6 +++++- > arch/um/kernel/mem.c | 18 ++++++++++++++++++ > arch/um/os-Linux/mem.c | 22 ++++++++++++++++++++++ > arch/um/os-Linux/user_syms.c | 4 ++-- > arch/x86/um/Makefile | 3 ++- > arch/x86/um/vdso/Makefile | 3 +++ > mm/kasan/shadow.c | 20 +++++++++++++++++++- > Btw, it looks like you also forgot to git add the (new) file arch/um/include/asm/kasan.h from Patricia's patch? johannes
On Wed, 2022-05-25 at 18:01 -0700, David Gow wrote: > From: Patricia Alfonso <trishalfonso@google.com> > > Make KASAN run on User Mode Linux on x86_64. FWIW, I just added this to my virtual lab which I use as CI tests, and it immediately found a use-after-free bug in mac80211! I did note (this is more for kasan-dev@) that the "freed by" is fairly much useless when using kfree_rcu(), it might be worthwhile to annotate that somehow, so the stack trace is recorded by kfree_rcu() already, rather than just showing the RCU callback used for that. johannes
On Fri, 27 May 2022 at 15:05, Johannes Berg <johannes@sipsolutions.net> wrote: > > On Wed, 2022-05-25 at 18:01 -0700, David Gow wrote: > > From: Patricia Alfonso <trishalfonso@google.com> > > > > Make KASAN run on User Mode Linux on x86_64. > > FWIW, I just added this to my virtual lab which I use as CI tests, and > it immediately found a use-after-free bug in mac80211! > > I did note (this is more for kasan-dev@) that the "freed by" is fairly > much useless when using kfree_rcu(), it might be worthwhile to annotate > that somehow, so the stack trace is recorded by kfree_rcu() already, > rather than just showing the RCU callback used for that. KASAN is doing it for several years now, see e.g.: https://groups.google.com/g/syzkaller-bugs/c/eTW9zom4O2o/m/_v7cOo2RFwAJ
On Fri, 2022-05-27 at 15:09 +0200, Dmitry Vyukov wrote: > > I did note (this is more for kasan-dev@) that the "freed by" is fairly > > much useless when using kfree_rcu(), it might be worthwhile to annotate > > that somehow, so the stack trace is recorded by kfree_rcu() already, > > rather than just showing the RCU callback used for that. > > KASAN is doing it for several years now, see e.g.: > https://groups.google.com/g/syzkaller-bugs/c/eTW9zom4O2o/m/_v7cOo2RFwAJ > Hm. It didn't for me: > BUG: KASAN: use-after-free in ieee80211_vif_use_reserved_context+0x32d/0x40f [mac80211] > Read of size 4 at addr 0000000065c73340 by task kworker/u2:1/17 Yes. > CPU: 0 PID: 17 Comm: kworker/u2:1 Tainted: G O 5.18.0-rc1 #5 > Workqueue: phy0 ieee80211_chswitch_work [mac80211] > Stack: > 60ba783f 00000000 10000c268f4e 60ba783f > 60e60847 70dc9928 719f6e99 00000000 > 71883b20 60bb0b42 60bb0b19 65c73340 > Call Trace: > [<600447ea>] show_stack+0x13e/0x14d > [<60bb0b42>] dump_stack_lvl+0x29/0x2e > [<602ef7c0>] print_report+0x15d/0x60b > [<602efdc0>] kasan_report+0x98/0xbd > [<602f0cc2>] __asan_report_load4_noabort+0x1b/0x1d > [<719f6e99>] ieee80211_vif_use_reserved_context+0x32d/0x40f [mac80211] This is the user, it just got freed during a function call a few lines up. > Allocated by task 16: > save_stack_trace+0x2e/0x30 > stack_trace_save+0x81/0x9b > kasan_save_stack+0x2d/0x54 > kasan_set_track+0x34/0x3e > ____kasan_kmalloc+0x8d/0x99 > __kasan_kmalloc+0x10/0x12 > __kmalloc+0x1f6/0x20b > ieee80211_alloc_chanctx+0xdc/0x35f [mac80211] This makes sense too. > Freed by task 8: > save_stack_trace+0x2e/0x30 > stack_trace_save+0x81/0x9b > kasan_save_stack+0x2d/0x54 > kasan_set_track+0x34/0x3e > kasan_set_free_info+0x33/0x44 > ____kasan_slab_free+0x12b/0x149 > __kasan_slab_free+0x19/0x1b > slab_free_freelist_hook+0x10b/0x16a > kfree+0x10d/0x1fa > kvfree+0x38/0x3a > rcu_process_callbacks+0x2c5/0x349 This is the RCU callback. > Last potentially related work creation: > save_stack_trace+0x2e/0x30 > stack_trace_save+0x81/0x9b > kasan_save_stack+0x2d/0x54 > __kasan_record_aux_stack+0xd5/0xe2 > kasan_record_aux_stack_noalloc+0x12/0x14 > insert_work+0x50/0xd7 > __queue_work+0x805/0x95c > queue_work_on+0xba/0x131 > call_usermodehelper_exec+0x242/0x361 > kobject_uevent_env+0xe46/0xeaf > kobject_uevent+0x12/0x14 > driver_register+0x37e/0x38d > pcie_port_service_register+0x19d/0x1a5 This stuff is completely unrelated. > The buggy address belongs to the object at 0000000065c73300 > which belongs to the cache kmalloc-192 of size 192 > The buggy address is located 64 bytes inside of > 192-byte region [0000000065c73300, 0000000065c733c0) > and that's it? johannes
On Fri, 27 May 2022 at 15:15, Johannes Berg <johannes@sipsolutions.net> wrote: > > On Fri, 2022-05-27 at 15:09 +0200, Dmitry Vyukov wrote: > > > I did note (this is more for kasan-dev@) that the "freed by" is fairly > > > much useless when using kfree_rcu(), it might be worthwhile to annotate > > > that somehow, so the stack trace is recorded by kfree_rcu() already, > > > rather than just showing the RCU callback used for that. > > > > KASAN is doing it for several years now, see e.g.: > > https://groups.google.com/g/syzkaller-bugs/c/eTW9zom4O2o/m/_v7cOo2RFwAJ > > > > Hm. It didn't for me: Please post a full report with line numbers and kernel version. > > BUG: KASAN: use-after-free in ieee80211_vif_use_reserved_context+0x32d/0x40f [mac80211] > > Read of size 4 at addr 0000000065c73340 by task kworker/u2:1/17 > > Yes. > > > CPU: 0 PID: 17 Comm: kworker/u2:1 Tainted: G O 5.18.0-rc1 #5 > > Workqueue: phy0 ieee80211_chswitch_work [mac80211] > > Stack: > > 60ba783f 00000000 10000c268f4e 60ba783f > > 60e60847 70dc9928 719f6e99 00000000 > > 71883b20 60bb0b42 60bb0b19 65c73340 > > Call Trace: > > [<600447ea>] show_stack+0x13e/0x14d > > [<60bb0b42>] dump_stack_lvl+0x29/0x2e > > [<602ef7c0>] print_report+0x15d/0x60b > > [<602efdc0>] kasan_report+0x98/0xbd > > [<602f0cc2>] __asan_report_load4_noabort+0x1b/0x1d > > [<719f6e99>] ieee80211_vif_use_reserved_context+0x32d/0x40f [mac80211] > > This is the user, it just got freed during a function call a few lines > up. > > > Allocated by task 16: > > save_stack_trace+0x2e/0x30 > > stack_trace_save+0x81/0x9b > > kasan_save_stack+0x2d/0x54 > > kasan_set_track+0x34/0x3e > > ____kasan_kmalloc+0x8d/0x99 > > __kasan_kmalloc+0x10/0x12 > > __kmalloc+0x1f6/0x20b > > ieee80211_alloc_chanctx+0xdc/0x35f [mac80211] > > This makes sense too. > > > Freed by task 8: > > save_stack_trace+0x2e/0x30 > > stack_trace_save+0x81/0x9b > > kasan_save_stack+0x2d/0x54 > > kasan_set_track+0x34/0x3e > > kasan_set_free_info+0x33/0x44 > > ____kasan_slab_free+0x12b/0x149 > > __kasan_slab_free+0x19/0x1b > > slab_free_freelist_hook+0x10b/0x16a > > kfree+0x10d/0x1fa > > kvfree+0x38/0x3a > > rcu_process_callbacks+0x2c5/0x349 > > This is the RCU callback. > > > Last potentially related work creation: > > save_stack_trace+0x2e/0x30 > > stack_trace_save+0x81/0x9b > > kasan_save_stack+0x2d/0x54 > > __kasan_record_aux_stack+0xd5/0xe2 > > kasan_record_aux_stack_noalloc+0x12/0x14 > > insert_work+0x50/0xd7 > > __queue_work+0x805/0x95c > > queue_work_on+0xba/0x131 > > call_usermodehelper_exec+0x242/0x361 > > kobject_uevent_env+0xe46/0xeaf > > kobject_uevent+0x12/0x14 > > driver_register+0x37e/0x38d > > pcie_port_service_register+0x19d/0x1a5 > > This stuff is completely unrelated. > > > The buggy address belongs to the object at 0000000065c73300 > > which belongs to the cache kmalloc-192 of size 192 > > The buggy address is located 64 bytes inside of > > 192-byte region [0000000065c73300, 0000000065c733c0) > > > > and that's it? > > johannes > > -- > You received this message because you are subscribed to the Google Groups "kasan-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/6fa1ebe49b8d574fb1c82aefeeb54439d9c98750.camel%40sipsolutions.net.
On Fri, 2022-05-27 at 15:18 +0200, Dmitry Vyukov wrote: > On Fri, 27 May 2022 at 15:15, Johannes Berg <johannes@sipsolutions.net> wrote: > > > > On Fri, 2022-05-27 at 15:09 +0200, Dmitry Vyukov wrote: > > > > I did note (this is more for kasan-dev@) that the "freed by" is fairly > > > > much useless when using kfree_rcu(), it might be worthwhile to annotate > > > > that somehow, so the stack trace is recorded by kfree_rcu() already, > > > > rather than just showing the RCU callback used for that. > > > > > > KASAN is doing it for several years now, see e.g.: > > > https://groups.google.com/g/syzkaller-bugs/c/eTW9zom4O2o/m/_v7cOo2RFwAJ > > > > > > > Hm. It didn't for me: > > Please post a full report with line numbers and kernel version. That was basically it, apart from a few lines snipped from the stack traces. Kernel version was admittedly a little older - 5.18.0-rc1 + a few UML fixes + this KASAN patch (+ the fixes I pointed out earlier) I guess it doesn't really matter that much, just had to dig a bit to understand why it was freed. johannes
On Fri, 27 May 2022 at 15:27, Johannes Berg <johannes@sipsolutions.net> wrote: > > On Fri, 2022-05-27 at 15:18 +0200, Dmitry Vyukov wrote: > > On Fri, 27 May 2022 at 15:15, Johannes Berg <johannes@sipsolutions.net> wrote: > > > > > > On Fri, 2022-05-27 at 15:09 +0200, Dmitry Vyukov wrote: > > > > > I did note (this is more for kasan-dev@) that the "freed by" is fairly > > > > > much useless when using kfree_rcu(), it might be worthwhile to annotate > > > > > that somehow, so the stack trace is recorded by kfree_rcu() already, > > > > > rather than just showing the RCU callback used for that. > > > > > > > > KASAN is doing it for several years now, see e.g.: > > > > https://groups.google.com/g/syzkaller-bugs/c/eTW9zom4O2o/m/_v7cOo2RFwAJ > > > > > > > > > > Hm. It didn't for me: > > > > Please post a full report with line numbers and kernel version. > > That was basically it, apart from a few lines snipped from the stack > traces. Kernel version was admittedly a little older - 5.18.0-rc1 + a > few UML fixes + this KASAN patch (+ the fixes I pointed out earlier) > > I guess it doesn't really matter that much, just had to dig a bit to > understand why it was freed. Humm... I don't have any explanation based only on this info. Generally call_rcu stacks are memorized and I see the call is still there: https://elixir.bootlin.com/linux/v5.18/source/kernel/rcu/tree.c#L3595 It may be caused by some narrow races, depleted reserve memory in stackdepot, or race with quarantine eviction.
On Fri, 2022-05-27 at 15:52 +0200, Dmitry Vyukov wrote: > On Fri, 27 May 2022 at 15:27, Johannes Berg <johannes@sipsolutions.net> wrote: > > > > On Fri, 2022-05-27 at 15:18 +0200, Dmitry Vyukov wrote: > > > On Fri, 27 May 2022 at 15:15, Johannes Berg <johannes@sipsolutions.net> wrote: > > > > > > > > On Fri, 2022-05-27 at 15:09 +0200, Dmitry Vyukov wrote: > > > > > > I did note (this is more for kasan-dev@) that the "freed by" is fairly > > > > > > much useless when using kfree_rcu(), it might be worthwhile to annotate > > > > > > that somehow, so the stack trace is recorded by kfree_rcu() already, > > > > > > rather than just showing the RCU callback used for that. [...] > Humm... I don't have any explanation based only on this info. > Generally call_rcu stacks are memorized and I see the call is still there: > https://elixir.bootlin.com/linux/v5.18/source/kernel/rcu/tree.c#L3595 Oh, that's simple then, UML is !SMP && !PREEMPT so it gets TINY_RCU instead of TREE_RCU. Unfortunately, it's not entirely trivial to fix, something like this, mostly because of header maze (cannot include kasan.h in rcutiny.h): diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h index 5fed476f977f..d84e13f2c384 100644 --- a/include/linux/rcutiny.h +++ b/include/linux/rcutiny.h @@ -38,7 +38,7 @@ static inline void synchronize_rcu_expedited(void) */ extern void kvfree(const void *addr); -static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) +static inline void __kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) { if (head) { call_rcu(head, func); @@ -51,6 +51,15 @@ static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) kvfree((void *) func); } +#ifdef CONFIG_KASAN_GENERIC +void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func); +#else +static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) +{ + __kvfree_call_rcu(head, func); +} +#endif + void rcu_qs(void); static inline void rcu_softirq_qs(void) diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c index 340b3f8b090d..aa235f0332ba 100644 --- a/kernel/rcu/tiny.c +++ b/kernel/rcu/tiny.c @@ -217,6 +217,18 @@ bool poll_state_synchronize_rcu(unsigned long oldstate) } EXPORT_SYMBOL_GPL(poll_state_synchronize_rcu); +void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) +{ + if (head) { + void *ptr = (void *) head - (unsigned long) func; + + kasan_record_aux_stack_noalloc(ptr); + } + + __kvfree_call_rcu(head, func); +} +EXPORT_SYMBOL_GPL(kvfree_call_rcu); + void __init rcu_init(void) { open_softirq(RCU_SOFTIRQ, rcu_process_callbacks); Or I guess I could copy/paste #ifdef CONFIG_KASAN_GENERIC void kasan_record_aux_stack_noalloc(void *ptr); #else /* CONFIG_KASAN_GENERIC */ static inline void kasan_record_aux_stack_noalloc(void *ptr) {} #endif /* CONFIG_KASAN_GENERIC */ into rcutiny.h, that'd be smaller, and export the symbol ... johannes
On Fri, 27 May 2022 at 16:28, Johannes Berg <johannes@sipsolutions.net> wrote: > > > > > On Fri, 2022-05-27 at 15:09 +0200, Dmitry Vyukov wrote: > > > > > > > I did note (this is more for kasan-dev@) that the "freed by" is fairly > > > > > > > much useless when using kfree_rcu(), it might be worthwhile to annotate > > > > > > > that somehow, so the stack trace is recorded by kfree_rcu() already, > > > > > > > rather than just showing the RCU callback used for that. > [...] > > Humm... I don't have any explanation based only on this info. > > Generally call_rcu stacks are memorized and I see the call is still there: > > https://elixir.bootlin.com/linux/v5.18/source/kernel/rcu/tree.c#L3595 > > Oh, that's simple then, UML is !SMP && !PREEMPT so it gets TINY_RCU > instead of TREE_RCU. Nice! > Unfortunately, it's not entirely trivial to fix, something like this, > mostly because of header maze (cannot include kasan.h in rcutiny.h): > > diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h > index 5fed476f977f..d84e13f2c384 100644 > --- a/include/linux/rcutiny.h > +++ b/include/linux/rcutiny.h > @@ -38,7 +38,7 @@ static inline void synchronize_rcu_expedited(void) > */ > extern void kvfree(const void *addr); > > -static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > +static inline void __kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > { > if (head) { > call_rcu(head, func); > @@ -51,6 +51,15 @@ static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > kvfree((void *) func); > } > > +#ifdef CONFIG_KASAN_GENERIC > +void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func); > +#else > +static inline void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > +{ > + __kvfree_call_rcu(head, func); > +} > +#endif > + > void rcu_qs(void); > > static inline void rcu_softirq_qs(void) > diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c > index 340b3f8b090d..aa235f0332ba 100644 > --- a/kernel/rcu/tiny.c > +++ b/kernel/rcu/tiny.c > @@ -217,6 +217,18 @@ bool poll_state_synchronize_rcu(unsigned long oldstate) > } > EXPORT_SYMBOL_GPL(poll_state_synchronize_rcu); > > +void kvfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > +{ > + if (head) { > + void *ptr = (void *) head - (unsigned long) func; > + > + kasan_record_aux_stack_noalloc(ptr); > + } > + > + __kvfree_call_rcu(head, func); > +} > +EXPORT_SYMBOL_GPL(kvfree_call_rcu); > + > void __init rcu_init(void) > { > open_softirq(RCU_SOFTIRQ, rcu_process_callbacks); > > > > > Or I guess I could copy/paste > > #ifdef CONFIG_KASAN_GENERIC > void kasan_record_aux_stack_noalloc(void *ptr); > #else /* CONFIG_KASAN_GENERIC */ > static inline void kasan_record_aux_stack_noalloc(void *ptr) {} > #endif /* CONFIG_KASAN_GENERIC */ > > > into rcutiny.h, that'd be smaller, and export the symbol ... > > johannes > > -- > You received this message because you are subscribed to the Google Groups "kasan-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/5eef2f1b43c25447ccca2f50f4964fd77a719b08.camel%40sipsolutions.net.
diff --git a/arch/um/Kconfig b/arch/um/Kconfig index 4d398b80aea8..c28ea5c89381 100644 --- a/arch/um/Kconfig +++ b/arch/um/Kconfig @@ -11,6 +11,8 @@ config UML select ARCH_HAS_STRNLEN_USER select ARCH_NO_PREEMPT select HAVE_ARCH_AUDITSYSCALL + select HAVE_ARCH_KASAN if X86_64 + select HAVE_ARCH_KASAN_VMALLOC if HAVE_ARCH_KASAN select HAVE_ARCH_SECCOMP_FILTER select HAVE_ASM_MODVERSIONS select HAVE_UID16 @@ -219,6 +221,19 @@ config UML_TIME_TRAVEL_SUPPORT It is safe to say Y, but you probably don't need this. +config KASAN_SHADOW_OFFSET + hex + depends on KASAN + default 0x100000000000 + help + This is the offset at which the ~2.25TB of shadow memory is + mapped and used by KASAN for memory debugging. This can be any + address that has at least KASAN_SHADOW_SIZE(total address space divided + by 8) amount of space so that the KASAN shadow memory does not conflict + with anything. The default is 0x100000000000, which works even if mem is + set to a large value. On low-memory systems, try 0x7fff8000, as it fits + into the immediate of most instructions, improving performance. + endmenu source "arch/um/drivers/Kconfig" diff --git a/arch/um/Makefile b/arch/um/Makefile index f2fe63bfd819..a98405f4ecb8 100644 --- a/arch/um/Makefile +++ b/arch/um/Makefile @@ -75,6 +75,12 @@ USER_CFLAGS = $(patsubst $(KERNEL_DEFINES),,$(patsubst -I%,,$(KBUILD_CFLAGS))) \ -D_FILE_OFFSET_BITS=64 -idirafter $(srctree)/include \ -idirafter $(objtree)/include -D__KERNEL__ -D__UM_HOST__ +# Kernel config options are not included in USER_CFLAGS, but the option for KASAN +# should be included if the KASAN config option was set. +ifdef CONFIG_KASAN + USER_CFLAGS+=-DCONFIG_KASAN=y +endif + #This will adjust *FLAGS accordingly to the platform. include $(srctree)/$(ARCH_DIR)/Makefile-os-$(OS) diff --git a/arch/um/include/asm/common.lds.S b/arch/um/include/asm/common.lds.S index eca6c452a41b..fd481ac371de 100644 --- a/arch/um/include/asm/common.lds.S +++ b/arch/um/include/asm/common.lds.S @@ -83,6 +83,8 @@ } .init_array : { __init_array_start = .; + *(.kasan_init) + *(.init_array.*) *(.init_array) __init_array_end = .; } diff --git a/arch/um/kernel/Makefile b/arch/um/kernel/Makefile index 1c2d4b29a3d4..a089217e2f0e 100644 --- a/arch/um/kernel/Makefile +++ b/arch/um/kernel/Makefile @@ -27,6 +27,9 @@ obj-$(CONFIG_EARLY_PRINTK) += early_printk.o obj-$(CONFIG_STACKTRACE) += stacktrace.o obj-$(CONFIG_GENERIC_PCI_IOMAP) += ioport.o +KASAN_SANITIZE_stacktrace.o := n +KASAN_SANITIZE_sysrq.o := n + USER_OBJS := config.o include arch/um/scripts/Makefile.rules diff --git a/arch/um/kernel/dyn.lds.S b/arch/um/kernel/dyn.lds.S index 2f2a8ce92f1e..2b7fc5b54164 100644 --- a/arch/um/kernel/dyn.lds.S +++ b/arch/um/kernel/dyn.lds.S @@ -109,7 +109,11 @@ SECTIONS be empty, which isn't pretty. */ . = ALIGN(32 / 8); .preinit_array : { *(.preinit_array) } - .init_array : { *(.init_array) } + .init_array : { + *(.kasan_init) + *(.init_array.*) + *(.init_array) + } .fini_array : { *(.fini_array) } .data : { INIT_TASK_DATA(KERNEL_STACK_SIZE) diff --git a/arch/um/kernel/mem.c b/arch/um/kernel/mem.c index 15295c3237a0..a32cfce53efb 100644 --- a/arch/um/kernel/mem.c +++ b/arch/um/kernel/mem.c @@ -18,6 +18,24 @@ #include <kern_util.h> #include <mem_user.h> #include <os.h> +#include <linux/sched/task.h> + +#ifdef CONFIG_KASAN +void kasan_init(void) +{ + /* + * kasan_map_memory will map all of the required address space and + * the host machine will allocate physical memory as necessary. + */ + kasan_map_memory((void *)KASAN_SHADOW_START, KASAN_SHADOW_SIZE); + init_task.kasan_depth = 0; + os_info("KernelAddressSanitizer initialized\n"); +} + +static void (*kasan_init_ptr)(void) +__section(".kasan_init") __used += kasan_init; +#endif /* allocated in paging_init, zeroed in mem_init, and unchanged thereafter */ unsigned long *empty_zero_page = NULL; diff --git a/arch/um/os-Linux/mem.c b/arch/um/os-Linux/mem.c index 3c1b77474d2d..8530b2e08604 100644 --- a/arch/um/os-Linux/mem.c +++ b/arch/um/os-Linux/mem.c @@ -17,6 +17,28 @@ #include <init.h> #include <os.h> +/* + * kasan_map_memory - maps memory from @start with a size of @len. + * The allocated memory is filled with zeroes upon success. + * @start: the start address of the memory to be mapped + * @len: the length of the memory to be mapped + * + * This function is used to map shadow memory for KASAN in uml + */ +void kasan_map_memory(void *start, size_t len) +{ + if (mmap(start, + len, + PROT_READ|PROT_WRITE, + MAP_FIXED|MAP_ANONYMOUS|MAP_PRIVATE|MAP_NORESERVE, + -1, + 0) == MAP_FAILED) { + os_info("Couldn't allocate shadow memory: %s\n.", + strerror(errno)); + exit(1); + } +} + /* Set by make_tempfile() during early boot. */ static char *tempdir = NULL; diff --git a/arch/um/os-Linux/user_syms.c b/arch/um/os-Linux/user_syms.c index 715594fe5719..cb667c9225ab 100644 --- a/arch/um/os-Linux/user_syms.c +++ b/arch/um/os-Linux/user_syms.c @@ -27,10 +27,10 @@ EXPORT_SYMBOL(strstr); #ifndef __x86_64__ extern void *memcpy(void *, const void *, size_t); EXPORT_SYMBOL(memcpy); -#endif - EXPORT_SYMBOL(memmove); EXPORT_SYMBOL(memset); +#endif + EXPORT_SYMBOL(printf); /* Here, instead, I can provide a fake prototype. Yes, someone cares: genksyms. diff --git a/arch/x86/um/Makefile b/arch/x86/um/Makefile index ba5789c35809..f778e37494ba 100644 --- a/arch/x86/um/Makefile +++ b/arch/x86/um/Makefile @@ -28,7 +28,8 @@ else obj-y += syscalls_64.o vdso/ -subarch-y = ../lib/csum-partial_64.o ../lib/memcpy_64.o ../entry/thunk_64.o +subarch-y = ../lib/csum-partial_64.o ../lib/memcpy_64.o ../entry/thunk_64.o \ + ../lib/memmove_64.o ../lib/memset_64.o endif diff --git a/arch/x86/um/vdso/Makefile b/arch/x86/um/vdso/Makefile index 5943387e3f35..8c0396fd0e6f 100644 --- a/arch/x86/um/vdso/Makefile +++ b/arch/x86/um/vdso/Makefile @@ -3,6 +3,9 @@ # Building vDSO images for x86. # +# do not instrument on vdso because KASAN is not compatible with user mode +KASAN_SANITIZE := n + # Prevents link failures: __sanitizer_cov_trace_pc() is not linked in. KCOV_INSTRUMENT := n diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c index a4f07de21771..d8c518bd0e7d 100644 --- a/mm/kasan/shadow.c +++ b/mm/kasan/shadow.c @@ -295,8 +295,14 @@ int kasan_populate_vmalloc(unsigned long addr, unsigned long size) return 0; shadow_start = (unsigned long)kasan_mem_to_shadow((void *)addr); - shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE); shadow_end = (unsigned long)kasan_mem_to_shadow((void *)addr + size); + + if (IS_ENABLED(CONFIG_UML)) { + __memset(kasan_mem_to_shadow((void *)addr), KASAN_VMALLOC_INVALID, shadow_end - shadow_start); + return 0; + } + + shadow_start = ALIGN_DOWN(shadow_start, PAGE_SIZE); shadow_end = ALIGN(shadow_end, PAGE_SIZE); ret = apply_to_page_range(&init_mm, shadow_start, @@ -466,6 +472,10 @@ void kasan_release_vmalloc(unsigned long start, unsigned long end, if (shadow_end > shadow_start) { size = shadow_end - shadow_start; + if (IS_ENABLED(CONFIG_UML)) { + __memset(shadow_start, KASAN_SHADOW_INIT, shadow_end - shadow_start); + return; + } apply_to_existing_page_range(&init_mm, (unsigned long)shadow_start, size, kasan_depopulate_vmalloc_pte, @@ -531,6 +541,11 @@ int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask) if (WARN_ON(!PAGE_ALIGNED(shadow_start))) return -EINVAL; + if (IS_ENABLED(CONFIG_UML)) { + __memset((void *)shadow_start, KASAN_SHADOW_INIT, shadow_size); + return 0; + } + ret = __vmalloc_node_range(shadow_size, 1, shadow_start, shadow_start + shadow_size, GFP_KERNEL, @@ -554,6 +569,9 @@ int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask) void kasan_free_module_shadow(const struct vm_struct *vm) { + if (IS_ENABLED(CONFIG_UML)) + return; + if (vm->flags & VM_KASAN) vfree(kasan_mem_to_shadow(vm->addr)); }