[PATCHES] ceph d_name race fixes

[PATCHES] ceph d_name race fixes

[Al Viro]

	Series of race fixes for d_name handling in ceph had been posted
back in February, but apparently had fallen through the cracks - I expected
ceph folks to pull (or cherry-pick) it, they apparently thought I'd send
it to Linus and nobody checked what actually went down...

	I've rebased it to 6.16-rc1, with a couple of cosmetical changes
suggested back then.  Currently it's in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git work.ceph-d_name-fixes
Individual patches in followups.

	Folks, could you test and review it?  I really don't care which
tree would it go through, just let's make sure that everyone agrees who
pushes it out...

Shortlog:
Al Viro (3):
      [ceph] parse_longname(): strrchr() expects NUL-terminated string
      prep for ceph_encode_encrypted_fname() fixes
      ceph: fix a race with rename() in ceph_mdsc_build_path()

Diffstat:
 fs/ceph/caps.c       | 18 +++++-------
 fs/ceph/crypto.c     | 82 +++++++++++++++++-----------------------------------
 fs/ceph/crypto.h     | 18 +++---------
 fs/ceph/dir.c        |  7 ++---
 fs/ceph/mds_client.c |  4 +--
 5 files changed, 43 insertions(+), 86 deletions(-)

[PATCH 1/3] [ceph] parse_longname(): strrchr() expects NUL-terminated string

[Al Viro]

... 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 | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index 3b3c4d8d401e..9c7062245880 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -215,35 +215,31 @@ 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;
+	char *name_end, *inode_number;
 	int ret = -EIO;
-
+	/* NUL-terminate */
+	char *str __free(kfree) = kmemdup_nul(name, *name_len, GFP_KERNEL);
+	if (!str)
+		return ERR_PTR(-ENOMEM);
 	/* Skip initial '_' */
-	name++;
-	name_end = strrchr(name, '_');
+	str++;
+	name_end = strrchr(str, '_');
 	if (!name_end) {
-		doutc(cl, "failed to parse long snapshot name: %s\n", name);
+		doutc(cl, "failed to parse long snapshot name: %s\n", str);
 		return ERR_PTR(-EIO);
 	}
-	*name_len = (name_end - name);
+	*name_len = (name_end - str);
 	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);
+	inode_number = name_end + 1;
 	ret = kstrtou64(inode_number, 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", str);
+		return ERR_PTR(ret);
 	}
 
 	/* And finally the inode */
@@ -254,9 +250,6 @@ static struct inode *parse_longname(const struct inode *parent,
 		if (IS_ERR(dir))
 			doutc(cl, "can't find inode %s (%s)\n", inode_number, name);
 	}
-
-out:
-	kfree(inode_number);
 	return dir;
 }
 
-- 
2.39.5

[PATCH 2/3] prep for ceph_encode_encrypted_fname() fixes

[Al Viro]

ceph_encode_encrypted_dname() would be better off with plaintext name
already copied into buffer; we'll lift that into the callers on the
next step, which will allow to fix UAF on races with rename; for now
copy it in the very beginning of ceph_encode_encrypted_dname().

