diff mbox series

[PATCHv2] smb3: allow files to be created with backslash in name

Message ID CAH2r5mu+LPjja0TqNaDq6yA3GSy0=uBryMXAd4RTZOWinHOkOA@mail.gmail.com
State New
Headers show
Series [PATCHv2] smb3: allow files to be created with backslash in name | expand

Commit Message

Steve French Dec. 15, 2023, 8:29 p.m. UTC
Updated patch (rebased to current for-next-next)

Backslash is reserved in Windows (and SMB2/SMB3 by default) but
allowed in POSIX so must be remapped when POSIX extensions are
not enabled.

The default mapping for SMB3 mounts ("SFM") allows mapping backslash
(ie 0x5C in UTF8) to 0xF026 in UCS-2 (using the Unicode remapping
range reserved for these characters), but this was not mapped by
cifs.ko (unlike asterisk, greater than, question mark etc).  This patch
fixes that to allow creating files and directories with backslash
in the file or directory name.

Before this patch:
   touch "/mnt2/filewith\slash"
would return
   touch: setting times of '/mnt2/filewith\slash': Invalid argument

With the patch touch and mkdir with the backslash in the name works.

This problem was found while debugging xfstest generic/453
    https://bugzilla.kernel.org/show_bug.cgi?id=210961

Reported-by: Xiaoli Feng <xifeng@redhat.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210961
Signed-off-by: Steve French <stfrench@microsoft.com>


@Paulo Alcantara  any thoughts if additional changes needed for DFS or
prefixpaths?

Comments

Paulo Alcantara Dec. 22, 2023, 5:58 p.m. UTC | #1
Steve French <smfrench@gmail.com> writes:

> Updated patch (rebased to current for-next-next)
>
> Backslash is reserved in Windows (and SMB2/SMB3 by default) but
> allowed in POSIX so must be remapped when POSIX extensions are
> not enabled.
>
> The default mapping for SMB3 mounts ("SFM") allows mapping backslash
> (ie 0x5C in UTF8) to 0xF026 in UCS-2 (using the Unicode remapping
> range reserved for these characters), but this was not mapped by
> cifs.ko (unlike asterisk, greater than, question mark etc).  This patch
> fixes that to allow creating files and directories with backslash
> in the file or directory name.
>
> Before this patch:
>    touch "/mnt2/filewith\slash"
> would return
>    touch: setting times of '/mnt2/filewith\slash': Invalid argument
>
> With the patch touch and mkdir with the backslash in the name works.
>
> This problem was found while debugging xfstest generic/453
>     https://bugzilla.kernel.org/show_bug.cgi?id=210961
>
> Reported-by: Xiaoli Feng <xifeng@redhat.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210961
> Signed-off-by: Steve French <stfrench@microsoft.com>
>
>
> @Paulo Alcantara  any thoughts if additional changes needed for DFS or
> prefixpaths?

Yes - these changes break DFS mounts with iocharsets different than
utf8.  Consider dfs_cache_canonical_path() where the backslashes will
get remapped in cifs_strndup_to_utf16() and then broken DFS referral
paths will be sent over the wire.

You can simply reproduce this with

        mount.cifs //srv/dfs /mnt -o ...,iocharset=ascii

> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
> index 55b3ce944022..e6f4a28275a8 100644
> --- a/fs/smb/client/cifsglob.h
> +++ b/fs/smb/client/cifsglob.h
> @@ -1568,10 +1568,7 @@ CIFS_FILE_SB(struct file *file)
>  
>  static inline char CIFS_DIR_SEP(const struct cifs_sb_info *cifs_sb)
>  {
> -	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS)
> -		return '/';
> -	else
> -		return '\\';
> +	return '/';
>  }

This change breakes readdir(2) under SMB1 mounts (no UNIX extensions)
because CIFSFindFirst() ends up appending "/*" rather than "\\*" to
filename and then fails with STATUS_OBJECT_NAME_INVALID.

You'd need to check all other places where we could also append an UTF16
string with CIFS_DIR_SEP() after getting the path with
cifs_convert_path_to_utf16().

> diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
> index 580a27a3a7e6..6c446b1210ba 100644
> --- a/fs/smb/client/dir.c
> +++ b/fs/smb/client/dir.c
> @@ -160,12 +160,18 @@ check_name(struct dentry *direntry, struct cifs_tcon *tcon)
>  		     le32_to_cpu(tcon->fsAttrInfo.MaxPathNameComponentLength)))
>  		return -ENAMETOOLONG;
>  
> -	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS)) {
> -		for (i = 0; i < direntry->d_name.len; i++) {
> -			if (direntry->d_name.name[i] == '\\') {
> -				cifs_dbg(FYI, "Invalid file name\n");
> -				return -EINVAL;
> -			}
> +	/*
> +	 * SMB3.1.1 POSIX Extensions, CIFS Unix Extensions and SFM mappings
> +	 * allow \ in paths (or in latter case remaps \ to 0xF026)
> +	 */
> +	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) ||
> +	    (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SFM_CHR))

What about

        if (cifs_sb->mnt_cifs_flags & (CIFS_MOUNT_POSIX_PATHS | CIFS_MOUNT_MAP_SFM_CHR))

> diff --git a/fs/smb/client/smb2misc.c b/fs/smb/client/smb2misc.c
> index e20b4354e703..0edc888ec3f8 100644
> --- a/fs/smb/client/smb2misc.c
> +++ b/fs/smb/client/smb2misc.c
> @@ -469,13 +469,17 @@ cifs_convert_path_to_utf16(const char *from, struct cifs_sb_info *cifs_sb)
>  	if (from[0] == '\\')
>  		start_of_path = from + 1;
>  
> -	/* SMB311 POSIX extensions paths do not include leading slash */
> -	else if (cifs_sb_master_tlink(cifs_sb) &&
> -		 cifs_sb_master_tcon(cifs_sb)->posix_extensions &&
> -		 (from[0] == '/')) {
> -		start_of_path = from + 1;
> -	} else
> -		start_of_path = from;
> +	start_of_path = from;
> +	/*
> +	 * Only old CIFS Unix extensions paths include leading slash
> +	 * Need to skip if for SMB3.1.1 POSIX Extensions and SMB1/2/3
> +	 */
> +	if (from[0] == '/') {
> +		if (((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) == false) ||

I guess you meant "== 0".  Also, no need for extra parentheses.
diff mbox series

Patch

From ab1e7cf272e913d537b9600453bc8e5ccb26ee6d Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Fri, 15 Dec 2023 14:25:02 -0600
Subject: [PATCH] smb3: allow files to be created with backslash in name

Backslash is reserved in Windows (and SMB2/SMB3 by default) but
allowed in POSIX so must be remapped when POSIX extensions are
not enabled.

The default mapping for SMB3 mounts ("SFM") allows mapping backslash
(ie 0x5C in UTF8) to 0xF026 in UCS-2 (using the Unicode remapping
range reserved for these characters), but this was not mapped by
cifs.ko (unlike asterisk, greater than, question mark etc).  This patch
fixes that to allow creating files and directories with backslash
in the file or directory name.

Before this patch:
   touch "/mnt2/filewith\slash"
would return
   touch: setting times of '/mnt2/filewith\slash': Invalid argument

With the patch touch and mkdir with the backslash in the name works.

This problem was found while debugging xfstest generic/453
    https://bugzilla.kernel.org/show_bug.cgi?id=210961

Reported-by: Xiaoli Feng <xifeng@redhat.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=210961
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/smb/client/cifs_unicode.c | 15 ++++++++++-----
 fs/smb/client/cifs_unicode.h |  3 +++
 fs/smb/client/cifsglob.h     |  5 +----
 fs/smb/client/dir.c          | 18 ++++++++++++------
 fs/smb/client/smb2misc.c     | 18 +++++++++++-------
 5 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/fs/smb/client/cifs_unicode.c b/fs/smb/client/cifs_unicode.c
index 79d99a913944..02966853419f 100644
--- a/fs/smb/client/cifs_unicode.c
+++ b/fs/smb/client/cifs_unicode.c
@@ -96,6 +96,9 @@  convert_sfm_char(const __u16 src_char, char *target)
 	case SFM_PERIOD:
 		*target = '.';
 		break;
+	case SFM_SLASH:
+		*target = '\\';
+		break;
 	default:
 		return false;
 	}
@@ -424,6 +427,9 @@  static __le16 convert_to_sfm_char(char src_char, bool end_of_string)
 	case '|':
 		dest_char = cpu_to_le16(SFM_PIPE);
 		break;
+	case '\\':
+		dest_char = cpu_to_le16(SFM_SLASH);
+		break;
 	case '.':
 		if (end_of_string)
 			dest_char = cpu_to_le16(SFM_PERIOD);
@@ -436,6 +442,9 @@  static __le16 convert_to_sfm_char(char src_char, bool end_of_string)
 		else
 			dest_char = 0;
 		break;
+	case '/':
+		dest_char = cpu_to_le16(UCS2_SLASH);
+		break;
 	default:
 		dest_char = 0;
 	}
