ksmbd: fix use-after-free of fp->owner.name in durable handle owner check

Two concurrent SMB2 durable reconnects (DH2C/DHnC) on the same
persistent_id race the fp->owner.name compare-read in
ksmbd_vfs_compare_durable_owner() against the kfree() in
ksmbd_reopen_durable_fd()'s reopen-success path. fp->owner.name is a
standalone kstrdup() buffer whose lifetime is independent of the fp
refcount, and the two sites share no lock: the compare reads the buffer
while the reopen frees it, so the strcmp() can dereference freed memory.

Commit 7ce4fc40018d ("ksmbd: fix durable reconnect double-bind race in
ksmbd_reopen_durable_fd") made the fp->conn claim atomic under
global_ft.lock (closing the owner.name double-free and the ksmbd_file
write-UAF), but the compare-read versus reopen-free pair was left
unserialized.

  BUG: KASAN: slab-use-after-free in strcmp+0x2c/0x80
  Read of size 1 by task kworker
    strcmp
    ksmbd_vfs_compare_durable_owner
    smb2_check_durable_oplock
    smb2_open
  Freed by task kworker:
    kfree
    ksmbd_reopen_durable_fd
    smb2_open
  Allocated by task kworker:
    kstrdup
    session_fd_check
    smb2_session_logoff
  The buggy address belongs to the cache kmalloc-8

Serialize both sides of the race with fp->f_lock.  The global durable
file-table lock still protects the durable reconnect claim, but
fp->owner.name is per-open state and does not need to block unrelated
durable table lookups or reconnects.  The teardown is left at its
existing location after the reopen-success point so that an __open_id()
rollback still retains owner.name for a later legitimate reconnect to
verify.

Fixes: 49110a8ce654 ("ksmbd: validate owner of durable handle on reconnect")
Assisted-by: Henry (Claude):claude-opus-4
Signed-off-by: Gil Portnoy <dddhkts1@gmail.com>
Co-developed-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
Signed-off-by: Steve French <stfrench@microsoft.com>

diff --git a/fs/smb/server/vfs_cache.c b/fs/smb/server/vfs_cache.c
index 525e9a3a2e9b8d86204e05f21edd50c44777aa1e..3c9443ac3522d948830d19913538efcd605fcd74 100644 (file)
--- a/fs/smb/server/vfs_cache.c
+++ b/fs/smb/server/vfs_cache.c
@@ -1461,16 +1461,21 @@ void ksmbd_stop_durable_scavenger(void)
 static int ksmbd_vfs_copy_durable_owner(struct ksmbd_file *fp,
                struct ksmbd_user *user)
 {
+       char *name;
+
        if (!user)
                return -EINVAL;
 
        /* Duplicate the user name to ensure identity persistence */
-       fp->owner.name = kstrdup(user->name, GFP_KERNEL);
-       if (!fp->owner.name)
+       name = kstrdup(user->name, GFP_KERNEL);
+       if (!name)
                return -ENOMEM;
 
+       spin_lock(&fp->f_lock);
        fp->owner.uid = user->uid;
        fp->owner.gid = user->gid;
+       fp->owner.name = name;
+       spin_unlock(&fp->f_lock);
 
        return 0;
 }
@@ -1488,18 +1493,24 @@ static int ksmbd_vfs_copy_durable_owner(struct ksmbd_file *fp,
 bool ksmbd_vfs_compare_durable_owner(struct ksmbd_file *fp,
                struct ksmbd_user *user)
 {
-       if (!user || !fp->owner.name)
+       bool ret = false;
+
+       if (!user)
                return false;
 
+       spin_lock(&fp->f_lock);
+       if (!fp->owner.name)
+               goto out;
+
        /* Check if the UID and GID match first (fast path) */
        if (fp->owner.uid != user->uid || fp->owner.gid != user->gid)
-               return false;
+               goto out;
 
        /* Validate the account name to ensure the same SecurityContext */
-       if (strcmp(fp->owner.name, user->name))
-               return false;
-
-       return true;
+       ret = (strcmp(fp->owner.name, user->name) == 0);
+out:
+       spin_unlock(&fp->f_lock);
+       return ret;
 }
 
 static bool session_fd_check(struct ksmbd_tree_connect *tcon,
@@ -1694,9 +1705,11 @@ int ksmbd_reopen_durable_fd(struct ksmbd_work *work, struct ksmbd_file *fp)
        }
        up_write(&ci->m_lock);
 
+       spin_lock(&fp->f_lock);
        fp->owner.uid = fp->owner.gid = 0;
        kfree(fp->owner.name);
        fp->owner.name = NULL;
+       spin_unlock(&fp->f_lock);
 
        return 0;
 }