Re: [PATCH] configfs: fix use-after-free of default group in configfs_symlink()

Re: [PATCH] configfs: fix use-after-free of default group in configfs_symlink()

[syzbot]

> #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Patch testing via #syz test is temporarily disabled.
See https://github.com/google/syzkaller/issues/8122 for details.

>
> configfs_symlink() drops the parent directory's inode lock while it
> resolves the link target, and keeps a reference to parent_item across
> that window. When the parent is a default group, such as
> ports/<n>/subsystems/ in the NVMe target, parent_item is embedded in
> the containing object (struct nvmet_port). A reference on the embedded
> item does not keep the containing object alive, so a concurrent rmdir of
> the port can free it. allow_link() and the final config_item_put() then
> touch freed memory:
>
>   BUG: KASAN: slab-use-after-free in nvmet_port_subsys_allow_link+0x41/0x2e0
>   Call Trace:
>    nvmet_port_subsys_allow_link+0x41/0x2e0 drivers/nvme/target/configfs.c:1059
>    configfs_symlink+0x59a/0x1030 fs/configfs/symlink.c:196
>    vfs_symlink+0x18b/0x330 fs/namei.c:5794
>
> detach_groups() marks a default group's directory S_DEAD while holding
> that directory's inode lock, before the containing object can be freed.
> So drop the parent_item reference before unlocking dir, and after
> re-taking the lock return -ENOENT if dir is dead. Otherwise look
> parent_item up again, which is safe while dir is locked and alive.
>
> Fixes: 351e5d869e5a ("configfs: fix a deadlock in configfs_symlink()")
> Reported-by: syzbot+b0996ac2197dd7420c3e@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=b0996ac2197dd7420c3e
> Cc: linux-nvme@lists.infradead.org
> Signed-off-by: Izaac Chong <izaacchong934@gmail.com>
> ---
>  fs/configfs/symlink.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
> index 89178f5d371a..1b2c9048a920 100644
> --- a/fs/configfs/symlink.c
> +++ b/fs/configfs/symlink.c
> @@ -166,11 +166,11 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
>  
>  	parent_item = configfs_get_config_item(dentry->d_parent);
>  	type = parent_item->ci_type;
> +	config_item_put(parent_item);
>  
> -	ret = -EPERM;
>  	if (!type || !type->ct_item_ops ||
>  	    !type->ct_item_ops->allow_link)
> -		goto out_put;
> +		return -EPERM;
>  
>  	/*
>  	 * This is really sick.  What they wanted was a hybrid of
> @@ -200,7 +200,20 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
>  	ret = get_target(symname, &target_item, &target_sd, dentry->d_sb);
>  	inode_lock(dir);
>  	if (ret)
> -		goto out_put;
> +		return ret;
> +
> +	/*
> +	 * parent_item may be a default group embedded in an object that a
> +	 * concurrent rmdir freed while dir was unlocked, so we dropped our
> +	 * reference above.  detach_groups() marks dir S_DEAD under its inode
> +	 * lock before the containing object can go away, so it is safe to
> +	 * look parent_item up again only if dir is still alive.
> +	 */
> +	if (IS_DEADDIR(dir)) {
> +		ret = -ENOENT;
> +		goto out_put_target;
> +	}
> +	parent_item = configfs_get_config_item(dentry->d_parent);
>  
>  	if (dentry->d_inode || d_unhashed(dentry))
>  		ret = -EEXIST;
> @@ -218,11 +231,10 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
>  						     target_item);
>  	}
>  
> +	config_item_put(parent_item);
> +out_put_target:
>  	configfs_put(target_sd);
>  	config_item_put(target_item);
> -
> -out_put:
> -	config_item_put(parent_item);
>  	return ret;
>  }
>  
> -- 
> 2.39.2 (Apple Git-143)
>

[PATCH] configfs: fix use-after-free of default group in configfs_symlink()

[Izaac Chong]