That has a pleasant side benefit - we don't need to mess with tmp_buf
anymore (i.e. that's 256 bytes off the stack footprint).

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/ceph/crypto.c | 40 +++++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 23 deletions(-)

diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index 9c7062245880..2aef56fc6275 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -258,31 +258,28 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
 {
 	struct ceph_client *cl = ceph_inode_to_client(parent);
 	struct inode *dir = parent;
-	struct qstr iname;
+	char *p = buf;
 	u32 len;
 	int name_len;
 	int elen;
 	int ret;
 	u8 *cryptbuf = NULL;
 
-	iname.name = d_name->name;
-	name_len = d_name->len;
+	memcpy(buf, d_name->name, d_name->len);
+	elen = d_name->len;
+
+	name_len = elen;
 
 	/* Handle the special case of snapshot names that start with '_' */
-	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
-	    (iname.name[0] == '_')) {
-		dir = parse_longname(parent, iname.name, &name_len);
+	if (ceph_snap(dir) == CEPH_SNAPDIR && *p == '_') {
+		dir = parse_longname(parent, p, &name_len);
 		if (IS_ERR(dir))
 			return PTR_ERR(dir);
-		iname.name++; /* skip initial '_' */
+		p++; /* skip initial '_' */
 	}
-	iname.len = name_len;
 
-	if (!fscrypt_has_encryption_key(dir)) {
-		memcpy(buf, d_name->name, d_name->len);
-		elen = d_name->len;
+	if (!fscrypt_has_encryption_key(dir))
 		goto out;
-	}
 
 	/*
 	 * Convert cleartext d_name to ciphertext. If result is longer than
@@ -290,7 +287,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
 	 *
 	 * See: fscrypt_setup_filename
 	 */
-	if (!fscrypt_fname_encrypted_size(dir, iname.len, NAME_MAX, &len)) {
+	if (!fscrypt_fname_encrypted_size(dir, name_len, NAME_MAX, &len)) {
 		elen = -ENAMETOOLONG;
 		goto out;
 	}
@@ -303,7 +300,9 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
 		goto out;
 	}
 
-	ret = fscrypt_fname_encrypt(dir, &iname, cryptbuf, len);
+	ret = fscrypt_fname_encrypt(dir,
+				    &(struct qstr)QSTR_INIT(p, name_len),
+				    cryptbuf, len);
 	if (ret) {
 		elen = ret;
 		goto out;
@@ -324,18 +323,13 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
 	}
 
 	/* base64 encode the encrypted name */
-	elen = ceph_base64_encode(cryptbuf, len, buf);
-	doutc(cl, "base64-encoded ciphertext name = %.*s\n", elen, buf);
+	elen = ceph_base64_encode(cryptbuf, len, p);
+	doutc(cl, "base64-encoded ciphertext name = %.*s\n", elen, p);
 
 	/* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
 	WARN_ON(elen > 240);
-	if ((elen > 0) && (dir != parent)) {
-		char tmp_buf[NAME_MAX];
-
-		elen = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
-				elen, buf, dir->i_ino);
-		memcpy(buf, tmp_buf, elen);
-	}
+	if (dir != parent) // leading _ is already there; append _<inum>
+		elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
 
 out:
 	kfree(cryptbuf);
-- 
2.39.5

[PATCH 3/3] ceph: fix a race with rename() in ceph_mdsc_build_path()

[Al Viro]

Lift copying the name into callers of ceph_encode_encrypted_dname()
that do not have it already copied; ceph_encode_encrypted_fname()
disappears.

That fixes a UAF in ceph_mdsc_build_path() - while the initial copy
of plaintext into buf is done under ->d_lock, we access the
original name again in ceph_encode_encrypted_fname() and that is
done without any locking.  With ceph_encode_encrypted_dname() using
the stable copy the problem goes away.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/ceph/caps.c       | 18 +++++++-----------
 fs/ceph/crypto.c     | 19 ++-----------------
 fs/ceph/crypto.h     | 18 ++++--------------
 fs/ceph/dir.c        |  7 +++----
 fs/ceph/mds_client.c |  4 ++--
 5 files changed, 18 insertions(+), 48 deletions(-)

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index a8d8b56cf9d2..b1a8ff612c41 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -4957,24 +4957,20 @@ int ceph_encode_dentry_release(void **p, struct dentry *dentry,
 	cl = ceph_inode_to_client(dir);
 	spin_lock(&dentry->d_lock);
 	if (ret && di->lease_session && di->lease_session->s_mds == mds) {
+		int len = dentry->d_name.len;
 		doutc(cl, "%p mds%d seq %d\n",  dentry, mds,
 		      (int)di->lease_seq);
 		rel->dname_seq = cpu_to_le32(di->lease_seq);
 		__ceph_mdsc_drop_dentry_lease(dentry);
+		memcpy(*p, dentry->d_name.name, len);
 		spin_unlock(&dentry->d_lock);
 		if (IS_ENCRYPTED(dir) && fscrypt_has_encryption_key(dir)) {
-			int ret2 = ceph_encode_encrypted_fname(dir, dentry, *p);
-
-			if (ret2 < 0)
-				return ret2;
-
-			rel->dname_len = cpu_to_le32(ret2);
-			*p += ret2;
-		} else {
-			rel->dname_len = cpu_to_le32(dentry->d_name.len);
-			memcpy(*p, dentry->d_name.name, dentry->d_name.len);
-			*p += dentry->d_name.len;
+			len = ceph_encode_encrypted_dname(dir, *p, len);
+			if (len < 0)
+				return len;
 		}
+		rel->dname_len = cpu_to_le32(len);
+		*p += len;
 	} else {
 		spin_unlock(&dentry->d_lock);
 	}
diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
index 2aef56fc6275..e312f52f48e4 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -253,23 +253,16 @@ static struct inode *parse_longname(const struct inode *parent,
 	return dir;
 }
 
-int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
-				char *buf)
+int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
 {
 	struct ceph_client *cl = ceph_inode_to_client(parent);
 	struct inode *dir = parent;
 	char *p = buf;
 	u32 len;
-	int name_len;
-	int elen;
+	int name_len = elen;
 	int ret;
 	u8 *cryptbuf = NULL;
 
-	memcpy(buf, d_name->name, d_name->len);
-	elen = d_name->len;
-
-	name_len = elen;
-
 	/* Handle the special case of snapshot names that start with '_' */
 	if (ceph_snap(dir) == CEPH_SNAPDIR && *p == '_') {
 		dir = parse_longname(parent, p, &name_len);
@@ -342,14 +335,6 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
 	return elen;
 }
 
-int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry,
-				char *buf)
-{
-	WARN_ON_ONCE(!fscrypt_has_encryption_key(parent));
-
-	return ceph_encode_encrypted_dname(parent, &dentry->d_name, buf);
-}
-
 /**
  * ceph_fname_to_usr - convert a filename for userland presentation
  * @fname: ceph_fname to be converted
diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
index d0768239a1c9..f752bbb2eb06 100644
--- a/fs/ceph/crypto.h
+++ b/fs/ceph/crypto.h
@@ -102,10 +102,7 @@ int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
 				 struct ceph_acl_sec_ctx *as);
 void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req,
 				struct ceph_acl_sec_ctx *as);
-int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
-				char *buf);
-int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry,
-				char *buf);
+int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int len);
 
 static inline int ceph_fname_alloc_buffer(struct inode *parent,
 					  struct fscrypt_str *fname)
@@ -194,17 +191,10 @@ static inline void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req,
 {
 }
 
-static inline int ceph_encode_encrypted_dname(struct inode *parent,
-					      struct qstr *d_name, char *buf)
+static inline int ceph_encode_encrypted_dname(struct inode *parent, char *buf,
+					      int len)
 {
-	memcpy(buf, d_name->name, d_name->len);
-	return d_name->len;
-}
-
-static inline int ceph_encode_encrypted_fname(struct inode *parent,
-					      struct dentry *dentry, char *buf)
-{
-	return -EOPNOTSUPP;
+	return len;
 }
 
 static inline int ceph_fname_alloc_buffer(struct inode *parent,
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index a321aa6d0ed2..8478e7e75df6 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -423,17 +423,16 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
 			req->r_inode_drop = CEPH_CAP_FILE_EXCL;
 		}
 		if (dfi->last_name) {
-			struct qstr d_name = { .name = dfi->last_name,
-					       .len = strlen(dfi->last_name) };
+			int len = strlen(dfi->last_name);
 
 			req->r_path2 = kzalloc(NAME_MAX + 1, GFP_KERNEL);
 			if (!req->r_path2) {
 				ceph_mdsc_put_request(req);
 				return -ENOMEM;
 			}
+			memcpy(req->r_path2, dfi->last_name, len);
 
-			err = ceph_encode_encrypted_dname(inode, &d_name,
-							  req->r_path2);
+			err = ceph_encode_encrypted_dname(inode, req->r_path2, len);
 			if (err < 0) {
 				ceph_mdsc_put_request(req);
 				return err;
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 230e0c3f341f..0f497c39ff82 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -2766,8 +2766,8 @@ char *ceph_mdsc_build_path(struct ceph_mds_client *mdsc, struct dentry *dentry,
 			}
 
 			if (fscrypt_has_encryption_key(d_inode(parent))) {
-				len = ceph_encode_encrypted_fname(d_inode(parent),
-								  cur, buf);
+				len = ceph_encode_encrypted_dname(d_inode(parent),
+								  buf, len);
 				if (len < 0) {
 					dput(parent);
 					dput(cur);
-- 
2.39.5

Re: [PATCH 1/3] [ceph] parse_longname(): strrchr() expects NUL-terminated string

[Viacheslav Dubeyko]

On Sat, 2025-06-14 at 07:22 +0100, Al Viro wrote:
> ... 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>
> ---

The patch set looks well and reasonable. Let me spend some time for testing it.
I'll be back ASAP.

Thanks,
Slava.

>  fs/ceph/crypto.c | 31 ++++++++++++-------------------
>  1 file changed, 12 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index 3b3c4d8d401e..9c7062245880 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -215,35 +215,31 @@ 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;
> +	char *name_end, *inode_number;
>  	int ret = -EIO;
> -
> +	/* NUL-terminate */
> +	char *str __free(kfree) = kmemdup_nul(name, *name_len, GFP_KERNEL);
> +	if (!str)
> +		return ERR_PTR(-ENOMEM);
>  	/* Skip initial '_' */
> -	name++;
> -	name_end = strrchr(name, '_');
> +	str++;
> +	name_end = strrchr(str, '_');
>  	if (!name_end) {
> -		doutc(cl, "failed to parse long snapshot name: %s\n", name);
> +		doutc(cl, "failed to parse long snapshot name: %s\n", str);
>  		return ERR_PTR(-EIO);
>  	}
> -	*name_len = (name_end - name);
> +	*name_len = (name_end - str);
>  	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);
> +	inode_number = name_end + 1;
>  	ret = kstrtou64(inode_number, 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", str);
> +		return ERR_PTR(ret);
>  	}
>  
>  	/* And finally the inode */
> @@ -254,9 +250,6 @@ static struct inode *parse_longname(const struct inode *parent,
>  		if (IS_ERR(dir))
>  			doutc(cl, "can't find inode %s (%s)\n", inode_number, name);
>  	}
> -
> -out:
> -	kfree(inode_number);
>  	return dir;
>  }
>  

