1. Patchew
  2. linux
  3. View series

[PATCH] dcache: fully initialize the inline name in __d_alloc()

Drif Abdelmalek Mohamed Said posted 1 patch 1 week, 4 days ago
There is a newer version of this series
fs/dcache.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
[PATCH] dcache: fully initialize the inline name in __d_alloc()
Posted by Drif Abdelmalek Mohamed Said 1 week, 4 days ago 
syzbot reported:

    BUG: KMSAN: uninit-value in dentry_string_cmp fs/dcache.c:291 [inline]
    BUG: KMSAN: uninit-value in dentry_cmp fs/dcache.c:322 [inline]
    BUG: KMSAN: uninit-value in __d_lookup_rcu+0x37d/0x5e0 fs/dcache.c:2522

     dentry_string_cmp fs/dcache.c:291 [inline]
     dentry_cmp fs/dcache.c:322 [inline]
     __d_lookup_rcu+0x37d/0x5e0 fs/dcache.c:2522
     lookup_fast+0x194/0xa40 fs/namei.c:1854
     lookup_fast_for_open fs/namei.c:4545 [inline]
     open_last_lookups fs/namei.c:4579 [inline]
     path_openat+0x9ef/0x6540 fs/namei.c:4856
     do_file_open+0x2aa/0x680 fs/namei.c:4888
     do_sys_openat2+0x17c/0x390 fs/open.c:1395
     do_sys_open fs/open.c:1401 [inline]
     __do_sys_openat fs/open.c:1417 [inline]
     __se_sys_openat fs/open.c:1412 [inline]
     __x64_sys_openat+0x240/0x300 fs/open.c:1412
     x64_sys_call+0x2445/0x3ea0 arch/x86/include/generated/asm/syscalls_64.h:258
     do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
     do_syscall_64+0x15d/0x3c0 arch/x86/entry/syscall_64.c:94
     entry_SYSCALL_64_after_hwframe+0x77/0x7f

Uninit was stored to memory at:
     copy_name fs/dcache.c:3031 [inline]
     __d_move+0xd29/0x21f0 fs/dcache.c:3099
     d_move+0x71/0xf0 fs/dcache.c:3147
     vfs_rename+0x2619/0x2770 fs/namei.c:6085
     filename_renameat2+0xa59/0x1230 fs/namei.c:6188
     __do_sys_rename fs/namei.c:6232 [inline]
     __se_sys_rename+0xc5/0x5c0 fs/namei.c:6228
     __x64_sys_rename+0x78/0xb0 fs/namei.c:6228
     x64_sys_call+0x329/0x3ea0 arch/x86/include/generated/asm/syscalls_64.h:83
     do_syscall_x64 arch/x86/entry/syscall_64.c:63
     do_syscall_64+0x15d/0x3c0 arch/x86/entry/syscall_64.c:94
     entry_SYSCALL_64_after_hwframe+0x77/0x7f

Uninit was created at:
     slab_post_alloc_hook mm/slub.c:4617 [inline]
     slab_alloc_node mm/slub.c:4939 [inline]
     kmem_cache_alloc_lru_noprof+0x376/0x1230 mm/s
     __d_alloc+0x52/0x9f0 fs/dcache.c:1902
     d_alloc+0x57/0x300 fs/dcache.c:1981
     lookup_one_qstr_excl+0x19d/0x7a0 fs/namei.c:1806
     __start_renaming+0x341/0x850 fs/namei.c:3888
     filename_renameat2+0x625/0x1230 fs/namei.c:6163
     __do_sys_rename fs/namei.c:6232 [inline]
     __se_sys_rename+0xc5/0x5c0 fs/namei.c:6228
     __x64_sys_rename+0x78/0xb0 fs/namei.c:6228
     x64_sys_call+0x329/0x3ea0 arch/x86/include/generated/asm/syscalls_64.h:83
     do_syscall_x64 arch/x86/entry/syscall_64.c:63
     do_syscall_64+0x15d/0x3c0 arch/x86/entry/syscall_64.c:94
     entry_SYSCALL_64_after_hwframe+0x77/0x7f

The race happens between a concurrent open() and rename() of the same
path.  __d_alloc() initializes only the name itsel
of the inline buffer; the tail in between is left uninitialized.
copy_name(), called from rename(), copies the enti
including that uninitialized tail - into the moved dentry.  Meanwhile
__d_lookup_rcu(), called from open(), is an optimi
it checks d_name.hash_len first and leaves the seqcount retry to its
caller, so a lookup of the old (longer) name racin
compares with a stale length.  While copy_name() is rewriting the name
in place, the walker can transiently step past the
terminating NUL and read bytes from the uninitialized tail, which KMSAN
reports.

The race is benign by design - the read stays in bounds and the lookup
result is discarded by the seqcount retry - but th
is real.  Fix it by zeroing the entire inline buffer in __d_alloc(),
so every byte a racy walker can touch is defined;
past a mid-rewrite name now simply stops at a NUL.

