[PATCH] smb: client: fix native SMB symlink traversal

[PATCH] smb: client: fix native SMB symlink traversal

[Paulo Alcantara]

We've seen customers having shares mounted in paths like /??/C:/ or
/??/UNC/foo.example.com/share in order to get their native SMB
symlinks successfully followed from different mounts.

After commit 12b466eb52d9 ("cifs: Fix creating and resolving absolute NT-style symlinks"),
the client would then convert absolute paths from /??/C:/ to "/mnt/c/"
by default.  The absolute paths would vary depending on the value of
symlink= mount option.

Fix this by restoring old behavior of not trying to convert absolute
paths by default.  Only do this if symlinkroot= was _explicitly_ set.

Before patch:

  $ mount.cifs //w22-fs0/test2 /mnt/1 -o vers=3.1.1,username=xxx,password=yyy
  $ ls -l /mnt/1/symlink2
  lrwxr-xr-x 1 root root 15 Jun 20 14:22 /mnt/1/symlink2 -> /mnt/c/testfile
  $ mkdir -p /??/C:; echo foo > //??/C:/testfile
  $ cat /mnt/1/symlink2
  cat: /mnt/1/symlink2: No such file or directory

After patch:

  $ mount.cifs //w22-fs0/test2 /mnt/1 -o vers=3.1.1,username=xxx,password=yyy
  $ ls -l /mnt/1/symlink2
  lrwxr-xr-x 1 root root 15 Jun 20 14:22 /mnt/1/symlink2 -> '/??/C:/testfile'
  $ mkdir -p /??/C:; echo foo > //??/C:/testfile
  $ cat /mnt/1/symlink2
  foo

Cc: linux-cifs@vger.kernel.org
Cc: Pierguido Lambri <plambri@redhat.com>
Cc: David Howells <dhowells@redhat.com>
Fixes: 12b466eb52d9 ("cifs: Fix creating and resolving absolute NT-style symlinks")
Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.org>
---
 fs/smb/client/fs_context.c | 13 ++++---------
 fs/smb/client/reparse.c    | 22 +++++++++++++---------
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index a634a34d4086..d8d2d4a739e8 100644
--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -1825,9 +1825,11 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
 			goto cifs_parse_mount_err;
 		}
 		kfree(ctx->symlinkroot);
-		ctx->symlinkroot = kstrdup(param->string, GFP_KERNEL);
-		if (!ctx->symlinkroot)
+		ctx->symlinkroot = kstrndup(param->string, PATH_MAX, GFP_KERNEL);
+		if (!ctx->symlinkroot) {
+			cifs_errorf(fc, "OOM when copying symlinkroot string\n");
 			goto cifs_parse_mount_err;
+		}
 		break;
 	}
 	/* case Opt_ignore: - is ignored as expected ... */
@@ -1837,13 +1839,6 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
 		goto cifs_parse_mount_err;
 	}
 
-	/*
-	 * By default resolve all native absolute symlinks relative to "/mnt/".
-	 * Same default has drvfs driver running in WSL for resolving SMB shares.
-	 */
-	if (!ctx->symlinkroot)
-		ctx->symlinkroot = kstrdup("/mnt/", GFP_KERNEL);
-
 	return 0;
 
  cifs_parse_mount_err:
diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c
index 1c40e42e4d89..5fa29a97ac15 100644
--- a/fs/smb/client/reparse.c
+++ b/fs/smb/client/reparse.c
@@ -57,6 +57,7 @@ static int create_native_symlink(const unsigned int xid, struct inode *inode,
 	struct reparse_symlink_data_buffer *buf = NULL;
 	struct cifs_open_info_data data = {};
 	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
+	const char *symroot = cifs_sb->ctx->symlinkroot;
 	struct inode *new;
 	struct kvec iov;
 	__le16 *path = NULL;
@@ -82,7 +83,8 @@ static int create_native_symlink(const unsigned int xid, struct inode *inode,
 		.symlink_target = symlink_target,
 	};
 
-	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) && symname[0] == '/') {
+	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) &&
+	    symroot && symname[0] == '/') {
 		/*
 		 * This is a request to create an absolute symlink on the server
 		 * which does not support POSIX paths, and expects symlink in
@@ -92,7 +94,7 @@ static int create_native_symlink(const unsigned int xid, struct inode *inode,
 		 * ensure compatibility of this symlink stored in absolute form
 		 * on the SMB server.
 		 */
-		if (!strstarts(symname, cifs_sb->ctx->symlinkroot)) {
+		if (!strstarts(symname, symroot)) {
 			/*
 			 * If the absolute Linux symlink target path is not
 			 * inside "symlinkroot" location then there is no way
@@ -101,12 +103,12 @@ static int create_native_symlink(const unsigned int xid, struct inode *inode,
 			cifs_dbg(VFS,
 				 "absolute symlink '%s' cannot be converted to NT format "
 				 "because it is outside of symlinkroot='%s'\n",
-				 symname, cifs_sb->ctx->symlinkroot);
+				 symname, symroot);
 			rc = -EINVAL;
 			goto out;
 		}
-		len = strlen(cifs_sb->ctx->symlinkroot);
-		if (cifs_sb->ctx->symlinkroot[len-1] != '/')
+		len = strlen(symroot);
+		if (symroot[len - 1] != '/')
 			len++;
 		if (symname[len] >= 'a' && symname[len] <= 'z' &&
 		    (symname[len+1] == '/' || symname[len+1] == '\0')) {
@@ -782,6 +784,7 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
 			      const char *full_path,
 			      struct cifs_sb_info *cifs_sb)
 {
+	const char *symroot = cifs_sb->ctx->symlinkroot;
 	char sep = CIFS_DIR_SEP(cifs_sb);
 	char *linux_target = NULL;
 	char *smb_target = NULL;
@@ -815,7 +818,8 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
 		goto out;
 	}
 
-	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) && !relative) {
+	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) &&
+	    symroot && !relative) {
 		/*
 		 * This is an absolute symlink from the server which does not
 		 * support POSIX paths, so the symlink is in NT-style path.
@@ -907,15 +911,15 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
 		}
 
 		abs_path_len = strlen(abs_path)+1;
-		symlinkroot_len = strlen(cifs_sb->ctx->symlinkroot);
-		if (cifs_sb->ctx->symlinkroot[symlinkroot_len-1] == '/')
+		symlinkroot_len = strlen(symroot);
+		if (symroot[symlinkroot_len - 1] == '/')
 			symlinkroot_len--;
 		linux_target = kmalloc(symlinkroot_len + 1 + abs_path_len, GFP_KERNEL);
 		if (!linux_target) {
 			rc = -ENOMEM;
 			goto out;
 		}
-		memcpy(linux_target, cifs_sb->ctx->symlinkroot, symlinkroot_len);
+		memcpy(linux_target, symroot, symlinkroot_len);
 		linux_target[symlinkroot_len] = '/';
 		memcpy(linux_target + symlinkroot_len + 1, abs_path, abs_path_len);
 	} else if (smb_target[0] == sep && relative) {
-- 
2.50.0

Re: [PATCH] smb: client: fix native SMB symlink traversal

[Stefan Metzmacher]

Am 02.07.25 um 19:40 schrieb Paulo Alcantara:
> We've seen customers having shares mounted in paths like /??/C:/ or
> /??/UNC/foo.example.com/share in order to get their native SMB
> symlinks successfully followed from different mounts.
> 
> After commit 12b466eb52d9 ("cifs: Fix creating and resolving absolute NT-style symlinks"),
> the client would then convert absolute paths from /??/C:/ to "/mnt/c/"
> by default.  The absolute paths would vary depending on the value of
> symlink= mount option.
> 
> Fix this by restoring old behavior of not trying to convert absolute
> paths by default.  Only do this if symlinkroot= was _explicitly_ set.
> 
> Before patch:
> 
>    $ mount.cifs //w22-fs0/test2 /mnt/1 -o vers=3.1.1,username=xxx,password=yyy
>    $ ls -l /mnt/1/symlink2
>    lrwxr-xr-x 1 root root 15 Jun 20 14:22 /mnt/1/symlink2 -> /mnt/c/testfile
>    $ mkdir -p /??/C:; echo foo > //??/C:/testfile
>    $ cat /mnt/1/symlink2
>    cat: /mnt/1/symlink2: No such file or directory
> 
> After patch:
> 
>    $ mount.cifs //w22-fs0/test2 /mnt/1 -o vers=3.1.1,username=xxx,password=yyy
>    $ ls -l /mnt/1/symlink2
>    lrwxr-xr-x 1 root root 15 Jun 20 14:22 /mnt/1/symlink2 -> '/??/C:/testfile'
>    $ mkdir -p /??/C:; echo foo > //??/C:/testfile
>    $ cat /mnt/1/symlink2
>    foo
> 
> Cc: linux-cifs@vger.kernel.org
> Cc: Pierguido Lambri <plambri@redhat.com>
> Cc: David Howells <dhowells@redhat.com>
> Fixes: 12b466eb52d9 ("cifs: Fix creating and resolving absolute NT-style symlinks")
> Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.org>
> ---
>   fs/smb/client/fs_context.c | 13 ++++---------
>   fs/smb/client/reparse.c    | 22 +++++++++++++---------
>   2 files changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> index a634a34d4086..d8d2d4a739e8 100644
> --- a/fs/smb/client/fs_context.c
> +++ b/fs/smb/client/fs_context.c
> @@ -1825,9 +1825,11 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
>   			goto cifs_parse_mount_err;
>   		}
>   		kfree(ctx->symlinkroot);
> -		ctx->symlinkroot = kstrdup(param->string, GFP_KERNEL);
> -		if (!ctx->symlinkroot)
> +		ctx->symlinkroot = kstrndup(param->string, PATH_MAX, GFP_KERNEL);

Should we really truncate the string instead of generating an error?
I really don't know, maybe it is a good thing, but we should have a comment
that explains it why we truncate.

Thanks!
metze

Re: [PATCH] smb: client: fix native SMB symlink traversal

[Paulo Alcantara]

Stefan Metzmacher <metze@samba.org> writes:

> Am 02.07.25 um 19:40 schrieb Paulo Alcantara:
>> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
>> index a634a34d4086..d8d2d4a739e8 100644
>> --- a/fs/smb/client/fs_context.c
>> +++ b/fs/smb/client/fs_context.c
>> @@ -1825,9 +1825,11 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
>>   			goto cifs_parse_mount_err;
>>   		}
>>   		kfree(ctx->symlinkroot);
>> -		ctx->symlinkroot = kstrdup(param->string, GFP_KERNEL);
>> -		if (!ctx->symlinkroot)
>> +		ctx->symlinkroot = kstrndup(param->string, PATH_MAX, GFP_KERNEL);
>
> Should we really truncate the string instead of generating an error?
> I really don't know, maybe it is a good thing, but we should have a comment
> that explains it why we truncate.

Good point.  We should bail out and then print an error in case the path
set in symlinkroot= is greater than PATH_MAX.  Will send v2 soon with
the fix, thanks.