Message ID | 20221022180455.never.023-kees@kernel.org (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | mm: Make ksize() a reporting-only function | expand |
Greeting, FYI, we noticed BUG:KASAN:slab-out-of-bounds_in_copy_array due to commit (built with gcc-11): commit: d916d97f33952312ca6b8240b40ed79cc9cb04b2 ("[PATCH] mm: Make ksize() a reporting-only function") url: https://github.com/intel-lab-lkp/linux/commits/Kees-Cook/mm-Make-ksize-a-reporting-only-function/20221023-020908 base: https://git.kernel.org/cgit/linux/kernel/git/akpm/mm.git mm-everything patch link: https://lore.kernel.org/linux-mm/20221022180455.never.023-kees@kernel.org patch subject: [PATCH] mm: Make ksize() a reporting-only function in testcase: boot on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace): If you fix the issue, kindly add following tag | Reported-by: kernel test robot <oliver.sang@intel.com> | Link: https://lore.kernel.org/r/202210241317.54083ce6-oliver.sang@intel.com [ 32.059131][ T1] BUG: KASAN: slab-out-of-bounds in copy_array (kbuild/src/x86_64-3/arch/x86/kernel/cpu/hypervisor.c:91) [ 32.059705][ T1] Write of size 32 at addr ffff88814b9d75c0 by task systemd/1 [ 32.060330][ T1] [ 32.060576][ T1] CPU: 1 PID: 1 Comm: systemd Not tainted 6.1.0-rc1-00216-gd916d97f3395 #2 [ 32.061273][ T1] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-debian-1.16.0-4 04/01/2014 [ 32.062082][ T1] Call Trace: [ 32.062393][ T1] <TASK> [ 32.062680][ T1] dump_stack_lvl (kbuild/src/x86_64-3/lib/dump_stack.c:107 (discriminator 4)) [ 32.063071][ T1] print_address_description+0x87/0x2a5 [ 32.063609][ T1] print_report (kbuild/src/x86_64-3/mm/kasan/report.c:365) [ 32.064003][ T1] ? kasan_addr_to_slab (kbuild/src/x86_64-3/mm/kasan/common.c:35) [ 32.064424][ T1] ? copy_array (kbuild/src/x86_64-3/arch/x86/kernel/cpu/hypervisor.c:91) [ 32.064801][ T1] kasan_report (kbuild/src/x86_64-3/mm/kasan/report.c:131 kbuild/src/x86_64-3/mm/kasan/report.c:466) [ 32.065188][ T1] ? copy_array (kbuild/src/x86_64-3/arch/x86/kernel/cpu/hypervisor.c:91) [ 32.065570][ T1] kasan_check_range (kbuild/src/x86_64-3/mm/kasan/generic.c:190) [ 32.065986][ T1] memcpy (kbuild/src/x86_64-3/mm/kasan/shadow.c:65 (discriminator 1)) [ 32.066333][ T1] copy_array (kbuild/src/x86_64-3/arch/x86/kernel/cpu/hypervisor.c:91) [ 32.066699][ T1] copy_verifier_state (kbuild/src/x86_64-3/kernel/bpf/verifier.c:1189) [ 32.067124][ T1] pop_stack (kbuild/src/x86_64-3/kernel/bpf/verifier.c:1252) [ 32.067502][ T1] do_check (kbuild/src/x86_64-3/kernel/bpf/verifier.c:12441) [ 32.067878][ T1] ? lock_downgrade (kbuild/src/x86_64-3/kernel/locking/lockdep.c:5320) [ 32.068294][ T1] ? check_helper_call (kbuild/src/x86_64-3/kernel/bpf/verifier.c:12144) [ 32.068737][ T1] ? kasan_quarantine_put (kbuild/src/x86_64-3/arch/x86/include/asm/irqflags.h:45 (discriminator 1) kbuild/src/x86_64-3/arch/x86/include/asm/irqflags.h:80 (discriminator 1) kbuild/src/x86_64-3/arch/x86/include/asm/irqflags.h:138 (discriminator 1) kbuild/src/x86_64-3/mm/kasan/quarantine.c:242 (discriminator 1)) [ 32.069181][ T1] ? trace_hardirqs_on (kbuild/src/x86_64-3/kernel/trace/trace_preemptirq.c:50 (discriminator 22)) [ 32.069605][ T1] ? memset (kbuild/src/x86_64-3/mm/kasan/shadow.c:44) [ 32.069960][ T1] ? memset (kbuild/src/x86_64-3/mm/kasan/shadow.c:44) [ 32.070317][ T1] ? memset (kbuild/src/x86_64-3/mm/kasan/shadow.c:44) [ 32.070683][ T1] do_check_common (kbuild/src/x86_64-3/kernel/bpf/verifier.c:14642) [ 32.071088][ T1] ? check_cfg (kbuild/src/x86_64-3/kernel/bpf/verifier.c:10938) [ 32.071483][ T1] bpf_check (kbuild/src/x86_64-3/kernel/bpf/verifier.c:14705 kbuild/src/x86_64-3/kernel/bpf/verifier.c:15275) [ 32.071869][ T1] ? find_held_lock (kbuild/src/x86_64-3/kernel/locking/lockdep.c:5158) [ 32.072279][ T1] ? bpf_prog_load (kbuild/src/x86_64-3/kernel/bpf/syscall.c:2598) [ 32.072696][ T1] ? bpf_get_btf_vmlinux (kbuild/src/x86_64-3/kernel/bpf/verifier.c:15159) [ 32.073124][ T1] ? lockdep_hardirqs_on_prepare (kbuild/src/x86_64-3/kernel/locking/lockdep.c:4528) [ 32.073647][ T1] ? memset (kbuild/src/x86_64-3/mm/kasan/shadow.c:44) [ 32.074006][ T1] bpf_prog_load (kbuild/src/x86_64-3/kernel/bpf/syscall.c:2605) [ 32.074412][ T1] ? bpf_prog_bind_map (kbuild/src/x86_64-3/kernel/bpf/syscall.c:2464) [ 32.074839][ T1] ? lock_acquire (kbuild/src/x86_64-3/kernel/locking/lockdep.c:466 kbuild/src/x86_64-3/kernel/locking/lockdep.c:5670 kbuild/src/x86_64-3/kernel/locking/lockdep.c:5633) [ 32.075244][ T1] ? __might_fault (kbuild/src/x86_64-3/mm/memory.c:5645 kbuild/src/x86_64-3/mm/memory.c:5638) [ 32.075654][ T1] ? lock_downgrade (kbuild/src/x86_64-3/kernel/locking/lockdep.c:5320) [ 32.076067][ T1] ? lock_is_held_type (kbuild/src/x86_64-3/kernel/locking/lockdep.c:5409 kbuild/src/x86_64-3/kernel/locking/lockdep.c:5711) [ 32.076489][ T1] ? __might_fault (kbuild/src/x86_64-3/mm/memory.c:5645 kbuild/src/x86_64-3/mm/memory.c:5638) [ 32.076887][ T1] ? lock_release (kbuild/src/x86_64-3/kernel/locking/lockdep.c:466 kbuild/src/x86_64-3/kernel/locking/lockdep.c:5690) [ 32.077284][ T1] __sys_bpf (kbuild/src/x86_64-3/kernel/bpf/syscall.c:4965) [ 32.077662][ T1] ? link_create (kbuild/src/x86_64-3/kernel/bpf/syscall.c:4912) [ 32.078055][ T1] ? trace_hardirqs_on (kbuild/src/x86_64-3/kernel/trace/trace_preemptirq.c:50 (discriminator 22)) [ 32.079667][ T1] ? task_work_run (kbuild/src/x86_64-3/kernel/task_work.c:182 (discriminator 1)) [ 32.080090][ T1] __ia32_sys_bpf (kbuild/src/x86_64-3/kernel/bpf/syscall.c:5067) [ 32.080483][ T1] __do_fast_syscall_32 (kbuild/src/x86_64-3/arch/x86/entry/common.c:112 kbuild/src/x86_64-3/arch/x86/entry/common.c:178) [ 32.080911][ T1] ? lockdep_hardirqs_on_prepare (kbuild/src/x86_64-3/kernel/locking/lockdep.c:4528) [ 32.081432][ T1] ? __do_fast_syscall_32 (kbuild/src/x86_64-3/arch/x86/entry/common.c:183) [ 32.081869][ T1] ? __do_fast_syscall_32 (kbuild/src/x86_64-3/arch/x86/entry/common.c:183) [ 32.082310][ T1] ? lockdep_hardirqs_on_prepare (kbuild/src/x86_64-3/kernel/locking/lockdep.c:4528) [ 32.082836][ T1] ? __do_fast_syscall_32 (kbuild/src/x86_64-3/arch/x86/entry/common.c:183) [ 32.083292][ T1] do_fast_syscall_32 (kbuild/src/x86_64-3/arch/x86/entry/common.c:203) [ 32.083709][ T1] entry_SYSENTER_compat_after_hwframe (kbuild/src/x86_64-3/arch/x86/entry/entry_64_compat.S:122) [ 32.084213][ T1] RIP: 0023:0xf7fb6549 [ 32.084574][ T1] Code: 03 74 c0 01 10 05 03 74 b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d b4 26 00 00 00 00 8d b4 26 00 00 00 00 All code ======== 0: 03 74 c0 01 add 0x1(%rax,%rax,8),%esi 4: 10 05 03 74 b8 01 adc %al,0x1b87403(%rip) # 0x1b8740d a: 10 06 adc %al,(%rsi) c: 03 74 b4 01 add 0x1(%rsp,%rsi,4),%esi 10: 10 07 adc %al,(%rdi) 12: 03 74 b0 01 add 0x1(%rax,%rsi,4),%esi 16: 10 08 adc %cl,(%rax) 18: 03 74 d8 01 add 0x1(%rax,%rbx,8),%esi 1c: 00 00 add %al,(%rax) 1e: 00 00 add %al,(%rax) 20: 00 51 52 add %dl,0x52(%rcx) 23: 55 push %rbp 24: 89 e5 mov %esp,%ebp 26: 0f 34 sysenter 28: cd 80 int $0x80 2a:* 5d pop %rbp <-- trapping instruction 2b: 5a pop %rdx 2c: 59 pop %rcx 2d: c3 retq 2e: 90 nop 2f: 90 nop 30: 90 nop 31: 90 nop 32: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi 39: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi Code starting with the faulting instruction =========================================== 0: 5d pop %rbp 1: 5a pop %rdx 2: 59 pop %rcx 3: c3 retq 4: 90 nop 5: 90 nop 6: 90 nop 7: 90 nop 8: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi f: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi [ 32.086028][ T1] RSP: 002b:00000000ffdde7dc EFLAGS: 00200206 ORIG_RAX: 0000000000000165 [ 32.086706][ T1] RAX: ffffffffffffffda RBX: 0000000000000005 RCX: 00000000ffdde818 [ 32.087375][ T1] RDX: 0000000000000070 RSI: 000000005805ebe0 RDI: 0000000000000000 [ 32.088030][ T1] RBP: 000000000000000f R08: 0000000000000000 R09: 0000000000000000 [ 32.088688][ T1] R10: 0000000000000000 R11: 0000000000200206 R12: 0000000000000000 [ 32.089341][ T1] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 [ 32.090006][ T1] </TASK> [ 32.090303][ T1] [ 32.090550][ T1] Allocated by task 1: [ 32.090908][ T1] kasan_save_stack (kbuild/src/x86_64-3/mm/kasan/common.c:46) [ 32.091316][ T1] kasan_set_track (kbuild/src/x86_64-3/mm/kasan/common.c:52) [ 32.091717][ T1] __kasan_krealloc (kbuild/src/x86_64-3/mm/kasan/common.c:440) [ 32.092131][ T1] krealloc (kbuild/src/x86_64-3/include/linux/kasan.h:231 kbuild/src/x86_64-3/mm/slab_common.c:1346 kbuild/src/x86_64-3/mm/slab_common.c:1383) [ 32.092490][ T1] is_state_visited (kbuild/src/x86_64-3/kernel/bpf/verifier.c:2505 kbuild/src/x86_64-3/kernel/bpf/verifier.c:11982) [ 32.092912][ T1] do_check (kbuild/src/x86_64-3/kernel/bpf/verifier.c:12176) [ 32.093285][ T1] do_check_common (kbuild/src/x86_64-3/kernel/bpf/verifier.c:14642) [ 32.093692][ T1] bpf_check (kbuild/src/x86_64-3/kernel/bpf/verifier.c:14705 kbuild/src/x86_64-3/kernel/bpf/verifier.c:15275) [ 32.094074][ T1] bpf_prog_load (kbuild/src/x86_64-3/kernel/bpf/syscall.c:2605) [ 32.094473][ T1] __sys_bpf (kbuild/src/x86_64-3/kernel/bpf/syscall.c:4965) [ 32.094851][ T1] __ia32_sys_bpf (kbuild/src/x86_64-3/kernel/bpf/syscall.c:5067) [ 32.095246][ T1] __do_fast_syscall_32 (kbuild/src/x86_64-3/arch/x86/entry/common.c:112 kbuild/src/x86_64-3/arch/x86/entry/common.c:178) [ 32.095674][ T1] do_fast_syscall_32 (kbuild/src/x86_64-3/arch/x86/entry/common.c:203) [ 32.096084][ T1] entry_SYSENTER_compat_after_hwframe (kbuild/src/x86_64-3/arch/x86/entry/entry_64_compat.S:122) [ 32.096593][ T1] [ 32.096840][ T1] The buggy address belongs to the object at ffff88814b9d75c0 [ 32.096840][ T1] which belongs to the cache kmalloc-32 of size 32 [ 32.097916][ T1] The buggy address is located 0 bytes inside of [ 32.097916][ T1] 32-byte region [ffff88814b9d75c0, ffff88814b9d75e0) [ 32.098895][ T1] [ 32.099143][ T1] The buggy address belongs to the physical page: [ 32.099656][ T1] page:00000000a3d33dcb refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x14b9d7 [ 32.100459][ T1] flags: 0x17ffffc0000200(slab|node=0|zone=2|lastcpupid=0x1fffff) [ 32.101113][ T1] raw: 0017ffffc0000200 0000000000000000 dead000000000122 ffff888100041500 [ 32.101813][ T1] raw: 0000000000000000 0000000080400040 00000001ffffffff 0000000000000000 [ 32.102518][ T1] page dumped because: kasan: bad access detected [ 32.103022][ T1] page_owner tracks the page as allocated [ 32.103496][ T1] page last allocated via order 0, migratetype Unmovable, gfp_mask 0x112cc0(GFP_USER|__GFP_NOWARN|__GFP_NORETRY), pid 1, tgid 1 (systemd), ts 32049485962, free_ts 0 [ 32.104752][ T1] get_page_from_freelist (kbuild/src/x86_64-3/mm/page_alloc.c:2549 kbuild/src/x86_64-3/mm/page_alloc.c:4287) [ 32.105197][ T1] __alloc_pages (kbuild/src/x86_64-3/mm/page_alloc.c:5547) [ 32.105592][ T1] allocate_slab (kbuild/src/x86_64-3/mm/slub.c:1794 kbuild/src/x86_64-3/mm/slub.c:1939) [ 32.105988][ T1] ___slab_alloc (kbuild/src/x86_64-3/mm/slub.c:3180) [ 32.106385][ T1] __kmem_cache_alloc_node (kbuild/src/x86_64-3/mm/slub.c:3279 kbuild/src/x86_64-3/mm/slub.c:3364 kbuild/src/x86_64-3/mm/slub.c:3437) [ 32.106832][ T1] __kmalloc_node_track_caller (kbuild/src/x86_64-3/include/linux/kasan.h:211 kbuild/src/x86_64-3/mm/slab_common.c:955 kbuild/src/x86_64-3/mm/slab_common.c:975) [ 32.107304][ T1] copy_array (kbuild/src/x86_64-3/arch/x86/kernel/cpu/hypervisor.c:93) [ 32.107672][ T1] copy_verifier_state (kbuild/src/x86_64-3/kernel/bpf/verifier.c:1189) [ 32.108094][ T1] push_stack (kbuild/src/x86_64-3/kernel/bpf/verifier.c:1288) [ 32.108466][ T1] check_cond_jmp_op (kbuild/src/x86_64-3/kernel/bpf/verifier.c:10223) [ 32.108884][ T1] do_check (kbuild/src/x86_64-3/kernel/bpf/verifier.c:12451) [ 32.109259][ T1] do_check_common (kbuild/src/x86_64-3/kernel/bpf/verifier.c:14642) [ 32.109663][ T1] bpf_check (kbuild/src/x86_64-3/kernel/bpf/verifier.c:14705 kbuild/src/x86_64-3/kernel/bpf/verifier.c:15275) [ 32.110044][ T1] bpf_prog_load (kbuild/src/x86_64-3/kernel/bpf/syscall.c:2605) [ 32.110442][ T1] __sys_bpf (kbuild/src/x86_64-3/kernel/bpf/syscall.c:4965) [ 32.110815][ T1] __ia32_sys_bpf (kbuild/src/x86_64-3/kernel/bpf/syscall.c:5067) To reproduce: # build kernel cd linux cp config-6.1.0-rc1-00216-gd916d97f3395 .config make HOSTCC=gcc-11 CC=gcc-11 ARCH=x86_64 olddefconfig prepare modules_prepare bzImage modules make HOSTCC=gcc-11 CC=gcc-11 ARCH=x86_64 INSTALL_MOD_PATH=<mod-install-dir> modules_install cd <mod-install-dir> find lib/ | cpio -o -H newc --quiet | gzip > modules.cgz git clone https://github.com/intel/lkp-tests.git cd lkp-tests bin/lkp qemu -k <bzImage> -m modules.cgz job-script # job-script is attached in this email # if come across any failure that blocks the test, # please remove ~/.lkp and /lkp dir to run from a clean state.
On 10/22/22 20:08, Kees Cook wrote: > With all "silently resizing" callers of ksize() refactored, remove the > logic in ksize() that would allow it to be used to effectively change > the size of an allocation (bypassing __alloc_size hints, etc). Users > wanting this feature need to either use kmalloc_size_roundup() before an > allocation, or use krealloc() directly. > > For kfree_sensitive(), move the unpoisoning logic inline. Replace the > some of the partially open-coded ksize() in __do_krealloc with ksize() > now that it doesn't perform unpoisoning. > > Adjust the KUnit tests to match the new ksize() behavior. > > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: Christoph Lameter <cl@linux.com> > Cc: Pekka Enberg <penberg@kernel.org> > Cc: David Rientjes <rientjes@google.com> > Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: Roman Gushchin <roman.gushchin@linux.dev> > Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com> > Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com> > Cc: Alexander Potapenko <glider@google.com> > Cc: Andrey Konovalov <andreyknvl@gmail.com> > Cc: Vincenzo Frascino <vincenzo.frascino@arm.com> > Cc: linux-mm@kvack.org > Cc: kasan-dev@googlegroups.com > Cc: netdev@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Acked-by: Vlastimil Babka <vbabka@suse.cz> > --- > This requires at least this be landed first: > https://lore.kernel.org/lkml/20221021234713.you.031-kees@kernel.org/ Don't we need all parts to have landed first, even if the skbuff one is the most prominent? > I suspect given that is the most central ksize() user, this ksize() > fix might be best to land through the netdev tree... > --- > mm/kasan/kasan_test.c | 8 +++++--- > mm/slab_common.c | 33 ++++++++++++++------------------- > 2 files changed, 19 insertions(+), 22 deletions(-) > > diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c > index 0d59098f0876..cb5c54adb503 100644 > --- a/mm/kasan/kasan_test.c > +++ b/mm/kasan/kasan_test.c > @@ -783,7 +783,7 @@ static void kasan_global_oob_left(struct kunit *test) > KUNIT_EXPECT_KASAN_FAIL(test, *(volatile char *)p); > } > > -/* Check that ksize() makes the whole object accessible. */ > +/* Check that ksize() does NOT unpoison whole object. */ > static void ksize_unpoisons_memory(struct kunit *test) > { > char *ptr; > @@ -791,15 +791,17 @@ static void ksize_unpoisons_memory(struct kunit *test) > > ptr = kmalloc(size, GFP_KERNEL); > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr); > + > real_size = ksize(ptr); > + KUNIT_EXPECT_GT(test, real_size, size); > > OPTIMIZER_HIDE_VAR(ptr); > > /* This access shouldn't trigger a KASAN report. */ > - ptr[size] = 'x'; > + ptr[size - 1] = 'x'; > > /* This one must. */ > - KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[real_size]); > + KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[real_size - 1]); > > kfree(ptr); > } > diff --git a/mm/slab_common.c b/mm/slab_common.c > index 33b1886b06eb..eabd66fcabd0 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -1333,11 +1333,11 @@ __do_krealloc(const void *p, size_t new_size, gfp_t flags) > void *ret; > size_t ks; > > - /* Don't use instrumented ksize to allow precise KASAN poisoning. */ > + /* Check for double-free before calling ksize. */ > if (likely(!ZERO_OR_NULL_PTR(p))) { > if (!kasan_check_byte(p)) > return NULL; > - ks = kfence_ksize(p) ?: __ksize(p); > + ks = ksize(p); > } else > ks = 0; > > @@ -1405,8 +1405,10 @@ void kfree_sensitive(const void *p) > void *mem = (void *)p; > > ks = ksize(mem); > - if (ks) > + if (ks) { > + kasan_unpoison_range(mem, ks); > memzero_explicit(mem, ks); > + } > kfree(mem); > } > EXPORT_SYMBOL(kfree_sensitive); > @@ -1415,10 +1417,11 @@ EXPORT_SYMBOL(kfree_sensitive); > * ksize - get the actual amount of memory allocated for a given object > * @objp: Pointer to the object > * > - * kmalloc may internally round up allocations and return more memory > + * kmalloc() may internally round up allocations and return more memory > * than requested. ksize() can be used to determine the actual amount of > - * memory allocated. The caller may use this additional memory, even though > - * a smaller amount of memory was initially specified with the kmalloc call. > + * allocated memory. The caller may NOT use this additional memory, unless > + * it calls krealloc(). To avoid an alloc/realloc cycle, callers can use > + * kmalloc_size_roundup() to find the size of the associated kmalloc bucket. > * The caller must guarantee that objp points to a valid object previously > * allocated with either kmalloc() or kmem_cache_alloc(). The object > * must not be freed during the duration of the call. > @@ -1427,13 +1430,11 @@ EXPORT_SYMBOL(kfree_sensitive); > */ > size_t ksize(const void *objp) > { > - size_t size; > - > /* > - * We need to first check that the pointer to the object is valid, and > - * only then unpoison the memory. The report printed from ksize() is > - * more useful, then when it's printed later when the behaviour could > - * be undefined due to a potential use-after-free or double-free. > + * We need to first check that the pointer to the object is valid. > + * The KASAN report printed from ksize() is more useful, then when > + * it's printed later when the behaviour could be undefined due to > + * a potential use-after-free or double-free. > * > * We use kasan_check_byte(), which is supported for the hardware > * tag-based KASAN mode, unlike kasan_check_read/write(). > @@ -1447,13 +1448,7 @@ size_t ksize(const void *objp) > if (unlikely(ZERO_OR_NULL_PTR(objp)) || !kasan_check_byte(objp)) > return 0; > > - size = kfence_ksize(objp) ?: __ksize(objp); > - /* > - * We assume that ksize callers could use whole allocated area, > - * so we need to unpoison this area. > - */ > - kasan_unpoison_range(objp, size); > - return size; > + return kfence_ksize(objp) ?: __ksize(objp); > } > EXPORT_SYMBOL(ksize); >
On Tue, Oct 25, 2022 at 01:53:54PM +0200, Vlastimil Babka wrote: > On 10/22/22 20:08, Kees Cook wrote: > > With all "silently resizing" callers of ksize() refactored, remove the > > logic in ksize() that would allow it to be used to effectively change > > the size of an allocation (bypassing __alloc_size hints, etc). Users > > wanting this feature need to either use kmalloc_size_roundup() before an > > allocation, or use krealloc() directly. > > > > For kfree_sensitive(), move the unpoisoning logic inline. Replace the > > some of the partially open-coded ksize() in __do_krealloc with ksize() > > now that it doesn't perform unpoisoning. > > > > [...] > > Signed-off-by: Kees Cook <keescook@chromium.org> > > Acked-by: Vlastimil Babka <vbabka@suse.cz> Thanks! > > --- > > This requires at least this be landed first: > > https://lore.kernel.org/lkml/20221021234713.you.031-kees@kernel.org/ > > Don't we need all parts to have landed first, even if the skbuff one is the > most prominent? Yes, though, I suspect there will be some cases we couldn't easily find. Here are the prerequisites I'm aware of: in -next: 36875a063b5e ("net: ipa: Proactively round up to kmalloc bucket size") ab3f7828c979 ("openvswitch: Use kmalloc_size_roundup() to match ksize() usage") d6dd508080a3 ("bnx2: Use kmalloc_size_roundup() to match ksize() usage") reviewed, waiting to land (should I take these myself?) btrfs: send: Proactively round up to kmalloc bucket size https://lore.kernel.org/lkml/20220923202822.2667581-8-keescook@chromium.org/ dma-buf: Proactively round up to kmalloc bucket size https://lore.kernel.org/lkml/20221018090858.never.941-kees@kernel.org/ partially reviewed: igb: Proactively round up to kmalloc bucket size https://lore.kernel.org/lkml/20221018092340.never.556-kees@kernel.org/ unreviewed: coredump: Proactively round up to kmalloc bucket size https://lore.kernel.org/lkml/20221018090701.never.996-kees@kernel.org/ devres: Use kmalloc_size_roundup() to match ksize() usage https://lore.kernel.org/lkml/20221018090406.never.856-kees@kernel.org/ needs updating: mempool: Use kmalloc_size_roundup() to match ksize() usage https://lore.kernel.org/lkml/20221018090323.never.897-kees@kernel.org/ bpf: Use kmalloc_size_roundup() to match ksize() usage https://lore.kernel.org/lkml/20221018090550.never.834-kees@kernel.org/
On Sat, Oct 22, 2022 at 8:08 PM Kees Cook <keescook@chromium.org> wrote: > > With all "silently resizing" callers of ksize() refactored, remove the > logic in ksize() that would allow it to be used to effectively change > the size of an allocation (bypassing __alloc_size hints, etc). Users > wanting this feature need to either use kmalloc_size_roundup() before an > allocation, or use krealloc() directly. > > For kfree_sensitive(), move the unpoisoning logic inline. Replace the > some of the partially open-coded ksize() in __do_krealloc with ksize() > now that it doesn't perform unpoisoning. > > Adjust the KUnit tests to match the new ksize() behavior. Hi Kees, > -/* Check that ksize() makes the whole object accessible. */ > +/* Check that ksize() does NOT unpoison whole object. */ > static void ksize_unpoisons_memory(struct kunit *test) > { > char *ptr; > @@ -791,15 +791,17 @@ static void ksize_unpoisons_memory(struct kunit *test) > > ptr = kmalloc(size, GFP_KERNEL); > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr); > + > real_size = ksize(ptr); > + KUNIT_EXPECT_GT(test, real_size, size); > > OPTIMIZER_HIDE_VAR(ptr); > > /* This access shouldn't trigger a KASAN report. */ > - ptr[size] = 'x'; > + ptr[size - 1] = 'x'; > > /* This one must. */ > - KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[real_size]); > + KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[real_size - 1]); How about also accessing ptr[size] here? It would allow for a more precise checking of the in-object redzone. > > kfree(ptr); > } Thanks!
On Thu, Oct 27, 2022 at 09:05:45PM +0200, Andrey Konovalov wrote: > On Sat, Oct 22, 2022 at 8:08 PM Kees Cook <keescook@chromium.org> wrote: > [...] > > -/* Check that ksize() makes the whole object accessible. */ > > +/* Check that ksize() does NOT unpoison whole object. */ > > static void ksize_unpoisons_memory(struct kunit *test) > > { > > char *ptr; > > @@ -791,15 +791,17 @@ static void ksize_unpoisons_memory(struct kunit *test) > > > > ptr = kmalloc(size, GFP_KERNEL); > > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr); > > + > > real_size = ksize(ptr); > > + KUNIT_EXPECT_GT(test, real_size, size); > > > > OPTIMIZER_HIDE_VAR(ptr); > > > > /* This access shouldn't trigger a KASAN report. */ > > - ptr[size] = 'x'; > > + ptr[size - 1] = 'x'; > > > > /* This one must. */ > > - KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[real_size]); > > + KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[real_size - 1]); > > How about also accessing ptr[size] here? It would allow for a more > precise checking of the in-object redzone. Sure! Probably both ptr[size] and ptr[real_size -1], yes?
On Thu, Oct 27, 2022 at 9:13 PM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Oct 27, 2022 at 09:05:45PM +0200, Andrey Konovalov wrote: > > On Sat, Oct 22, 2022 at 8:08 PM Kees Cook <keescook@chromium.org> wrote: > > [...] > > > -/* Check that ksize() makes the whole object accessible. */ > > > +/* Check that ksize() does NOT unpoison whole object. */ > > > static void ksize_unpoisons_memory(struct kunit *test) > > > { > > > char *ptr; > > > @@ -791,15 +791,17 @@ static void ksize_unpoisons_memory(struct kunit *test) > > > > > > ptr = kmalloc(size, GFP_KERNEL); > > > KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr); > > > + > > > real_size = ksize(ptr); > > > + KUNIT_EXPECT_GT(test, real_size, size); > > > > > > OPTIMIZER_HIDE_VAR(ptr); > > > > > > /* This access shouldn't trigger a KASAN report. */ > > > - ptr[size] = 'x'; > > > + ptr[size - 1] = 'x'; > > > > > > /* This one must. */ > > > - KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[real_size]); > > > + KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[real_size - 1]); > > > > How about also accessing ptr[size] here? It would allow for a more > > precise checking of the in-object redzone. > > Sure! Probably both ptr[size] and ptr[real_size -1], yes? Yes, sounds good. Thank you!
diff --git a/mm/kasan/kasan_test.c b/mm/kasan/kasan_test.c index 0d59098f0876..cb5c54adb503 100644 --- a/mm/kasan/kasan_test.c +++ b/mm/kasan/kasan_test.c @@ -783,7 +783,7 @@ static void kasan_global_oob_left(struct kunit *test) KUNIT_EXPECT_KASAN_FAIL(test, *(volatile char *)p); } -/* Check that ksize() makes the whole object accessible. */ +/* Check that ksize() does NOT unpoison whole object. */ static void ksize_unpoisons_memory(struct kunit *test) { char *ptr; @@ -791,15 +791,17 @@ static void ksize_unpoisons_memory(struct kunit *test) ptr = kmalloc(size, GFP_KERNEL); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ptr); + real_size = ksize(ptr); + KUNIT_EXPECT_GT(test, real_size, size); OPTIMIZER_HIDE_VAR(ptr); /* This access shouldn't trigger a KASAN report. */ - ptr[size] = 'x'; + ptr[size - 1] = 'x'; /* This one must. */ - KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[real_size]); + KUNIT_EXPECT_KASAN_FAIL(test, ((volatile char *)ptr)[real_size - 1]); kfree(ptr); } diff --git a/mm/slab_common.c b/mm/slab_common.c index 33b1886b06eb..eabd66fcabd0 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -1333,11 +1333,11 @@ __do_krealloc(const void *p, size_t new_size, gfp_t flags) void *ret; size_t ks; - /* Don't use instrumented ksize to allow precise KASAN poisoning. */ + /* Check for double-free before calling ksize. */ if (likely(!ZERO_OR_NULL_PTR(p))) { if (!kasan_check_byte(p)) return NULL; - ks = kfence_ksize(p) ?: __ksize(p); + ks = ksize(p); } else ks = 0; @@ -1405,8 +1405,10 @@ void kfree_sensitive(const void *p) void *mem = (void *)p; ks = ksize(mem); - if (ks) + if (ks) { + kasan_unpoison_range(mem, ks); memzero_explicit(mem, ks); + } kfree(mem); } EXPORT_SYMBOL(kfree_sensitive); @@ -1415,10 +1417,11 @@ EXPORT_SYMBOL(kfree_sensitive); * ksize - get the actual amount of memory allocated for a given object * @objp: Pointer to the object * - * kmalloc may internally round up allocations and return more memory + * kmalloc() may internally round up allocations and return more memory * than requested. ksize() can be used to determine the actual amount of - * memory allocated. The caller may use this additional memory, even though - * a smaller amount of memory was initially specified with the kmalloc call. + * allocated memory. The caller may NOT use this additional memory, unless + * it calls krealloc(). To avoid an alloc/realloc cycle, callers can use + * kmalloc_size_roundup() to find the size of the associated kmalloc bucket. * The caller must guarantee that objp points to a valid object previously * allocated with either kmalloc() or kmem_cache_alloc(). The object * must not be freed during the duration of the call. @@ -1427,13 +1430,11 @@ EXPORT_SYMBOL(kfree_sensitive); */ size_t ksize(const void *objp) { - size_t size; - /* - * We need to first check that the pointer to the object is valid, and - * only then unpoison the memory. The report printed from ksize() is - * more useful, then when it's printed later when the behaviour could - * be undefined due to a potential use-after-free or double-free. + * We need to first check that the pointer to the object is valid. + * The KASAN report printed from ksize() is more useful, then when + * it's printed later when the behaviour could be undefined due to + * a potential use-after-free or double-free. * * We use kasan_check_byte(), which is supported for the hardware * tag-based KASAN mode, unlike kasan_check_read/write(). @@ -1447,13 +1448,7 @@ size_t ksize(const void *objp) if (unlikely(ZERO_OR_NULL_PTR(objp)) || !kasan_check_byte(objp)) return 0; - size = kfence_ksize(objp) ?: __ksize(objp); - /* - * We assume that ksize callers could use whole allocated area, - * so we need to unpoison this area. - */ - kasan_unpoison_range(objp, size); - return size; + return kfence_ksize(objp) ?: __ksize(objp); } EXPORT_SYMBOL(ksize);
With all "silently resizing" callers of ksize() refactored, remove the logic in ksize() that would allow it to be used to effectively change the size of an allocation (bypassing __alloc_size hints, etc). Users wanting this feature need to either use kmalloc_size_roundup() before an allocation, or use krealloc() directly. For kfree_sensitive(), move the unpoisoning logic inline. Replace the some of the partially open-coded ksize() in __do_krealloc with ksize() now that it doesn't perform unpoisoning. Adjust the KUnit tests to match the new ksize() behavior. Cc: Dmitry Vyukov <dvyukov@google.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Christoph Lameter <cl@linux.com> Cc: Pekka Enberg <penberg@kernel.org> Cc: David Rientjes <rientjes@google.com> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Roman Gushchin <roman.gushchin@linux.dev> Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com> Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com> Cc: Alexander Potapenko <glider@google.com> Cc: Andrey Konovalov <andreyknvl@gmail.com> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com> Cc: linux-mm@kvack.org Cc: kasan-dev@googlegroups.com Cc: netdev@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- This requires at least this be landed first: https://lore.kernel.org/lkml/20221021234713.you.031-kees@kernel.org/ I suspect given that is the most central ksize() user, this ksize() fix might be best to land through the netdev tree... --- mm/kasan/kasan_test.c | 8 +++++--- mm/slab_common.c | 33 ++++++++++++++------------------- 2 files changed, 19 insertions(+), 22 deletions(-)