Reported-by: syzbot+7ff3adde89dd795ad4c4@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=7ff3adde89dd795ad4c4
Signed-off-by: Drif Abdelmalek Mohamed Said <drifabdelmalekmohamedsaid@gmail.com>
---
 fs/dcache.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 1b1a81f10da6..2de1bc76dc8e 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1910,12 +1910,17 @@ static struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
 		return NULL;
 
 	/*
-	 * We guarantee that the inline name is always NUL-terminated.
-	 * This way the memcpy() done by the name switching in rename
-	 * will still always have a NUL at the end, even if we might
-	 * be overwriting an internal NUL character
+	 * Fully initialize the inline name buffer.  copy_name() and
+	 * swap_names() copy d_shortname in its entirety, so any
+	 * uninitialized tail would propagate to the other dentry, and
+	 * __d_lookup_rcu() may transiently read any byte of the inline
+	 * name while rename() rewrites it in place.
+	 *
+	 * This also keeps the inline name NUL-terminated: the name
+	 * switching in rename will still always have a NUL at the end,
+	 * even if we might be overwriting an internal NUL character.
 	 */
-	dentry->d_shortname.string[DNAME_INLINE_LEN-1] = 0;
+	memcpy(dentry->d_shortname.string, 0, DNAME_INLINE_LEN);
 	if (unlikely(!name)) {
 		name = &slash_name;
 		dname = dentry->d_shortname.string;
-- 
2.43.0
[syzbot ci] Re: dcache: fully initialize the inline name in __d_alloc()
Posted by syzbot ci 1 week, 4 days ago 
syzbot ci has tested the following series

[v1] dcache: fully initialize the inline name in __d_alloc()
https://lore.kernel.org/all/20260913152802.13413-1-drifabdelmalekmohamedsaid@gmail.com
* [PATCH] dcache: fully initialize the inline name in __d_alloc()

and found the following issue:
KASAN: null-ptr-deref Read in __d_alloc

Full report is available here:
https://ci.syzbot.org/series/5b221ae6-70dc-4b99-963a-3d9e7db97db0

***

KASAN: null-ptr-deref Read in __d_alloc

tree:      vfs
URL:       https://kernel.googlesource.com/pub/scm/linux/kernel/git/vfs/vfs.git
base:      f5d607b8091438d8416d0e3ae02532a18539b48b
arch:      amd64
compiler:  Debian clang version 22.1.8 (++20260613092233+e80beda6e255-1~exp1~20260613092250.77), Debian LLD 22.1.8
config:    https://ci.syzbot.org/builds/9b8e9369-7fc0-4fbd-a090-6f785f103d1b/config

kfence: initialized - using 2097152 bytes for 255 objects at 0xffff88823c400000-0xffff88823c600000
Console: colour VGA+ 80x25
printk: console [ttyS0] enabled
printk: console [ttyS0] enabled
printk: legacy bootconsole [earlyser0] disabled
printk: legacy bootconsole [earlyser0] disabled
Lock dependency validator: Copyright (c) 2006 Red Hat, Inc., Ingo Molnar
... MAX_LOCKDEP_SUBCLASSES:  8
... MAX_LOCK_DEPTH:          48
... MAX_LOCKDEP_KEYS:        8192
... CLASSHASH_SIZE:          4096
... MAX_LOCKDEP_ENTRIES:     1048576
... MAX_LOCKDEP_CHAINS:      1048576
... CHAINHASH_SIZE:          524288
 memory used by lock dependency info: 106625 kB
 memory used for stack traces: 8320 kB
 per task-struct memory footprint: 1920 bytes
mempolicy: Enabling automatic NUMA balancing. Configure with numa_balancing= or the kernel.numa_balancing sysctl
ACPI: Core revision 20260408
clocksource: hpet: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604467 ns
APIC: Switch to symmetric I/O mode setup
x2apic enabled
APIC: Switched APIC routing to: physical x2apic
..TIMER: vector=0x30 apic1=0 pin1=2 apic2=-1 pin2=-1
clocksource: tsc-early: mask: 0xffffffffffffffff max_cycles: 0x285c3ee517e, max_idle_ns: 440795257231 ns
Calibrating delay loop (skipped) preset value.. 5599.99 BogoMIPS (lpj=27999980)
Last level iTLB entries: 4KB 0, 2MB 0, 4MB 0
Last level dTLB entries: 4KB 0, 2MB 0, 4MB 0, 1GB 0
mitigations: Enabled attack vectors: user_kernel, user_user, guest_host, guest_guest, SMT mitigations: auto
Speculative Store Bypass: Vulnerable
Spectre V2 : Mitigation: Retpolines
ITS: Mitigation: Aligned branch/return thunks
MDS: Vulnerable: Clear CPU buffers attempted, no microcode
Spectre V1 : Mitigation: usercopy/swapgs barriers and __user pointer sanitization
Spectre V2 : Spectre v2 / SpectreRSB: Filling RSB on context switch and VMEXIT
active return thunk: its_return_thunk
x86/fpu: x87 FPU will use FXSAVE
pid_max: default: 32768 minimum: 301
landlock: Up and running.
Yama: becoming mindful.
TOMOYO Linux initialized
AppArmor: AppArmor initialized
LSM support for eBPF active
debugfs: Unable to create file 'net_refcnt@ffffffff9aedda40', debugfs is not initialized yet
debugfs: Unable to create file 'net_notrefcnt@ffffffff9aeddab8', debugfs is not initialized yet
Dentry cache hash table entries: 1048576 (order: 11, 8388608 bytes, vmalloc hugepage)
Inode-cache hash table entries: 524288 (order: 10, 4194304 bytes, vmalloc hugepage)
Mount-cache hash table entries: 16384 (order: 5, 131072 bytes, vmalloc)
Mountpoint-cache hash table entries: 16384 (order: 5, 131072 bytes, vmalloc)
==================================================================
BUG: KASAN: null-ptr-deref in __d_alloc+0x65/0x7b0
Read of size 40 at addr 0000000000000000 by task swapper/0/0

CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted syzkaller #0 PREEMPT(full) 
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
Call Trace:
 <TASK>
 dump_stack_lvl+0xe8/0x150
 kasan_report+0x117/0x150
 kasan_check_range+0x264/0x2c0
 __asan_memcpy+0x29/0x70
 __d_alloc+0x65/0x7b0
 d_make_root+0x41/0x80
 shmem_fill_super+0xc07/0x1090
 get_tree_nodev+0xbb/0x150
 vfs_get_tree+0x92/0x2a0
 vfs_kern_mount+0x15b/0x220
 kern_mount+0x43/0x90
 shmem_init+0x37/0x170
 mnt_init+0x19b/0x1f0
 vfs_caches_init+0x22/0x30
 start_kernel+0x34c/0x3e0
 x86_64_start_reservations+0x24/0x30
 x86_64_start_kernel+0x137/0x1b0
 common_startup_64+0x13e/0x157
 </TASK>
==================================================================


***

If these findings have caused you to resend the series or submit a
separate fix, please add the following tag to your commit message:
  Tested-by: syzbot@syzkaller.appspotmail.com

---
This report is generated by a bot. It may contain errors.
syzbot ci engineers can be reached at syzkaller@googlegroups.com.

To test a fix for this bug, please reply with `#syz test`
(on a separate line) and attach the patch to the email.

Notes:
- The patch will be applied on top of the tested series (as an
  incremental fix).
- To test a new version of the whole series, please send it directly
  to syzbot@lists.linux.dev.
- Arguments like custom git repos and branches are not supported.
[PATCH v2] dcache: fully initialize the inline name in __d_alloc()
Posted by Drif Abdelmalek Mohamed Said 1 week, 3 days ago 
syzbot reported:

    BUG: KMSAN: uninit-value in dentry_string_cmp fs/dcache.c:291 [inline]
    BUG: KMSAN: uninit-value in dentry_cmp fs/dcache.c:322 [inline]
    BUG: KMSAN: uninit-value in __d_lookup_rcu+0x37d/0x5e0 fs/dcache.c:2522

     dentry_string_cmp fs/dcache.c:291 [inline]
     dentry_cmp fs/dcache.c:322 [inline]
     __d_lookup_rcu+0x37d/0x5e0 fs/dcache.c:2522
     lookup_fast+0x194/0xa40 fs/namei.c:1854
     lookup_fast_for_open fs/namei.c:4545 [inline]
     open_last_lookups fs/namei.c:4579 [inline]
     path_openat+0x9ef/0x6540 fs/namei.c:4856
     do_file_open+0x2aa/0x680 fs/namei.c:4888
     do_sys_openat2+0x17c/0x390 fs/open.c:1395
     do_sys_open fs/open.c:1401 [inline]
     __do_sys_openat fs/open.c:1417 [inline]
     __se_sys_openat fs/open.c:1412 [inline]
     __x64_sys_openat+0x240/0x300 fs/open.c:1412
     x64_sys_call+0x2445/0x3ea0 arch/x86/include/generated/asm/syscalls_64.h:258
     do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
     do_syscall_64+0x15d/0x3c0 arch/x86/entry/syscall_64.c:94
     entry_SYSCALL_64_after_hwframe+0x77/0x7f

Uninit was stored to memory at:
     copy_name fs/dcache.c:3031 [inline]
     __d_move+0xd29/0x21f0 fs/dcache.c:3099
     d_move+0x71/0xf0 fs/dcache.c:3147
     vfs_rename+0x2619/0x2770 fs/namei.c:6085
     filename_renameat2+0xa59/0x1230 fs/namei.c:6188
     __do_sys_rename fs/namei.c:6232 [inline]
     __se_sys_rename+0xc5/0x5c0 fs/namei.c:6228
     __x64_sys_rename+0x78/0xb0 fs/namei.c:6228
     x64_sys_call+0x329/0x3ea0 arch/x86/include/generated/asm/syscalls_64.h:83
     do_syscall_x64 arch/x86/entry/syscall_64.c:63
     do_syscall_64+0x15d/0x3c0 arch/x86/entry/syscall_64.c:94
     entry_SYSCALL_64_after_hwframe+0x77/0x7f

Uninit was created at:
     slab_post_alloc_hook mm/slub.c:4617 [inline]
     slab_alloc_node mm/slub.c:4939 [inline]
     kmem_cache_alloc_lru_noprof+0x376/0x1230 mm/s
     __d_alloc+0x52/0x9f0 fs/dcache.c:1902
     d_alloc+0x57/0x300 fs/dcache.c:1981
     lookup_one_qstr_excl+0x19d/0x7a0 fs/namei.c:1806
     __start_renaming+0x341/0x850 fs/namei.c:3888
     filename_renameat2+0x625/0x1230 fs/namei.c:6163
     __do_sys_rename fs/namei.c:6232 [inline]
     __se_sys_rename+0xc5/0x5c0 fs/namei.c:6228
     __x64_sys_rename+0x78/0xb0 fs/namei.c:6228
     x64_sys_call+0x329/0x3ea0 arch/x86/include/generated/asm/syscalls_64.h:83
     do_syscall_x64 arch/x86/entry/syscall_64.c:63
     do_syscall_64+0x15d/0x3c0 arch/x86/entry/syscall_64.c:94
     entry_SYSCALL_64_after_hwframe+0x77/0x7f

The race happens between a concurrent open() and rename() of the same
path.  __d_alloc() initializes only the name itsel
of the inline buffer; the tail in between is left uninitialized.
copy_name(), called from rename(), copies the enti
including that uninitialized tail - into the moved dentry.  Meanwhile
__d_lookup_rcu(), called from open(), is an optimi
it checks d_name.hash_len first and leaves the seqcount retry to its
caller, so a lookup of the old (longer) name racin
compares with a stale length.  While copy_name() is rewriting the name
in place, the walker can transiently step past the
terminating NUL and read bytes from the uninitialized tail, which KMSAN
reports.

The race is benign by design - the read stays in bounds and the lookup
result is discarded by the seqcount retry - but th
is real.  Fix it by zeroing the entire inline buffer in __d_alloc(),
so every byte a racy walker can touch is defined;
past a mid-rewrite name now simply stops at a NUL.

Reported-by: syzbot+7ff3adde89dd795ad4c4@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=7ff3adde89dd795ad4c4
Signed-off-by: Drif Abdelmalek Mohamed Said <drifabdelmalekmohamedsaid@gmail.com>
---
v2:
  - Screwed up by using memcpy in v1, fixed it here by using memset, only change
 fs/dcache.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 1b1a81f10da6..730571e92af8 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1910,12 +1910,17 @@ static struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
 		return NULL;
 
 	/*
-	 * We guarantee that the inline name is always NUL-terminated.
-	 * This way the memcpy() done by the name switching in rename
-	 * will still always have a NUL at the end, even if we might
-	 * be overwriting an internal NUL character
+	 * Fully initialize the inline name buffer.  copy_name() and
+	 * swap_names() copy d_shortname in its entirety, so any
+	 * uninitialized tail would propagate to the other dentry, and
+	 * __d_lookup_rcu() may transiently read any byte of the inline
+	 * name while rename() rewrites it in place.
+	 *
+	 * This also keeps the inline name NUL-terminated: the name
+	 * switching in rename will still always have a NUL at the end,
+	 * even if we might be overwriting an internal NUL character.
 	 */
-	dentry->d_shortname.string[DNAME_INLINE_LEN-1] = 0;
+	memset(dentry->d_shortname.string, 0, DNAME_INLINE_LEN);
 	if (unlikely(!name)) {
 		name = &slash_name;
 		dname = dentry->d_shortname.string;
-- 
2.43.0
Re: [PATCH v2] dcache: fully initialize the inline name in __d_alloc()
Posted by Jan Kara 1 week, 2 days ago 
On Mon 14-09-26 20:29:26, Drif Abdelmalek Mohamed Said wrote:
> syzbot reported:
> 
>     BUG: KMSAN: uninit-value in dentry_string_cmp fs/dcache.c:291 [inline]
>     BUG: KMSAN: uninit-value in dentry_cmp fs/dcache.c:322 [inline]
>     BUG: KMSAN: uninit-value in __d_lookup_rcu+0x37d/0x5e0 fs/dcache.c:2522
> 
>      dentry_string_cmp fs/dcache.c:291 [inline]
>      dentry_cmp fs/dcache.c:322 [inline]
>      __d_lookup_rcu+0x37d/0x5e0 fs/dcache.c:2522
>      lookup_fast+0x194/0xa40 fs/namei.c:1854
>      lookup_fast_for_open fs/namei.c:4545 [inline]
>      open_last_lookups fs/namei.c:4579 [inline]
>      path_openat+0x9ef/0x6540 fs/namei.c:4856
>      do_file_open+0x2aa/0x680 fs/namei.c:4888
>      do_sys_openat2+0x17c/0x390 fs/open.c:1395
>      do_sys_open fs/open.c:1401 [inline]
>      __do_sys_openat fs/open.c:1417 [inline]
>      __se_sys_openat fs/open.c:1412 [inline]
>      __x64_sys_openat+0x240/0x300 fs/open.c:1412
>      x64_sys_call+0x2445/0x3ea0 arch/x86/include/generated/asm/syscalls_64.h:258
>      do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
>      do_syscall_64+0x15d/0x3c0 arch/x86/entry/syscall_64.c:94
>      entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> Uninit was stored to memory at:
>      copy_name fs/dcache.c:3031 [inline]
>      __d_move+0xd29/0x21f0 fs/dcache.c:3099
>      d_move+0x71/0xf0 fs/dcache.c:3147
>      vfs_rename+0x2619/0x2770 fs/namei.c:6085
>      filename_renameat2+0xa59/0x1230 fs/namei.c:6188
>      __do_sys_rename fs/namei.c:6232 [inline]
>      __se_sys_rename+0xc5/0x5c0 fs/namei.c:6228
>      __x64_sys_rename+0x78/0xb0 fs/namei.c:6228
>      x64_sys_call+0x329/0x3ea0 arch/x86/include/generated/asm/syscalls_64.h:83
>      do_syscall_x64 arch/x86/entry/syscall_64.c:63
>      do_syscall_64+0x15d/0x3c0 arch/x86/entry/syscall_64.c:94
>      entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> Uninit was created at:
>      slab_post_alloc_hook mm/slub.c:4617 [inline]
>      slab_alloc_node mm/slub.c:4939 [inline]
>      kmem_cache_alloc_lru_noprof+0x376/0x1230 mm/s
>      __d_alloc+0x52/0x9f0 fs/dcache.c:1902
>      d_alloc+0x57/0x300 fs/dcache.c:1981
>      lookup_one_qstr_excl+0x19d/0x7a0 fs/namei.c:1806
>      __start_renaming+0x341/0x850 fs/namei.c:3888
>      filename_renameat2+0x625/0x1230 fs/namei.c:6163
>      __do_sys_rename fs/namei.c:6232 [inline]
>      __se_sys_rename+0xc5/0x5c0 fs/namei.c:6228
>      __x64_sys_rename+0x78/0xb0 fs/namei.c:6228
>      x64_sys_call+0x329/0x3ea0 arch/x86/include/generated/asm/syscalls_64.h:83
>      do_syscall_x64 arch/x86/entry/syscall_64.c:63
>      do_syscall_64+0x15d/0x3c0 arch/x86/entry/syscall_64.c:94
>      entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> The race happens between a concurrent open() and rename() of the same
> path.  __d_alloc() initializes only the name itsel
> of the inline buffer; the tail in between is left uninitialized.
> copy_name(), called from rename(), copies the enti
> including that uninitialized tail - into the moved dentry.  Meanwhile
> __d_lookup_rcu(), called from open(), is an optimi
> it checks d_name.hash_len first and leaves the seqcount retry to its
> caller, so a lookup of the old (longer) name racin
> compares with a stale length.  While copy_name() is rewriting the name
> in place, the walker can transiently step past the
> terminating NUL and read bytes from the uninitialized tail, which KMSAN
> reports.
> 
> The race is benign by design - the read stays in bounds and the lookup
> result is discarded by the seqcount retry - but th
> is real.  Fix it by zeroing the entire inline buffer in __d_alloc(),
> so every byte a racy walker can touch is defined;
> past a mid-rewrite name now simply stops at a NUL.
> 
> Reported-by: syzbot+7ff3adde89dd795ad4c4@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7ff3adde89dd795ad4c4
> Signed-off-by: Drif Abdelmalek Mohamed Said <drifabdelmalekmohamedsaid@gmail.com>

Firstly, the lines in the changelog are weirdly truncated so it is barely
readable. Please fix that. Secondly, I don't think it is reasonable to
impose additional cost of zeroing on this hot path only to fix harmless
KMSAN warning. So if we can somehow annotate this to silence KMSAN then
that would be a way to go in my opinion.

								Honza

> ---
> v2:
>   - Screwed up by using memcpy in v1, fixed it here by using memset, only change
>  fs/dcache.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 1b1a81f10da6..730571e92af8 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1910,12 +1910,17 @@ static struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
>  		return NULL;
>  
>  	/*
> -	 * We guarantee that the inline name is always NUL-terminated.
> -	 * This way the memcpy() done by the name switching in rename
> -	 * will still always have a NUL at the end, even if we might
> -	 * be overwriting an internal NUL character
> +	 * Fully initialize the inline name buffer.  copy_name() and
> +	 * swap_names() copy d_shortname in its entirety, so any
> +	 * uninitialized tail would propagate to the other dentry, and
> +	 * __d_lookup_rcu() may transiently read any byte of the inline
> +	 * name while rename() rewrites it in place.
> +	 *
> +	 * This also keeps the inline name NUL-terminated: the name
> +	 * switching in rename will still always have a NUL at the end,
> +	 * even if we might be overwriting an internal NUL character.
>  	 */
> -	dentry->d_shortname.string[DNAME_INLINE_LEN-1] = 0;
> +	memset(dentry->d_shortname.string, 0, DNAME_INLINE_LEN);
>  	if (unlikely(!name)) {
>  		name = &slash_name;
>  		dname = dentry->d_shortname.string;
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR
[PATCH v3] dcache: unpoison the inline name buffer in __d_alloc()
Posted by Drif Abdelmalek Mohamed Said 6 days, 11 hours ago 
syzbot reported:

    BUG: KMSAN: uninit-value in dentry_string_cmp fs/dcache.c:291 [inline]
    BUG: KMSAN: uninit-value in dentry_cmp fs/dcache.c:322 [inline]
    BUG: KMSAN: uninit-value in __d_lookup_rcu+0x37d/0x5e0 fs/dcache.c:2522

     dentry_string_cmp fs/dcache.c:291 [inline]
     dentry_cmp fs/dcache.c:322 [inline]
     __d_lookup_rcu+0x37d/0x5e0 fs/dcache.c:2522
     lookup_fast+0x194/0xa40 fs/namei.c:1854
     lookup_fast_for_open fs/namei.c:4545 [inline]
     open_last_lookups fs/namei.c:4579 [inline]
     path_openat+0x9ef/0x6540 fs/namei.c:4856
     do_file_open+0x2aa/0x680 fs/namei.c:4888
     do_sys_openat2+0x17c/0x390 fs/open.c:1395
     do_sys_open fs/open.c:1401 [inline]
     __do_sys_openat fs/open.c:1417 [inline]
     __se_sys_openat fs/open.c:1412 [inline]
     __x64_sys_openat+0x240/0x300 fs/open.c:1412
     x64_sys_call+0x2445/0x3ea0 arch/x86/include/generated/asm/syscalls_64.h:258
     do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
     do_syscall_64+0x15d/0x3c0 arch/x86/entry/syscall_64.c:94
     entry_SYSCALL_64_after_hwframe+0x77/0x7f

Uninit was stored to memory at:
     copy_name fs/dcache.c:3031 [inline]
     __d_move+0xd29/0x21f0 fs/dcache.c:3099
     d_move+0x71/0xf0 fs/dcache.c:3147
     vfs_rename+0x2619/0x2770 fs/namei.c:6085
     filename_renameat2+0xa59/0x1230 fs/namei.c:6188
     __do_sys_rename fs/namei.c:6232 [inline]
     __se_sys_rename+0xc5/0x5c0 fs/namei.c:6228
     __x64_sys_rename+0x78/0xb0 fs/namei.c:6228
     x64_sys_call+0x329/0x3ea0 arch/x86/include/generated/asm/syscalls_64.h:83
     do_syscall_x64 arch/x86/entry/syscall_64.c:63
     do_syscall_64+0x15d/0x3c0 arch/x86/entry/syscall_64.c:94
     entry_SYSCALL_64_after_hwframe+0x77/0x7f

Uninit was created at:
     slab_post_alloc_hook mm/slub.c:4617 [inline]
     slab_alloc_node mm/slub.c:4939 [inline]
     kmem_cache_alloc_lru_noprof+0x376/0x1230 mm/s
     __d_alloc+0x52/0x9f0 fs/dcache.c:1902
     d_alloc+0x57/0x300 fs/dcache.c:1981
     lookup_one_qstr_excl+0x19d/0x7a0 fs/namei.c:1806
     __start_renaming+0x341/0x850 fs/namei.c:3888
     filename_renameat2+0x625/0x1230 fs/namei.c:6163
     __do_sys_rename fs/namei.c:6232 [inline]
     __se_sys_rename+0xc5/0x5c0 fs/namei.c:6228
     __x64_sys_rename+0x78/0xb0 fs/namei.c:6228
     x64_sys_call+0x329/0x3ea0 arch/x86/include/generated/asm/syscalls_64.h:83
     do_syscall_x64 arch/x86/entry/syscall_64.c:63
     do_syscall_64+0x15d/0x3c0 arch/x86/entry/syscall_64.c:94
     entry_SYSCALL_64_after_hwframe+0x77/0x7f

The race is between a concurrent open() and rename() of the same path.

__d_alloc() only stores the name itself and its terminating NUL, so the
rest of the inline buffer (d_shortname, DNAME_INLINE_LEN bytes) is left
uninitialized.  copy_name(), called from rename(), copies that buffer as a
whole, so the uninitialized tail is propagated into the dentry that is
being moved.  Meanwhile __d_lookup_rcu(), called from open(), is an
optimistic lockless lookup: it checks d_name.hash_len first and leaves the
seqcount retry to its caller, so it can end up comparing against a dentry
whose name a rename is rewriting in place, using a stale (longer) length.
The comparison then runs past the terminating NUL and reads bytes of the
uninitialized tail, which KMSAN reports.

The read is harmless by design: it stays inside the buffer, the name is
still NUL-terminated, and the result is thrown away by the seqcount retry.
It is not specific to KMSAN either - with CONFIG_DCACHE_WORD_ACCESS
enabled the very same bytes are read by read_word_at_a_time(), which is
__no_sanitize_or_inline and therefore invisible to KMSAN.  KMSAN builds
only see the instrumented byte-at-a-time dentry_string_cmp() because
CONFIG_DCACHE_WORD_ACCESS is disabled when KMSAN is enabled on x86:

commit 7cf8f44a5a1c ("x86: fs: kmsan: disable CONFIG_DCACHE_WORD_ACCESS")

Zeroing the inline buffer would hide the report, but it would add a
memset() to a hot allocation path just to initialize bytes that are never
used as part of a name.  Instead, tell KMSAN the inline buffer is
initialized: kmsan_unpoison_memory() compiles to nothing unless
CONFIG_KMSAN is set, and doing it at allocation time is enough for every
dentry, because copy_name() and swap_names() copy the whole buffer and
thus propagate its shadow.

Reported-by: syzbot+7ff3adde89dd795ad4c4@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=7ff3adde89dd795ad4c4
Signed-off-by: Drif Abdelmalek Mohamed Said <drifabdelmalekmohamedsaid@gmail.com>

Changes in v3:
- Annotate for KMSAN instead of zeroing, as suggested in review: the read
  is harmless, so unpoison the inline buffer in __d_alloc() with
  kmsan_unpoison_memory() (a no-op unless CONFIG_KMSAN) rather than adding
  a memset() to the dentry allocation path.
- Document why only KMSAN builds report this at all: with
  CONFIG_DCACHE_WORD_ACCESS the same read goes through
  read_word_at_a_time(), which KMSAN does not instrument.
- Rewrite the commit message; the previous one had several truncated
  lines.
---
 fs/dcache.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/dcache.c b/fs/dcache.c
index 1b1a81f10da6..a66be85f9d01 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1916,6 +1916,10 @@ static struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
 	 * be overwriting an internal NUL character
 	 */
 	dentry->d_shortname.string[DNAME_INLINE_LEN-1] = 0;
+
+	/* Racy __d_lookup_rcu() walk may read past the NUL; harmless */
+	kmsan_unpoison_memory(dentry->d_shortname.string, DNAME_INLINE_LEN);
+
 	if (unlikely(!name)) {
 		name = &slash_name;
 		dname = dentry->d_shortname.string;
-- 
2.43.0
Re: [PATCH v3] dcache: unpoison the inline name buffer in __d_alloc()
Posted by Jan Kara 4 days, 2 hours ago 
On Fri 18-09-26 23:42:04, Drif Abdelmalek Mohamed Said wrote:
> syzbot reported:
> 
>     BUG: KMSAN: uninit-value in dentry_string_cmp fs/dcache.c:291 [inline]
>     BUG: KMSAN: uninit-value in dentry_cmp fs/dcache.c:322 [inline]
>     BUG: KMSAN: uninit-value in __d_lookup_rcu+0x37d/0x5e0 fs/dcache.c:2522
> 
>      dentry_string_cmp fs/dcache.c:291 [inline]
>      dentry_cmp fs/dcache.c:322 [inline]
>      __d_lookup_rcu+0x37d/0x5e0 fs/dcache.c:2522
>      lookup_fast+0x194/0xa40 fs/namei.c:1854
>      lookup_fast_for_open fs/namei.c:4545 [inline]
>      open_last_lookups fs/namei.c:4579 [inline]
>      path_openat+0x9ef/0x6540 fs/namei.c:4856
>      do_file_open+0x2aa/0x680 fs/namei.c:4888
>      do_sys_openat2+0x17c/0x390 fs/open.c:1395
>      do_sys_open fs/open.c:1401 [inline]
>      __do_sys_openat fs/open.c:1417 [inline]
>      __se_sys_openat fs/open.c:1412 [inline]
>      __x64_sys_openat+0x240/0x300 fs/open.c:1412
>      x64_sys_call+0x2445/0x3ea0 arch/x86/include/generated/asm/syscalls_64.h:258
>      do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
>      do_syscall_64+0x15d/0x3c0 arch/x86/entry/syscall_64.c:94
>      entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> Uninit was stored to memory at:
>      copy_name fs/dcache.c:3031 [inline]
>      __d_move+0xd29/0x21f0 fs/dcache.c:3099
>      d_move+0x71/0xf0 fs/dcache.c:3147
>      vfs_rename+0x2619/0x2770 fs/namei.c:6085
>      filename_renameat2+0xa59/0x1230 fs/namei.c:6188
>      __do_sys_rename fs/namei.c:6232 [inline]
>      __se_sys_rename+0xc5/0x5c0 fs/namei.c:6228
>      __x64_sys_rename+0x78/0xb0 fs/namei.c:6228
>      x64_sys_call+0x329/0x3ea0 arch/x86/include/generated/asm/syscalls_64.h:83
>      do_syscall_x64 arch/x86/entry/syscall_64.c:63
>      do_syscall_64+0x15d/0x3c0 arch/x86/entry/syscall_64.c:94
>      entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> Uninit was created at:
>      slab_post_alloc_hook mm/slub.c:4617 [inline]
>      slab_alloc_node mm/slub.c:4939 [inline]
>      kmem_cache_alloc_lru_noprof+0x376/0x1230 mm/s
>      __d_alloc+0x52/0x9f0 fs/dcache.c:1902
>      d_alloc+0x57/0x300 fs/dcache.c:1981
>      lookup_one_qstr_excl+0x19d/0x7a0 fs/namei.c:1806
>      __start_renaming+0x341/0x850 fs/namei.c:3888
>      filename_renameat2+0x625/0x1230 fs/namei.c:6163
>      __do_sys_rename fs/namei.c:6232 [inline]
>      __se_sys_rename+0xc5/0x5c0 fs/namei.c:6228
>      __x64_sys_rename+0x78/0xb0 fs/namei.c:6228
>      x64_sys_call+0x329/0x3ea0 arch/x86/include/generated/asm/syscalls_64.h:83
>      do_syscall_x64 arch/x86/entry/syscall_64.c:63
>      do_syscall_64+0x15d/0x3c0 arch/x86/entry/syscall_64.c:94
>      entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> The race is between a concurrent open() and rename() of the same path.
> 
> __d_alloc() only stores the name itself and its terminating NUL, so the
> rest of the inline buffer (d_shortname, DNAME_INLINE_LEN bytes) is left
> uninitialized.  copy_name(), called from rename(), copies that buffer as a
> whole, so the uninitialized tail is propagated into the dentry that is
> being moved.  Meanwhile __d_lookup_rcu(), called from open(), is an
> optimistic lockless lookup: it checks d_name.hash_len first and leaves the
> seqcount retry to its caller, so it can end up comparing against a dentry
> whose name a rename is rewriting in place, using a stale (longer) length.
> The comparison then runs past the terminating NUL and reads bytes of the
> uninitialized tail, which KMSAN reports.
> 
> The read is harmless by design: it stays inside the buffer, the name is
> still NUL-terminated, and the result is thrown away by the seqcount retry.
> It is not specific to KMSAN either - with CONFIG_DCACHE_WORD_ACCESS
> enabled the very same bytes are read by read_word_at_a_time(), which is
> __no_sanitize_or_inline and therefore invisible to KMSAN.  KMSAN builds
> only see the instrumented byte-at-a-time dentry_string_cmp() because
> CONFIG_DCACHE_WORD_ACCESS is disabled when KMSAN is enabled on x86:
> 
> commit 7cf8f44a5a1c ("x86: fs: kmsan: disable CONFIG_DCACHE_WORD_ACCESS")
> 
> Zeroing the inline buffer would hide the report, but it would add a
> memset() to a hot allocation path just to initialize bytes that are never
> used as part of a name.  Instead, tell KMSAN the inline buffer is
> initialized: kmsan_unpoison_memory() compiles to nothing unless
> CONFIG_KMSAN is set, and doing it at allocation time is enough for every
> dentry, because copy_name() and swap_names() copy the whole buffer and
> thus propagate its shadow.
> 
> Reported-by: syzbot+7ff3adde89dd795ad4c4@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7ff3adde89dd795ad4c4
> Signed-off-by: Drif Abdelmalek Mohamed Said <drifabdelmalekmohamedsaid@gmail.com>

The --- separator should be here. I think Christian can fix this up on
commit. Otherwise feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> 
> Changes in v3:
> - Annotate for KMSAN instead of zeroing, as suggested in review: the read
>   is harmless, so unpoison the inline buffer in __d_alloc() with
>   kmsan_unpoison_memory() (a no-op unless CONFIG_KMSAN) rather than adding
>   a memset() to the dentry allocation path.
> - Document why only KMSAN builds report this at all: with
>   CONFIG_DCACHE_WORD_ACCESS the same read goes through
>   read_word_at_a_time(), which KMSAN does not instrument.
> - Rewrite the commit message; the previous one had several truncated
>   lines.
> ---
>  fs/dcache.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 1b1a81f10da6..a66be85f9d01 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1916,6 +1916,10 @@ static struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
>  	 * be overwriting an internal NUL character
>  	 */
>  	dentry->d_shortname.string[DNAME_INLINE_LEN-1] = 0;
> +
> +	/* Racy __d_lookup_rcu() walk may read past the NUL; harmless */
> +	kmsan_unpoison_memory(dentry->d_shortname.string, DNAME_INLINE_LEN);
> +
>  	if (unlikely(!name)) {
>  		name = &slash_name;
>  		dname = dentry->d_shortname.string;
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.