diff mbox series

ksmbd: rework ksmbd_validate_name_reconnect() to use a dentry walk

Message ID 20260930001605.1917523-1-zhangdairui@gmail.com
State New
Headers show
Series ksmbd: rework ksmbd_validate_name_reconnect() to use a dentry walk | expand

Commit Message

Dairui Zhang Sept. 30, 2026, 12:16 a.m. UTC 
Namjae,

I went over my v2 patch again and found some problems with it.
Sorry for the issues in my earlier patch. I wrote a more
rigorous version; could you please take a look and let me know
if you see any problems with it?

The string prefix check in v2 rejects legitimate durable
reconnects when the share path contains a symlink, because
d_path() of the fp is resolved while the configured string is
not. The new version replaces the string check with a dentry
walk towards share->vfs_path, and also fixes the older case of
a share exported at "/".

From c8efcc786146a951091588e5fa7e3c754850cb3c Mon Sep 17 00:00:00 2001
From: Dairui Zhang <zhangdairui@gmail.com>
Date: Wed, 30 Sep 2026 03:10:00 +0800
Subject: [PATCH] ksmbd: rework ksmbd_validate_name_reconnect() to use a
 dentry walk

The name check in ksmbd_validate_name_reconnect() compares
d_path() of the durable fp against share->path as strings. The
fp path is resolved, the configured string is not, so when an
intermediate component of the share path is a symlink, a
legitimate reconnect to the same file on the same share is
rejected. Example: /a/link -> /real with the share configured
as /a/link/b: a durable handle for /real/b/foo is rejected.

Replace the string checks with an object-based walk from the
fp's dentry towards share->vfs_path, the same root the open
paths use. Each component is compared against the requested
name from its end, under rename_lock so a concurrent rename
cannot mix snapshots. Mount roots are detected by mnt_root and
crossed with follow_up(), so files in nested mounts (crossmnt)
keep working. Reaching the filesystem root without meeting the
share root means the fp does not belong to the share. The share
root itself only matches an empty name, and an unlinked fp is
rejected explicitly. Each walk is limited to 8192 hops and 16
walk attempts, returning -EAGAIN when a limit is hit.

This also fixes reconnects through a share exported at "/",
where the original code compared "oo" for /foo. With no PATH_MAX
buffer or d_path() left in this function, the out-of-bounds
read fixed by v1/v2 cannot reoccur.

Fixes: c8efcc786146 ("ksmbd: add support for durable handles v1/v2")
Cc: stable@vger.kernel.org
Signed-off-by: Dairui Zhang <zhangdairui@gmail.com>
---
Follow-up to the v2 patch in ksmbd-for-next: the string prefix
check still rejects legitimate reconnects on symlinked share
paths, this rework fixes that and the older root-"/" case.
- v2: https://lore.kernel.org/linux-cifs/20260928052026.1789765-1-zhangdairui@gmail.com/
---
 fs/smb/server/vfs_cache.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+), 27 deletions(-)

Comments

Namjae Jeon Oct. 4, 2026, 1:23 a.m. UTC | #1 
On Wed, Sep 30, 2026 at 9:16 AM Dairui Zhang <zhangdairui@gmail.com> wrote:
>
> Namjae,
Hi Dairui,
>
> I went over my v2 patch again and found some problems with it.
> Sorry for the issues in my earlier patch. I wrote a more
> rigorous version; could you please take a look and let me know
> if you see any problems with it?
>
> The string prefix check in v2 rejects legitimate durable
> reconnects when the share path contains a symlink, because
> d_path() of the fp is resolved while the configured string is
> not. The new version replaces the string check with a dentry
> walk towards share->vfs_path, and also fixes the older case of
> a share exported at "/".
Can you check if the attached patches fix this issue?

Thanks!
Dairui Zhang Oct. 4, 2026, 3:11 p.m. UTC | #2 
Namjae,

Thanks for the patches. I went through both and they look
correct, including the failure-path refcounting. My patch was
not good - it was too complicated. Yours is simpler and just
as effective.

One corner case: if the share root is unlinked between
kern_path() and d_path() at init, real_path can end up with a
" (deleted)" suffix, and a later cross-share fp at that
literal path would pass the prefix check.
diff mbox series

Patch

