Re: [PATCH v2] configfs: fix slab-UAF Write in config_item_put

Re: [PATCH v2] configfs: fix slab-UAF Write in config_item_put

[syzbot]

> #syz test:

I've failed to parse your command.
Did you perhaps forget to provide the branch name, or added an extra ':'?
Please use one of the two supported formats:
1. #syz test
2. #syz test: repo branch-or-commit-hash
Note the lack of ':' in option 1.

>
> diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
> index 89178f5d371a..7d746a10c369 100644
> --- a/fs/configfs/symlink.c
> +++ b/fs/configfs/symlink.c
> @@ -196,11 +196,26 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
>  	 *
>  	 *  AV, a thoroughly annoyed bastard.
>  	 */
> +
> +	/* Put the reference back, retake and check after relocking*/
> +	config_item_put(parent_item);
> +	parent_item = NULL;
>  	inode_unlock(dir);
>  	ret = get_target(symname, &target_item, &target_sd, dentry->d_sb);
>  	inode_lock(dir);
>  	if (ret)
> -		goto out_put;
> +		goto out_put; /* config_item_put on NULL, no problem */
> +
> +	if (IS_DEADDIR(dir)) {
> +		ret = -ENOENT;
> +		goto out_put_target;
> +	}
> +
> +	parent_item = configfs_get_config_item(dentry->d_parent);
> +	if (!parent_item) {
> +		ret = -ENOENT;
> +		goto out_put_target;
> +	}
>  
>  	if (dentry->d_inode || d_unhashed(dentry))
>  		ret = -EEXIST;
> @@ -218,6 +233,7 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
>  						     target_item);
>  	}
>  
> +out_put_target:
>  	configfs_put(target_sd);
>  	config_item_put(target_item);
>  

[PATCH v2] configfs: fix slab-UAF Write in config_item_put

[Drif Abdelmalek Mohamed Said]

syzbot reported:

BUG: KASAN: slab-use-after-free in instrument_atomic_read_write
           include/linux/instrumented.h:112 [inline]
BUG: KASAN: slab-use-after-free in atomic_fetch_sub_release
           include/linux/atomic/atomic-instrumented.h:400 [inline]
BUG: KASAN: slab-use-after-free in __refcount_sub_and_test
           include/linux/refcount.h:389 [inline]
BUG: KASAN: slab-use-after-free in __refcount_dec_and_test
           include/linux/refcount.h:432 [inline]
BUG: KASAN: slab-use-after-free in refcount_dec_and_test
           include/linux/refcount.h:450 [inline]
BUG: KASAN: slab-use-after-free in kref_put
           include/linux/kref.h:64 [inline]
BUG: KASAN: slab-use-after-free in config_item_put+0x26/0x90
           fs/configfs/item.c:149
Write of size 4 at addr ffff88803b6444b4 by task syz-executor324/5709

Call Trace:
 <TASK>
 dump_stack_lvl+0xe8/0x150 lib/dump_stack.c:120
 print_address_description+0x55/0x1e0 mm/kasan/report.c:378
 print_report+0x58/0x70 mm/kasan/report.c:482
 kasan_report+0x117/0x150 mm/kasan/report.c:595
 check_region_inline mm/kasan/generic.c:-1 [inline]
 kasan_check_range+0x264/0x2c0 mm/kasan/generic.c:200
 instrument_atomic_read_write include/linux/instrumented.h:112 [inline]
 atomic_fetch_sub_release include/linux/atomic/atomic-instrumented.h:400 [inline]
 __refcount_sub_and_test include/linux/refcount.h:389 [inline]
 __refcount_dec_and_test include/linux/refcount.h:432 [inline]
 refcount_dec_and_test include/linux/refcount.h:450 [inline]
 kref_put include/linux/kref.h:64 [inline]
 config_item_put+0x26/0x90 fs/configfs/item.c:149
 configfs_symlink+0x6be/0x11d0 fs/configfs/symlink.c:225
 vfs_symlink+0x18b/0x330 fs/namei.c:5823
 filename_symlinkat+0x1cd/0x420 fs/namei.c:5848
 __do_sys_symlinkat fs/namei.c:5868 [inline]
 __se_sys_symlinkat+0x4e/0x2b0 fs/namei.c:5863
 do_syscall_x64 arch/x86/entry/syscall_64.c:61 [inline]
 do_syscall_64+0x166/0x520 arch/x86/entry/syscall_64.c:84
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Allocated by task 5712:
 kasan_save_stack mm/kasan/common.c:57 [inline]
 kasan_save_track+0x3e/0x80 mm/kasan/common.c:78
 poison_kmalloc_redzone mm/kasan/common.c:409 [inline]
 __kasan_kmalloc+0x93/0xb0 mm/kasan/common.c:426
 kasan_kmalloc include/linux/kasan.h:263 [inline]
 __kmalloc_cache_noprof+0x3d5/0x680 mm/slub.c:5563
 _kmalloc_noprof include/linux/slab.h:991 [inline]
 _kzalloc_noprof include/linux/slab.h:1312 [inline]
 nvmet_ports_make+0xe6/0xf00 drivers/nvme/target/configfs.c:2054
 configfs_mkdir+0x4f6/0x9e0 fs/configfs/dir.c:1369
 vfs_mkdir+0x408/0x620 fs/namei.c:5439
 filename_mkdirat+0x289/0x520 fs/namei.c:5472
 __do_sys_mkdir fs/namei.c:5499 [inline]
 __se_sys_mkdir+0x34/0x150 fs/namei.c:5496
 do_syscall_x64 arch/x86/entry/syscall_64.c:61 [inline]
 do_syscall_64+0x166/0x520 arch/x86/entry/syscall_64.c:84
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Freed by task 5712:
 kasan_save_stack mm/kasan/common.c:57 [inline]
 kasan_save_track+0x3e/0x80 mm/kasan/common.c:78
 kasan_save_free_info+0x40/0x50 mm/kasan/generic.c:584
 poison_slab_object mm/kasan/common.c:264 [inline]
 __kasan_slab_free+0x5c/0x80 mm/kasan/common.c:296
 kasan_slab_free include/linux/kasan.h:235 [inline]
 slab_free_hook mm/slub.c:2748 [inline]
 slab_free mm/slub.c:6509 [inline]
 kfree+0x1c5/0x6d0 mm/slub.c:6802
 config_item_cleanup fs/configfs/item.c:128 [inline]
 config_item_release+0x13a/0x2d0 fs/configfs/item.c:137
 configfs_rmdir+0x885/0x950 fs/configfs/dir.c:1580
 vfs_rmdir+0x3e9/0x6b0 fs/namei.c:5544
 filename_rmdir+0x292/0x520 fs/namei.c:5601
 __do_sys_rmdir fs/namei.c:5624 [inline]
 __se_sys_rmdir+0x2e/0x140 fs/namei.c:5621
 do_syscall_x64 arch/x86/entry/syscall_64.c:61 [inline]
 do_syscall_64+0x166/0x520 arch/x86/entry/syscall_64.c:84
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

