[PATCH 0/2] smb: client: fix DFS handling of non-ASCII paths with non-utf8 charset

[PATCH 0/2] smb: client: fix DFS handling of non-ASCII paths with non-utf8 charset

[Fredric Cover]

Hi Paulo,

This series fixes two problems that show up when a DFS mount uses a
non-utf8 charset (iocharset=iso8859-1, or a kernel built with a
non-utf8 CONFIG_NLS_DEFAULT) and the DFS path has non-ASCII
characters.

Patch 1 fixes an out-of-bounds read in dfs_parse_target_referral().
path_consumed is a utf8 byte count (the DFS cache works in utf8), but
it is applied to full_path, which is in the mount charset.  When the
count exceeds strlen(full_path), the unsigned subtraction wraps and
prepath points past the terminating NUL.

Patch 2 fixes a double conversion in setup_dfs_ref().  The path is
converted to utf8, then converted again by dfs_cache_find(), so
"é" is sent to the server as c3 83 c2 a9 and the second-level
referral fails with -ENOENT.

Order matters.  Patch 2 lets these mounts get past the second-level
referral, which makes the over-read from patch 1 reachable.  Applied
in the other order, patch 2 would turn a clean -ENOENT into an
out-of-bounds read (a KASAN report on KASAN kernels).

Testing: Samba 4.23 DFS root with a link named "café", client on a
KASAN kernel, with iocharset=iso8859-1 and the raw 0xe9 byte in the
mount path.
  - On an unmodified tree the mount fails with -ENOENT.
  - With only the setup_dfs_ref() change applied: KASAN
    slab-out-of-bounds read in dfs_parse_target_referral()
    (strlen(full_path)=29, path_consumed=30).
  - With both patches: the mount succeeds, no KASAN report.
  - Regression run: iocharset=utf8 with "café" mounts as before.