diff --git a/fs/smb/server/vfs_cache.c b/fs/smb/server/vfs_cache.c
--- a/fs/smb/server/vfs_cache.c
+++ b/fs/smb/server/vfs_cache.c
@@ -11,6 +11,7 @@ 
 #include <linux/kthread.h>
 #include <linux/freezer.h>
 #include <linux/dcache.h>
+#include <linux/namei.h>
 
 #include "glob.h"
 #include "vfs_cache.h"
@@ -1701,38 +1702,98 @@ 
 int ksmbd_validate_name_reconnect(struct ksmbd_share_config *share,
 				  struct ksmbd_file *fp, char *name)
 {
-	char *pathname, *ab_pathname;
+	const struct path *root = &share->vfs_path;
+	struct path cur;
+	const char *p, *end;
+	unsigned int seq, attempts = 0, hops;
 	int ret = 0;
 
-	pathname = kmalloc(PATH_MAX, KSMBD_DEFAULT_GFP);
-	if (!pathname)
-		return -EACCES;
-
-	ab_pathname = d_path(&fp->filp->f_path, pathname, PATH_MAX);
-	if (IS_ERR(ab_pathname)) {
-		kfree(pathname);
-		return -EACCES;
-	}
-
-	if (name) {
-		size_t len = strlen(ab_pathname);
-
-		if (len == share->path_sz && !strncmp(ab_pathname, share->path, len)) {
-			/* the durable fp is the share root itself */
-			if (name[0])
+	if (!name)
+		return 0;
+	if (name[0] == '/' || d_unlinked(fp->filp->f_path.dentry))
+		return -EINVAL;
+
+	for (;;) {
+		if (++attempts > 16)
+			return -EAGAIN;
+		ret = 0;
+		p = name;
+		end = name + strlen(name);
+		cur = fp->filp->f_path;
+		path_get(&cur);
+		seq = read_seqbegin(&rename_lock);
+		hops = 0;
+		for (;;) {
+			const char *comp;
+			size_t clen;
+			struct dentry *parent;
+
+			/* Bound the walk so mount/rename storms cannot spin forever. */
+			if (++hops > 8192) {
+				ret = -EAGAIN;
+				break;
+			}
+
+			if (cur.dentry == root->dentry &&
+			    cur.mnt == root->mnt) {
+				/* the share root itself only matches an empty name */
+				if (end != p)
+					ret = -EINVAL;
+				break;
+			}
+
+			if (cur.dentry == cur.mnt->mnt_root) {
+				/*
+				 * Mount root: hop into the parent mount and
+				 * let its mountpoint supply the component.
+				 * Mount topology can change mid-walk
+				 * (mount_lock is not available here), but
+				 * racing that needs local mount privileges.
+				 */
+				if (!follow_up(&cur)) {
+					ret = -EINVAL;
+					break;
+				}
+				continue;
+			}
+
+			for (comp = end; comp > p && comp[-1] != '/'; comp--)
+				;
+			clen = end - comp;
+			if (!clen) {
+				/* name exhausted while the fp is deeper, or empty component */
 				ret = -EINVAL;
-		} else if (len <= share->path_sz ||
-			   strncmp(ab_pathname, share->path, share->path_sz) ||
-			   ab_pathname[share->path_sz] != '/' ||
-			   strcmp(&ab_pathname[share->path_sz + 1], name)) {
-			ret = -EINVAL;
-		}
-		if (ret)
-			ksmbd_debug(SMB, "invalid name reconnect %s\n", name);
-	}
+				break;
+			}
 
-	kfree(pathname);
+			spin_lock(&cur.dentry->d_lock);
+			if (cur.dentry->d_name.len != clen ||
+			    memcmp(cur.dentry->d_name.name, comp, clen))
+				ret = -EINVAL;
+			spin_unlock(&cur.dentry->d_lock);
+			if (ret)
+				break;
+
+			parent = dget_parent(cur.dentry);
+			if (parent == cur.dentry) {
+				/* escaped the mount without reaching the share root */
+				dput(parent);
+				ret = -EINVAL;
+				break;
+			}
+			dput(cur.dentry);
+			cur.dentry = parent;
 
+			end = comp;
+			if (end > p && end[-1] == '/')
+				end--;
+		}
+		path_put(&cur);
+		if (!read_seqretry(&rename_lock, seq))
+			break;
+	}
+	if (ret)
+		ksmbd_debug(SMB, "invalid name reconnect %s\n", name);
 	return ret;
 }