-- 
Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

Re: [PATCH 1/3] [ceph] parse_longname(): strrchr() expects NUL-terminated string

[Viacheslav Dubeyko]

On Sat, 2025-06-14 at 07:22 +0100, Al Viro wrote:
> ... 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 | 31 ++++++++++++-------------------
>  1 file changed, 12 insertions(+), 19 deletions(-)
> 

I did run xfstests with the patch set. I don't see any issues.

Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

Thanks,
Slava.

> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index 3b3c4d8d401e..9c7062245880 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -215,35 +215,31 @@ 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;
> +	char *name_end, *inode_number;
>  	int ret = -EIO;
> -
> +	/* NUL-terminate */
> +	char *str __free(kfree) = kmemdup_nul(name, *name_len, GFP_KERNEL);
> +	if (!str)
> +		return ERR_PTR(-ENOMEM);
>  	/* Skip initial '_' */
> -	name++;
> -	name_end = strrchr(name, '_');
> +	str++;
> +	name_end = strrchr(str, '_');
>  	if (!name_end) {
> -		doutc(cl, "failed to parse long snapshot name: %s\n", name);
> +		doutc(cl, "failed to parse long snapshot name: %s\n", str);
>  		return ERR_PTR(-EIO);
>  	}
> -	*name_len = (name_end - name);
> +	*name_len = (name_end - str);
>  	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);
> +	inode_number = name_end + 1;
>  	ret = kstrtou64(inode_number, 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", str);
> +		return ERR_PTR(ret);
>  	}
>  
>  	/* And finally the inode */
> @@ -254,9 +250,6 @@ static struct inode *parse_longname(const struct inode *parent,
>  		if (IS_ERR(dir))
>  			doutc(cl, "can't find inode %s (%s)\n", inode_number, name);
>  	}
> -
> -out:
> -	kfree(inode_number);
>  	return dir;
>  }
>  