@@ -495,11 +504,7 @@  cifsConvertToUTF16(__le16 *target, const char *source, int srclen,
 			dst_char = convert_to_sfm_char(src_char, end_of_string);
 		} else
 			dst_char = 0;
-		/*
-		 * FIXME: We can not handle remapping backslash (UNI_SLASH)
-		 * until all the calls to build_path_from_dentry are modified,
-		 * as they use backslash as separator.
-		 */
+
 		if (dst_char == 0) {
 			charlen = cp->char2uni(source + i, srclen - i, &tmp);
 			dst_char = cpu_to_le16(tmp);
diff --git a/fs/smb/client/cifs_unicode.h b/fs/smb/client/cifs_unicode.h
index e137a0dfbbe9..4d414966f504 100644
--- a/fs/smb/client/cifs_unicode.h
+++ b/fs/smb/client/cifs_unicode.h
@@ -23,6 +23,9 @@ 
 #include <linux/nls.h>
 #include "../../nls/nls_ucs2_utils.h"
 
+/* Unicode encoding of backslash character */
+#define UCS2_SLASH 0x005C
+
 /*
  * Macs use an older "SFM" mapping of the symbols above. Fortunately it does
  * not conflict (although almost does) with the mapping above.
diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index 55b3ce944022..e6f4a28275a8 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1568,10 +1568,7 @@  CIFS_FILE_SB(struct file *file)
 
 static inline char CIFS_DIR_SEP(const struct cifs_sb_info *cifs_sb)
 {
-	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS)
-		return '/';
-	else
-		return '\\';
+	return '/';
 }
 
 static inline void
diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
index 580a27a3a7e6..6c446b1210ba 100644
--- a/fs/smb/client/dir.c
+++ b/fs/smb/client/dir.c
@@ -160,12 +160,18 @@  check_name(struct dentry *direntry, struct cifs_tcon *tcon)
 		     le32_to_cpu(tcon->fsAttrInfo.MaxPathNameComponentLength)))
 		return -ENAMETOOLONG;
 
-	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS)) {
-		for (i = 0; i < direntry->d_name.len; i++) {
-			if (direntry->d_name.name[i] == '\\') {
-				cifs_dbg(FYI, "Invalid file name\n");
-				return -EINVAL;
-			}
+	/*
+	 * SMB3.1.1 POSIX Extensions, CIFS Unix Extensions and SFM mappings
+	 * allow \ in paths (or in latter case remaps \ to 0xF026)
+	 */
+	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) ||
+	    (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SFM_CHR))
+		return 0;
+
+	for (i = 0; i < direntry->d_name.len; i++) {
+		if (direntry->d_name.name[i] == '\\') {
+			cifs_dbg(FYI, "Invalid file name\n");
+			return -EINVAL;
 		}
 	}
 	return 0;
diff --git a/fs/smb/client/smb2misc.c b/fs/smb/client/smb2misc.c
index e20b4354e703..0edc888ec3f8 100644
--- a/fs/smb/client/smb2misc.c
+++ b/fs/smb/client/smb2misc.c
@@ -469,13 +469,17 @@  cifs_convert_path_to_utf16(const char *from, struct cifs_sb_info *cifs_sb)
 	if (from[0] == '\\')
 		start_of_path = from + 1;
 
-	/* SMB311 POSIX extensions paths do not include leading slash */
-	else if (cifs_sb_master_tlink(cifs_sb) &&
-		 cifs_sb_master_tcon(cifs_sb)->posix_extensions &&
-		 (from[0] == '/')) {
-		start_of_path = from + 1;
-	} else
-		start_of_path = from;
+	start_of_path = from;
+	/*
+	 * Only old CIFS Unix extensions paths include leading slash
+	 * Need to skip if for SMB3.1.1 POSIX Extensions and SMB1/2/3
+	 */
+	if (from[0] == '/') {
+		if (((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) == false) ||
+		    (cifs_sb_master_tlink(cifs_sb) &&
+		     (cifs_sb_master_tcon(cifs_sb)->posix_extensions)))
+			start_of_path = from + 1;
+	}
 
 	to = cifs_strndup_to_utf16(start_of_path, PATH_MAX, &len,
 				   cifs_sb->local_nls, map_type);
-- 
2.40.1