Known limitation, not addressed here: when a non-ASCII link is
followed by a path component (e.g. //srv/dfs/café/sub), path_consumed
is still in utf8 bytes while full_path is in the mount charset, so the
prefix path is offset incorrectly.  I saw this as -ENOENT.  Patch 1
only prevents the out-of-bounds access.  Fixing it needs the
consumed length converted to mount-charset units, which changes the
dfs_parse_target_referral() interface, so I haven't attempted it.
Paulo, how would you like that handled?

I haven't added Fixes tags.  The strlen() - path_consumed arithmetic
was moved by commit abdb1742a312 ("cifs: get rid of mount options
string parsing") and appears to date back further, and the double
conversion in setup_dfs_ref() also predates 5433c629e8d4, which only
moved the call. I couldn't pin down where either became reachable, so
I'd appreciate it if you could add the right tags. I haven't added
stable tags either; let me know if you'd like them.

Fredric Cover (2):
  smb: client: fix out-of-bounds read in dfs_parse_target_referral()
  smb: client: fix double conversion of DFS path in setup_dfs_ref()

 fs/smb/client/dfs.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

-- 
2.53.0

[PATCH 1/2] smb: client: fix out-of-bounds read in dfs_parse_target_referral()

[Fredric Cover]

dfs_parse_target_referral() decides whether a prefix path remains after
the consumed part of full_path with:

	if (strlen(full_path) - ref->path_consumed)

strlen() returns size_t, so if path_consumed is larger than the length
of full_path, the subtraction wraps around to a huge non-zero value.
The branch is then taken, prepath is set to a pointer past the
terminating NUL of full_path, and *prepath reads out of bounds.

path_consumed can exceed strlen(full_path) when the mount charset is
not utf8.  The referral code derives path_consumed from the canonical
path, which is in the DFS cache charset (utf8), but it is applied here
to full_path, which is in the mount charset.  Each non-ASCII character
in the consumed part of the path that is longer in utf8 than in the
mount charset makes the count too large by the difference.

This was reproduced with iocharset=iso8859-1 against a Samba DFS root
containing a link named "café", mounting with the single Latin-1 byte
0xe9 in the path.  On an unmodified tree such a mount fails earlier
with -ENOENT because the path is converted to utf8 twice in
setup_dfs_ref(); with that fixed ("smb: client: fix double conversion
of DFS path in setup_dfs_ref()"), the mount reaches this code.
full_path was 29 bytes while path_consumed was 30:

  BUG: KASAN: slab-out-of-bounds in dfs_parse_target_referral+0x26a/0x270
  Read of size 1 at addr ffff88810db89d5f by task mount/1743
  ...
  The buggy address is located 0 bytes to the right of
   allocated 31-byte region [ffff88810db89d40, ffff88810db89d5f)

Compare path_consumed against the string length instead of subtracting,
so that no prefix path is used when the consumed count covers the whole
string.  With the bounds check in place the same mount succeeds and no
KASAN report is produced.

This only prevents the out-of-bounds access.  path_consumed is still in
the wrong units, so a non-ASCII link followed by a path component can
still produce a wrong prefix path.  That needs a separate fix.

Signed-off-by: Fredric Cover <fredric.cover.lkernel@gmail.com>
---
 fs/smb/client/dfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/smb/client/dfs.c b/fs/smb/client/dfs.c
index f65a8a90ba27..e6dcaf9b8682 100644
--- a/fs/smb/client/dfs.c
+++ b/fs/smb/client/dfs.c
@@ -33,7 +33,7 @@ int dfs_parse_target_referral(const char *full_path, const struct dfs_info3_para
 	if (WARN_ON_ONCE(!ref->node_name || ref->path_consumed < 0))
 		return -EINVAL;
 
-	if (strlen(full_path) - ref->path_consumed) {
+	if ((size_t)ref->path_consumed < strlen(full_path)) {
 		prepath = full_path + ref->path_consumed;
 		/* skip initial delimiter */
 		if (*prepath == '/' || *prepath == '\\')
-- 
2.53.0

[PATCH 2/2] smb: client: fix double conversion of DFS path in setup_dfs_ref()

[Fredric Cover]

setup_dfs_ref() converts the path from the mount charset to the DFS
cache charset (utf8) via dfs_get_path(), and then passes that already
converted string to dfs_get_referral().  dfs_cache_find() converts its
input again, assuming it is still in the mount charset.

When the mount charset is not utf8 (iocharset=iso8859-1, or a kernel
built with a non-utf8 CONFIG_NLS_DEFAULT) and the DFS path contains
non-ASCII characters, the utf8 bytes are interpreted as mount-charset
characters and encoded a second time.  For example, "é" is sent as
c3 83 c2 a9 instead of c3 a9.  The server then fails the second-level
referral with STATUS_OBJECT_PATH_NOT_FOUND and the mount fails with
-ENOENT, even though the path exists.

Pass the unconverted path to dfs_get_referral() so that it is converted
exactly once.

Such mounts now get past the second-level referral and reach
dfs_parse_target_referral(), which is why this change depends on
"smb: client: fix out-of-bounds read in dfs_parse_target_referral()".

Tested with a Samba DFS root containing a link named "café", mounted
with iocharset=iso8859-1 and the single Latin-1 byte 0xe9 in the path:
before, the mount failed with -ENOENT; with both patches applied, it
succeeds.

Signed-off-by: Fredric Cover <fredric.cover.lkernel@gmail.com>
---
 fs/smb/client/dfs.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/fs/smb/client/dfs.c b/fs/smb/client/dfs.c
index e6dcaf9b8682..0660c0a1eefb 100644
--- a/fs/smb/client/dfs.c
+++ b/fs/smb/client/dfs.c
@@ -103,6 +103,7 @@ static int setup_dfs_ref(struct dfs_info3_param *tgt, struct dfs_ref_walk *rw)
 	struct cifs_sb_info *cifs_sb = rw->mnt_ctx->cifs_sb;
 	struct smb3_fs_context *ctx = rw->mnt_ctx->fs_ctx;
 	char *ref_path, *full_path;
+	const char *raw_path;
 	int rc;
 
 	set_root_smb_session(rw->mnt_ctx);
@@ -114,9 +115,10 @@ static int setup_dfs_ref(struct dfs_info3_param *tgt, struct dfs_ref_walk *rw)
 
 	if (!tgt || (tgt->server_type == DFS_TYPE_LINK &&
 		     DFS_INTERLINK(tgt->flags)))
-		ref_path = dfs_get_path(cifs_sb, ctx->UNC);
+		raw_path = ctx->UNC;
 	else
-		ref_path = dfs_get_path(cifs_sb, full_path);
+		raw_path = full_path;
+	ref_path = dfs_get_path(cifs_sb, raw_path);
 	if (IS_ERR(ref_path)) {
 		rc = PTR_ERR(ref_path);
 		kfree(full_path);
@@ -125,9 +127,13 @@ static int setup_dfs_ref(struct dfs_info3_param *tgt, struct dfs_ref_walk *rw)
 	ref_walk_path(rw) = ref_path;
 	ref_walk_fpath(rw) = full_path;
 
-	return dfs_get_referral(rw->mnt_ctx,
-				ref_walk_path(rw) + 1,
-				ref_walk_tl(rw));
+	/*
+	 * dfs_cache_find() canonicalizes its input from the mount's charset, so
+	 * give it the raw path. Passing ref_walk_path() (already in the cache
+	 * charset) would convert it a second time.
+	 */
+
+	return dfs_get_referral(rw->mnt_ctx, raw_path + 1, ref_walk_tl(rw));
 }
 
 static int __dfs_referral_walk(struct dfs_ref_walk *rw)
-- 
2.53.0