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

[Jan Kara]

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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=7ff3adde89dd795ad4c4
> Signed-off-by: Drif Abdelmalek Mohamed Said <drifabdelmalekmohamedsaid@xxxxxxxxx>

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@xxxxxxxx>
SUSE Labs, CR