configfs_symlink() drops the parent directory's inode lock while it
resolves the link target, and keeps a reference to parent_item across
that window. When the parent is a default group, such as
ports/<n>/subsystems/ in the NVMe target, parent_item is embedded in
the containing object (struct nvmet_port). A reference on the embedded
item does not keep the containing object alive, so a concurrent rmdir of
the port can free it. allow_link() and the final config_item_put() then
touch freed memory:

  BUG: KASAN: slab-use-after-free in nvmet_port_subsys_allow_link+0x41/0x2e0
  Call Trace:
   nvmet_port_subsys_allow_link+0x41/0x2e0 drivers/nvme/target/configfs.c:1059
   configfs_symlink+0x59a/0x1030 fs/configfs/symlink.c:196
   vfs_symlink+0x18b/0x330 fs/namei.c:5794

detach_groups() marks a default group's directory S_DEAD while holding
that directory's inode lock, before the containing object can be freed.
So drop the parent_item reference before unlocking dir, and after
re-taking the lock return -ENOENT if dir is dead. Otherwise look
parent_item up again, which is safe while dir is locked and alive.

Fixes: 351e5d869e5a ("configfs: fix a deadlock in configfs_symlink()")
Reported-by: syzbot+b0996ac2197dd7420c3e@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=b0996ac2197dd7420c3e
Cc: linux-nvme@lists.infradead.org
Signed-off-by: Izaac Chong <izaacchong934@gmail.com>
---
Tested with the syzbot C reproducer on v7.4-rc2 in QEMU with KASAN:
without this patch the reproducer triggers the slab-use-after-free in
nvmet_port_subsys_allow_link() on the first run; with it, 3 runs of
5 s each are clean, and normal link/unlink/rmdir of a port still work.
I tried to have syzbot test it, but #syz test is currently disabled
(syzkaller issue #8122).

syzbot's AI patch (v1-v3, still in moderation) pinned the root item of
the default group. This takes a simpler approach: drop the parent
reference before unlocking, and recheck IS_DEADDIR() after relocking,
since detach_groups() sets S_DEAD under the same inode lock.

Not tested: lockdep was not enabled during the reproducer runs, and I
did not confirm that the new -ENOENT path is hit during the race.
---
 fs/configfs/symlink.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index 89178f5d371a..1b2c9048a920 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -166,11 +166,11 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
 
 	parent_item = configfs_get_config_item(dentry->d_parent);
 	type = parent_item->ci_type;
+	config_item_put(parent_item);
 
-	ret = -EPERM;
 	if (!type || !type->ct_item_ops ||
 	    !type->ct_item_ops->allow_link)
-		goto out_put;
+		return -EPERM;
 
 	/*
 	 * This is really sick.  What they wanted was a hybrid of
@@ -200,7 +200,20 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
 	ret = get_target(symname, &target_item, &target_sd, dentry->d_sb);
 	inode_lock(dir);
 	if (ret)
-		goto out_put;
+		return ret;
+
+	/*
+	 * parent_item may be a default group embedded in an object that a
+	 * concurrent rmdir freed while dir was unlocked, so we dropped our
+	 * reference above.  detach_groups() marks dir S_DEAD under its inode
+	 * lock before the containing object can go away, so it is safe to
+	 * look parent_item up again only if dir is still alive.
+	 */
+	if (IS_DEADDIR(dir)) {
+		ret = -ENOENT;
+		goto out_put_target;
+	}
+	parent_item = configfs_get_config_item(dentry->d_parent);
 
 	if (dentry->d_inode || d_unhashed(dentry))
 		ret = -EEXIST;
@@ -218,11 +231,10 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
 						     target_item);
 	}
 
+	config_item_put(parent_item);
+out_put_target:
 	configfs_put(target_sd);
 	config_item_put(target_item);
-
-out_put:
-	config_item_put(parent_item);
 	return ret;
 }
 

---
base-commit: 97d73c8251226cf27cc6656b4c542869bb8c69fa
change-id: 20261114-configfs-symlink-uaf-c5d84bb6a976

