To avoid touching a target that is concurrently being removed, configfs_get_config_item() and create_link() both rely on a hashed dentry as proof that the config_item/configfs_dirent behind it is still alive (using locks could lead to deadlocks, as described on configfs_symlink). However, configfs_remove_dir drops the last reference without enforcing proper unhash over the dentry. This enables a possible race condition where a symlink leads to UAF over a dentry that has no reference but is still stale on the hash. Therefore, the dentry removal must also enforce proper unhash, avoiding this specific UAF scenario. Verified against syzbot's C reproducer: no longer triggers the WARN_ON/KASAN panics after this change. Reported-by: syzbot+608f7f2a86361e18ba0b@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=608f7f2a86361e18ba0b Signed-off-by: Marcelo Mendes Spessoto Junior --- fs/configfs/dir.c | 15 +++++++++++++++ fs/configfs/symlink.c | 30 ++++++++++++++++++++++++++---- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c index 3c88f13f1ca2..ec6f3550178a 100644 --- a/fs/configfs/dir.c +++ b/fs/configfs/dir.c @@ -411,6 +411,21 @@ static void configfs_remove_dir(struct dentry *d) { struct dentry * parent = dget(d->d_parent); + /* + * Unhash before dropping any reference to the dirent/item this + * dentry pins: a concurrent configfs_get_config_item() (e.g. from + * configfs_symlink()'s target resolution, which runs unlocked + * against directories it doesn't otherwise own) only checks + * d_unhashed() under d_lock before pinning the item. Unhashing + * first ensures that check reliably fails once we're past this + * point, instead of racing the dirent/item's refcount reaching + * zero while the dentry is still (briefly) hashed. + */ + spin_lock(&d->d_lock); + if (simple_positive(d)) + __d_drop(d); + spin_unlock(&d->d_lock); + configfs_remove_dirent(d); if (d_really_is_positive(d)) { diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c index 31eb28b27309..b8043a6b0b42 100644 --- a/fs/configfs/symlink.c +++ b/fs/configfs/symlink.c @@ -78,18 +78,40 @@ static int create_link(struct config_item *parent_item, struct config_item *item, struct dentry *dentry) { - struct configfs_dirent *target_sd = item->ci_dentry->d_fsdata; + struct dentry *target_dentry = item->ci_dentry; + struct configfs_dirent *target_sd; char *body; int ret; - if (!configfs_dirent_is_ready(target_sd)) + /* + * item is pinned by the caller, but that only keeps the config_item + * itself alive. item->ci_dentry's configfs_dirent (and thus its + * s_count) is a separate refcount that a concurrent rmdir of this + * same directory can drop to zero and free independently -- see the + * matching d_lock/d_unhashed() dance in configfs_get_config_item() + * and the unhash-before-free ordering configfs_remove_dir() now + * guarantees. Do the same check here instead of trusting + * target_dentry->d_fsdata unconditionally. + */ + spin_lock(&target_dentry->d_lock); + if (d_unhashed(target_dentry)) { + spin_unlock(&target_dentry->d_lock); return -ENOENT; + } + target_sd = configfs_get(target_dentry->d_fsdata); + spin_unlock(&target_dentry->d_lock); + + if (!configfs_dirent_is_ready(target_sd)) { + configfs_put(target_sd); + return -ENOENT; + } body = kzalloc(PAGE_SIZE, GFP_KERNEL); - if (!body) + if (!body) { + configfs_put(target_sd); return -ENOMEM; + } - configfs_get(target_sd); spin_lock(&configfs_dirent_lock); if (target_sd->s_type & CONFIGFS_USET_DROPPING) { spin_unlock(&configfs_dirent_lock); -- 2.55.0