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

[DRIF ABDELMALEK MOHAMED SAID]

Hello Breno,
Thank you for your feedback, i sent v3 for the patch,
> +     config_item_put(parent_item);
> +     parent_item = NULL;

>Why do you need to set it to NULL here?
Your were right here, configfs_get_config_item returns null upon
failure so that was redundant.

Here is link for the patch:
https://lore.kernel.org/all/20261105165055.362-1-drifabdelmalekmohamedsaid@xxxxxxxxx/

Cordially,

On Thu, Nov 5, 2026 at 12:29 PM Breno Leitao <leitao@xxxxxxxxxx> wrote:
>
> On Fri, Oct 23, 2026 at 05:29:06PM +0100, Drif Abdelmalek Mohamed Said wrote:
> > syzbot reported:
>
> Please drop this full log, given we have a link to the syzbot report.
>
> > 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.
>
> This is LLM-heavy and hard to follow, would you be able to make it a bit
> more human?
>
> > ---
> > 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@xxxxxxxxx/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*/
>
> Nit space before */
>
> > +     config_item_put(parent_item);
> > +     parent_item = NULL;
>
> Why do you need to set it to NULL here?
>
> >       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 */
>
> This comment is not necessary.
>
> Thanks for the patch,
> --breno