Re: [PATCH 2/3] prep for ceph_encode_encrypted_fname() fixes

[Viacheslav Dubeyko]

On Sat, 2025-06-14 at 07:22 +0100, Al Viro wrote:
> ceph_encode_encrypted_dname() would be better off with plaintext name
> already copied into buffer; we'll lift that into the callers on the
> next step, which will allow to fix UAF on races with rename; for now
> copy it in the very beginning of ceph_encode_encrypted_dname().
> 
> That has a pleasant side benefit - we don't need to mess with tmp_buf
> anymore (i.e. that's 256 bytes off the stack footprint).
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/ceph/crypto.c | 40 +++++++++++++++++-----------------------
>  1 file changed, 17 insertions(+), 23 deletions(-)
> 

Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

Thanks,
Slava.

> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index 9c7062245880..2aef56fc6275 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -258,31 +258,28 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
>  {
>  	struct ceph_client *cl = ceph_inode_to_client(parent);
>  	struct inode *dir = parent;
> -	struct qstr iname;
> +	char *p = buf;
>  	u32 len;
>  	int name_len;
>  	int elen;
>  	int ret;
>  	u8 *cryptbuf = NULL;
>  
> -	iname.name = d_name->name;
> -	name_len = d_name->len;
> +	memcpy(buf, d_name->name, d_name->len);
> +	elen = d_name->len;
> +
> +	name_len = elen;
>  
>  	/* Handle the special case of snapshot names that start with '_' */
> -	if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
> -	    (iname.name[0] == '_')) {
> -		dir = parse_longname(parent, iname.name, &name_len);
> +	if (ceph_snap(dir) == CEPH_SNAPDIR && *p == '_') {
> +		dir = parse_longname(parent, p, &name_len);
>  		if (IS_ERR(dir))
>  			return PTR_ERR(dir);
> -		iname.name++; /* skip initial '_' */
> +		p++; /* skip initial '_' */
>  	}
> -	iname.len = name_len;
>  
> -	if (!fscrypt_has_encryption_key(dir)) {
> -		memcpy(buf, d_name->name, d_name->len);
> -		elen = d_name->len;
> +	if (!fscrypt_has_encryption_key(dir))
>  		goto out;
> -	}
>  
>  	/*
>  	 * Convert cleartext d_name to ciphertext. If result is longer than
> @@ -290,7 +287,7 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
>  	 *
>  	 * See: fscrypt_setup_filename
>  	 */
> -	if (!fscrypt_fname_encrypted_size(dir, iname.len, NAME_MAX, &len)) {
> +	if (!fscrypt_fname_encrypted_size(dir, name_len, NAME_MAX, &len)) {
>  		elen = -ENAMETOOLONG;
>  		goto out;
>  	}
> @@ -303,7 +300,9 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
>  		goto out;
>  	}
>  
> -	ret = fscrypt_fname_encrypt(dir, &iname, cryptbuf, len);
> +	ret = fscrypt_fname_encrypt(dir,
> +				    &(struct qstr)QSTR_INIT(p, name_len),
> +				    cryptbuf, len);
>  	if (ret) {
>  		elen = ret;
>  		goto out;
> @@ -324,18 +323,13 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
>  	}
>  
>  	/* base64 encode the encrypted name */
> -	elen = ceph_base64_encode(cryptbuf, len, buf);
> -	doutc(cl, "base64-encoded ciphertext name = %.*s\n", elen, buf);
> +	elen = ceph_base64_encode(cryptbuf, len, p);
> +	doutc(cl, "base64-encoded ciphertext name = %.*s\n", elen, p);
>  
>  	/* To understand the 240 limit, see CEPH_NOHASH_NAME_MAX comments */
>  	WARN_ON(elen > 240);
> -	if ((elen > 0) && (dir != parent)) {
> -		char tmp_buf[NAME_MAX];
> -
> -		elen = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
> -				elen, buf, dir->i_ino);
> -		memcpy(buf, tmp_buf, elen);
> -	}
> +	if (dir != parent) // leading _ is already there; append _<inum>
> +		elen += 1 + sprintf(p + elen, "_%ld", dir->i_ino);
>  
>  out:
>  	kfree(cryptbuf);

Re: [PATCH 3/3] ceph: fix a race with rename() in ceph_mdsc_build_path()

[Viacheslav Dubeyko]

On Sat, 2025-06-14 at 07:22 +0100, Al Viro wrote:
> Lift copying the name into callers of ceph_encode_encrypted_dname()
> that do not have it already copied; ceph_encode_encrypted_fname()
> disappears.
> 
> That fixes a UAF in ceph_mdsc_build_path() - while the initial copy
> of plaintext into buf is done under ->d_lock, we access the
> original name again in ceph_encode_encrypted_fname() and that is
> done without any locking.  With ceph_encode_encrypted_dname() using
> the stable copy the problem goes away.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/ceph/caps.c       | 18 +++++++-----------
>  fs/ceph/crypto.c     | 19 ++-----------------
>  fs/ceph/crypto.h     | 18 ++++--------------
>  fs/ceph/dir.c        |  7 +++----
>  fs/ceph/mds_client.c |  4 ++--
>  5 files changed, 18 insertions(+), 48 deletions(-)
> 

Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

Thanks,
Slava.

> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index a8d8b56cf9d2..b1a8ff612c41 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -4957,24 +4957,20 @@ int ceph_encode_dentry_release(void **p, struct dentry *dentry,
>  	cl = ceph_inode_to_client(dir);
>  	spin_lock(&dentry->d_lock);
>  	if (ret && di->lease_session && di->lease_session->s_mds == mds) {
> +		int len = dentry->d_name.len;
>  		doutc(cl, "%p mds%d seq %d\n",  dentry, mds,
>  		      (int)di->lease_seq);
>  		rel->dname_seq = cpu_to_le32(di->lease_seq);
>  		__ceph_mdsc_drop_dentry_lease(dentry);
> +		memcpy(*p, dentry->d_name.name, len);
>  		spin_unlock(&dentry->d_lock);
>  		if (IS_ENCRYPTED(dir) && fscrypt_has_encryption_key(dir)) {
> -			int ret2 = ceph_encode_encrypted_fname(dir, dentry, *p);
> -
> -			if (ret2 < 0)
> -				return ret2;
> -
> -			rel->dname_len = cpu_to_le32(ret2);
> -			*p += ret2;
> -		} else {
> -			rel->dname_len = cpu_to_le32(dentry->d_name.len);
> -			memcpy(*p, dentry->d_name.name, dentry->d_name.len);
> -			*p += dentry->d_name.len;
> +			len = ceph_encode_encrypted_dname(dir, *p, len);
> +			if (len < 0)
> +				return len;
>  		}
> +		rel->dname_len = cpu_to_le32(len);
> +		*p += len;
>  	} else {
>  		spin_unlock(&dentry->d_lock);
>  	}
> diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c
> index 2aef56fc6275..e312f52f48e4 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -253,23 +253,16 @@ static struct inode *parse_longname(const struct inode *parent,
>  	return dir;
>  }
>  
> -int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
> -				char *buf)
> +int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int elen)
>  {
>  	struct ceph_client *cl = ceph_inode_to_client(parent);
>  	struct inode *dir = parent;
>  	char *p = buf;
>  	u32 len;
> -	int name_len;
> -	int elen;
> +	int name_len = elen;
>  	int ret;
>  	u8 *cryptbuf = NULL;
>  
> -	memcpy(buf, d_name->name, d_name->len);
> -	elen = d_name->len;
> -
> -	name_len = elen;
> -
>  	/* Handle the special case of snapshot names that start with '_' */
>  	if (ceph_snap(dir) == CEPH_SNAPDIR && *p == '_') {
>  		dir = parse_longname(parent, p, &name_len);
> @@ -342,14 +335,6 @@ int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
>  	return elen;
>  }
>  
> -int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry,
> -				char *buf)
> -{
> -	WARN_ON_ONCE(!fscrypt_has_encryption_key(parent));
> -
> -	return ceph_encode_encrypted_dname(parent, &dentry->d_name, buf);
> -}
> -
>  /**
>   * ceph_fname_to_usr - convert a filename for userland presentation
>   * @fname: ceph_fname to be converted
> diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h
> index d0768239a1c9..f752bbb2eb06 100644
> --- a/fs/ceph/crypto.h
> +++ b/fs/ceph/crypto.h
> @@ -102,10 +102,7 @@ int ceph_fscrypt_prepare_context(struct inode *dir, struct inode *inode,
>  				 struct ceph_acl_sec_ctx *as);
>  void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req,
>  				struct ceph_acl_sec_ctx *as);
> -int ceph_encode_encrypted_dname(struct inode *parent, struct qstr *d_name,
> -				char *buf);
> -int ceph_encode_encrypted_fname(struct inode *parent, struct dentry *dentry,
> -				char *buf);
> +int ceph_encode_encrypted_dname(struct inode *parent, char *buf, int len);
>  
>  static inline int ceph_fname_alloc_buffer(struct inode *parent,
>  					  struct fscrypt_str *fname)
> @@ -194,17 +191,10 @@ static inline void ceph_fscrypt_as_ctx_to_req(struct ceph_mds_request *req,
>  {
>  }
>  
> -static inline int ceph_encode_encrypted_dname(struct inode *parent,
> -					      struct qstr *d_name, char *buf)
> +static inline int ceph_encode_encrypted_dname(struct inode *parent, char *buf,
> +					      int len)
>  {
> -	memcpy(buf, d_name->name, d_name->len);
> -	return d_name->len;
> -}
> -
> -static inline int ceph_encode_encrypted_fname(struct inode *parent,
> -					      struct dentry *dentry, char *buf)
> -{
> -	return -EOPNOTSUPP;
> +	return len;
>  }
>  
>  static inline int ceph_fname_alloc_buffer(struct inode *parent,
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index a321aa6d0ed2..8478e7e75df6 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -423,17 +423,16 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
>  			req->r_inode_drop = CEPH_CAP_FILE_EXCL;
>  		}
>  		if (dfi->last_name) {
> -			struct qstr d_name = { .name = dfi->last_name,
> -					       .len = strlen(dfi->last_name) };
> +			int len = strlen(dfi->last_name);
>  
>  			req->r_path2 = kzalloc(NAME_MAX + 1, GFP_KERNEL);
>  			if (!req->r_path2) {
>  				ceph_mdsc_put_request(req);
>  				return -ENOMEM;
>  			}
> +			memcpy(req->r_path2, dfi->last_name, len);
>  
> -			err = ceph_encode_encrypted_dname(inode, &d_name,
> -							  req->r_path2);
> +			err = ceph_encode_encrypted_dname(inode, req->r_path2, len);
>  			if (err < 0) {
>  				ceph_mdsc_put_request(req);
>  				return err;
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 230e0c3f341f..0f497c39ff82 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -2766,8 +2766,8 @@ char *ceph_mdsc_build_path(struct ceph_mds_client *mdsc, struct dentry *dentry,
>  			}
>  
>  			if (fscrypt_has_encryption_key(d_inode(parent))) {
> -				len = ceph_encode_encrypted_fname(d_inode(parent),
> -								  cur, buf);
> +				len = ceph_encode_encrypted_dname(d_inode(parent),
> +								  buf, len);
>  				if (len < 0) {
>  					dput(parent);
>  					dput(cur);

Re: [PATCH 3/3] ceph: fix a race with rename() in ceph_mdsc_build_path()

[Al Viro]

On Tue, Jun 17, 2025 at 06:21:38PM +0000, Viacheslav Dubeyko wrote:

> Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>

OK, tested-by/reviewed-by applied to commits in that branch, branch
force-pushed to the same place
(git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git work.ceph-d_name-fixes)

Would you prefer to merge it via the ceph tree?  Or I could throw it
into my #for-next and push it to Linus come the next window - up to you...

RE: [PATCH 3/3] ceph: fix a race with rename() in ceph_mdsc_build_path()

[Viacheslav Dubeyko]

On Tue, 2025-06-17 at 23:01 +0100, Al Viro wrote:
> On Tue, Jun 17, 2025 at 06:21:38PM +0000, Viacheslav Dubeyko wrote:
> 
> > Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> 
> OK, tested-by/reviewed-by applied to commits in that branch, branch
> force-pushed to the same place
> (git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git work.ceph-d_name-fixes)
> 
> Would you prefer to merge it via the ceph tree?  Or I could throw it
> into my #for-next and push it to Linus come the next window - up to you...

Frankly speaking, your tree could be the faster way to upstream. However, I can
push this patch set into the ceph tree for more deeper testing in the internal
testing infrastructure. But I don't expect any serious issues in the patches
that could introduce some bugs.

Ilya,

What is your opinion on this? Would you prefer to go through the ceph tree?

Thanks,
Slava.

Re: [PATCH 3/3] ceph: fix a race with rename() in ceph_mdsc_build_path()

[Al Viro]

On Tue, Jun 17, 2025 at 10:12:08PM +0000, Viacheslav Dubeyko wrote:
> On Tue, 2025-06-17 at 23:01 +0100, Al Viro wrote:
> > On Tue, Jun 17, 2025 at 06:21:38PM +0000, Viacheslav Dubeyko wrote:
> > 
> > > Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > 
> > OK, tested-by/reviewed-by applied to commits in that branch, branch
> > force-pushed to the same place
> > (git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git work.ceph-d_name-fixes)
> > 
> > Would you prefer to merge it via the ceph tree?  Or I could throw it
> > into my #for-next and push it to Linus come the next window - up to you...
> 
> Frankly speaking, your tree could be the faster way to upstream. However, I can
> push this patch set into the ceph tree for more deeper testing in the internal
> testing infrastructure. But I don't expect any serious issues in the patches
> that could introduce some bugs.
> 
> Ilya,
> 
> What is your opinion on this? Would you prefer to go through the ceph tree?

I can send a pull request to you now just as easily as I could send it to Linus
a month and a half down the road... ;-)  Up to you, guys.

RE: [PATCH 3/3] ceph: fix a race with rename() in ceph_mdsc_build_path()

[Viacheslav Dubeyko]

On Tue, 2025-06-17 at 23:15 +0100, Al Viro wrote:
> On Tue, Jun 17, 2025 at 10:12:08PM +0000, Viacheslav Dubeyko wrote:
> > On Tue, 2025-06-17 at 23:01 +0100, Al Viro wrote:
> > > On Tue, Jun 17, 2025 at 06:21:38PM +0000, Viacheslav Dubeyko wrote:
> > > 
> > > > Tested-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > > > Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>
> > > 
> > > OK, tested-by/reviewed-by applied to commits in that branch, branch
> > > force-pushed to the same place
> > > (git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git work.ceph-d_name-fixes)
> > > 
> > > Would you prefer to merge it via the ceph tree?  Or I could throw it
> > > into my #for-next and push it to Linus come the next window - up to you...
> > 
> > Frankly speaking, your tree could be the faster way to upstream. However, I can
> > push this patch set into the ceph tree for more deeper testing in the internal
> > testing infrastructure. But I don't expect any serious issues in the patches
> > that could introduce some bugs.
> > 
> > Ilya,
> > 
> > What is your opinion on this? Would you prefer to go through the ceph tree?
> 
> I can send a pull request to you now just as easily as I could send it to Linus
> a month and a half down the road... ;-)  Up to you, guys.

So, if we don't have any other opinion, then let's send the patch set through
your tree.

Thanks,
Slava.

Re: [PATCH 3/3] ceph: fix a race with rename() in ceph_mdsc_build_path()

[Al Viro]

On Thu, Jun 19, 2025 at 06:04:37PM +0000, Viacheslav Dubeyko wrote:

> > I can send a pull request to you now just as easily as I could send it to Linus
> > a month and a half down the road... ;-)  Up to you, guys.
> 
> So, if we don't have any other opinion, then let's send the patch set through
> your tree.

merged into #for-next, will go to Linus at the next window...