The buggy address belongs to the object at ffff88803b644000
 which belongs to the cache kmalloc-4k of size 4096
The buggy address is located 1204 bytes inside of
 freed 4096-byte region [ffff88803b644000, ffff88803b645000)

This is a race between a thread creating a symlink and another one
doing rmdir on a parent (or ancestor) directory of symlink creation dir.

configfs_symlink() takes a reference on parent_item, the directory the
link is created in, and then drops the directory inode lock around
get_target(). While the lock is dropped, a concurrent rmdir can remove
the group that parent_item belongs to.

If parent_item is a default group embedded in a larger object, such as
nvmet_port.subsys_group, the reference on the embedded item does not
keep the containing object alive. rmdir frees the object, and the final
config_item_put(parent_item) in configfs_symlink() then decrements a
kref inside freed memory.

Fix this by not holding parent_item across the unlocked window. Drop
the reference before inode_unlock() and look it up again from
dentry->d_parent after inode_lock(). configfs_get_config_item() returns
NULL if the parent dentry was unhashed in the meantime, in which case
release the target references and return -ENOENT.

Re-looking up parent_item is not sufficient on its own.
configfs_get_config_item() only fails if the parent dentry has been
unhashed, and the rmdir path in another thread may skip that: if
simple_rmdir() failed, the earlier d_drop() was skipped, so the dentry
can stay hashed even though the directory has already been detached.

Such a directory was created as a default group (CONFIGFS_USET_DIR), so
configfs_rmdir() takes the CONFIGFS_USET_DIR branch and calls
configfs_detach_group(). detach_groups() then marks each child inode
S_DEAD (d_inode(child)->i_flags |= S_DEAD). Check IS_DEADDIR(dir)
after retaking the inode lock and before taking a reference on the
parent, and return -ENOENT. This also rejects a detached directory
whose dentry is still hashed.

Reported-by: syzbot+63336d70e1af377d7388@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=63336d70e1af377d7388
Signed-off-by: Drif Abdelmalek Mohamed Said <drifabdelmalekmohamedsaid@gmail.com>
---
v2: Added check for dead dir, this would be helpful when configfs_remove_dir skips
unhashing the dentry.
v1: https://lore.kernel.org/all/20261017123748.4430-1-drifabdelmalekmohamedsaid@gmail.com/T/
 fs/configfs/symlink.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index 89178f5d371a..7d746a10c369 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -196,11 +196,26 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
 	 *
 	 *  AV, a thoroughly annoyed bastard.
 	 */
+
+	/* Put the reference back, retake and check after relocking*/
+	config_item_put(parent_item);
+	parent_item = NULL;
 	inode_unlock(dir);
 	ret = get_target(symname, &target_item, &target_sd, dentry->d_sb);
 	inode_lock(dir);
 	if (ret)
-		goto out_put;
+		goto out_put; /* config_item_put on NULL, no problem */
+
+	if (IS_DEADDIR(dir)) {
+		ret = -ENOENT;
+		goto out_put_target;
+	}
+
+	parent_item = configfs_get_config_item(dentry->d_parent);
+	if (!parent_item) {
+		ret = -ENOENT;
+		goto out_put_target;
+	}
 
 	if (dentry->d_inode || d_unhashed(dentry))
 		ret = -EEXIST;
@@ -218,6 +233,7 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
 						     target_item);
 	}
 
+out_put_target:
 	configfs_put(target_sd);
 	config_item_put(target_item);
 
-- 
2.43.0