Best regards,
--  
Izaac Chong <izaacchong934@gmail.com>

Re: [PATCH] configfs: fix use-after-free of default group in configfs_symlink()

[syzbot]

> #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

Patch testing via #syz test is temporarily disabled.
See https://github.com/google/syzkaller/issues/8122 for details.

>
> configfs_symlink() drops the parent directory's inode lock while it
> resolves the link target, and keeps a reference to parent_item across
> that window. When the parent is a default group, such as
> ports/<n>/subsystems/ in the NVMe target, parent_item is embedded in
> the containing object (struct nvmet_port). A reference on the embedded
> item does not keep the containing object alive, so a concurrent rmdir of
> the port can free it. allow_link() and the final config_item_put() then
> touch freed memory:
>
>   BUG: KASAN: slab-use-after-free in nvmet_port_subsys_allow_link+0x41/0x2e0
>   Call Trace:
>    nvmet_port_subsys_allow_link+0x41/0x2e0 drivers/nvme/target/configfs.c:1059
>    configfs_symlink+0x59a/0x1030 fs/configfs/symlink.c:196
>    vfs_symlink+0x18b/0x330 fs/namei.c:5794
>
> detach_groups() marks a default group's directory S_DEAD while holding
> that directory's inode lock, before the containing object can be freed.
> So drop the parent_item reference before unlocking dir, and after
> re-taking the lock return -ENOENT if dir is dead. Otherwise look
> parent_item up again, which is safe while dir is locked and alive.
>
> Fixes: 351e5d869e5a ("configfs: fix a deadlock in configfs_symlink()")
> Reported-by: syzbot+b0996ac2197dd7420c3e@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=b0996ac2197dd7420c3e
> Cc: linux-nvme@lists.infradead.org
> Signed-off-by: Izaac Chong <izaacchong934@gmail.com>
> ---
>  fs/configfs/symlink.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
> index 89178f5d371a..1b2c9048a920 100644
> --- a/fs/configfs/symlink.c
> +++ b/fs/configfs/symlink.c
> @@ -166,11 +166,11 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
>  
>  	parent_item = configfs_get_config_item(dentry->d_parent);
>  	type = parent_item->ci_type;
> +	config_item_put(parent_item);
>  
> -	ret = -EPERM;
>  	if (!type || !type->ct_item_ops ||
>  	    !type->ct_item_ops->allow_link)
> -		goto out_put;
> +		return -EPERM;
>  
>  	/*
>  	 * This is really sick.  What they wanted was a hybrid of
> @@ -200,7 +200,20 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
>  	ret = get_target(symname, &target_item, &target_sd, dentry->d_sb);
>  	inode_lock(dir);
>  	if (ret)
> -		goto out_put;
> +		return ret;
> +
> +	/*
> +	 * parent_item may be a default group embedded in an object that a
> +	 * concurrent rmdir freed while dir was unlocked, so we dropped our
> +	 * reference above.  detach_groups() marks dir S_DEAD under its inode
> +	 * lock before the containing object can go away, so it is safe to
> +	 * look parent_item up again only if dir is still alive.
> +	 */
> +	if (IS_DEADDIR(dir)) {
> +		ret = -ENOENT;
> +		goto out_put_target;
> +	}
> +	parent_item = configfs_get_config_item(dentry->d_parent);
>  
>  	if (dentry->d_inode || d_unhashed(dentry))
>  		ret = -EEXIST;
> @@ -218,11 +231,10 @@ int configfs_symlink(const struct mnt_idmap *idmap, struct inode *dir,
>  						     target_item);
>  	}
>  
> +	config_item_put(parent_item);
> +out_put_target:
>  	configfs_put(target_sd);
>  	config_item_put(target_item);
> -
> -out_put:
> -	config_item_put(parent_item);
>  	return ret;
>  }
>  
> -- 
> 2.39.2 (Apple Git-143)
>