Re: [RFC] odd check in ceph_encode_encrypted_dname()

[Al Viro <viro@zeniv.linux.org.uk>]

On Tue, Feb 18, 2025 at 01:21:32AM +0000, Al Viro wrote:

> See the problem?  strrchr() expects a NUL-terminated string; giving it an
> array that has no zero bytes in it is an UB.
> 
> That one is -stable fodder on its own, IMO...

FWIW, it's more unpleasant; there are other call chains for parse_longname()
where it's not feasible to NUL-terminate in place.  I suspect that the
patch below is a better way to handle that.  Comments?

From ed016e5ea89550b567306207ba3ca8b60e147d89 Mon Sep 17 00:00:00 2001
From: Al Viro <viro@zeniv.linux.org.uk>
Date: Tue, 18 Feb 2025 17:57:17 -0500
Subject: [PATCH 1/3] [ceph] parse_longname(): strrchr() expects NUL-terminated
 string

... and parse_longname() is not guaranteed that.  That's the reason
why it uses kmemdup_nul() to build the argument for kstrtou64();
the problem is, kstrtou64() is not the only thing that need it.

Just get a NUL-terminated copy of the entire thing and be done
with that...

Fixes: dd66df0053ef "ceph: add support for encrypted snapshot names"
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/ceph/crypto.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index 3b3c4d8d401e..164e7981aecb 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -215,35 +215,30 @@ static struct inode *parse_longname(const struct inode *parent,
 	struct ceph_client *cl = ceph_inode_to_client(parent);
 	struct inode *dir = NULL;
 	struct ceph_vino vino = { .snap = CEPH_NOSNAP };
-	char *inode_number;
 	char *name_end;
-	int orig_len = *name_len;
 	int ret = -EIO;
-
+	/* NUL-terminate */
+	char *s __free(kfree) = kmemdup_nul(name, *name_len, GFP_KERNEL);
+	if (!s)
+		return ERR_PTR(-ENOMEM);
 	/* Skip initial '_' */
-	name++;
-	name_end = strrchr(name, '_');
+	s++;
+	name_end = strrchr(s, '_');
 	if (!name_end) {
-		doutc(cl, "failed to parse long snapshot name: %s\n", name);
+		doutc(cl, "failed to parse long snapshot name: %s\n", s);
 		return ERR_PTR(-EIO);
 	}
-	*name_len = (name_end - name);
+	*name_len = (name_end - s);
 	if (*name_len <= 0) {
 		pr_err_client(cl, "failed to parse long snapshot name\n");
 		return ERR_PTR(-EIO);
 	}
 
 	/* Get the inode number */
-	inode_number = kmemdup_nul(name_end + 1,
-				   orig_len - *name_len - 2,
-				   GFP_KERNEL);
-	if (!inode_number)
-		return ERR_PTR(-ENOMEM);
-	ret = kstrtou64(inode_number, 10, &vino.ino);
+	ret = kstrtou64(name_end + 1, 10, &vino.ino);
 	if (ret) {
-		doutc(cl, "failed to parse inode number: %s\n", name);
-		dir = ERR_PTR(ret);
-		goto out;
+		doutc(cl, "failed to parse inode number: %s\n", s);
+		return ERR_PTR(ret);
 	}
 
 	/* And finally the inode */
@@ -252,11 +247,8 @@ static struct inode *parse_longname(const struct inode *parent,
 		/* This can happen if we're not mounting cephfs on the root */
 		dir = ceph_get_inode(parent->i_sb, vino, NULL);
 		if (IS_ERR(dir))
-			doutc(cl, "can't find inode %s (%s)\n", inode_number, name);
+			doutc(cl, "can't find inode %s (%s)\n", name_end + 1, name);
 	}
-
-out:
-	kfree(inode_number);
 	return dir;
 }
 
-- 
2.39.5