diff mbox series

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

Message ID 20250215044739.GB3451131@ZenIV (mailing list archive)
State New
Headers show
Series [1/2] prep for ceph_encode_encrypted_fname() fixes | expand

Commit Message

Al Viro Feb. 15, 2025, 4:47 a.m. UTC 
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     | 20 +++-----------------
 fs/ceph/crypto.h     | 18 ++++--------------
 fs/ceph/dir.c        |  7 +++----
 fs/ceph/mds_client.c |  4 ++--
 5 files changed, 19 insertions(+), 48 deletions(-)

Comments

Jeff Layton Feb. 15, 2025, 12:42 p.m. UTC | #1 
On Sat, 2025-02-15 at 04:47 +0000, 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     | 20 +++-----------------
>  fs/ceph/crypto.h     | 18 ++++--------------
>  fs/ceph/dir.c        |  7 +++----
>  fs/ceph/mds_client.c |  4 ++--
>  5 files changed, 19 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 09346b91215a..76a4a7e60270 100644
> --- a/fs/ceph/crypto.c
> +++ b/fs/ceph/crypto.c
> @@ -260,23 +260,17 @@ 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;
> -
> +	buf[elen] = '\0';
>  	/* Handle the special case of snapshot names that start with '_' */
>  	if (ceph_snap(dir) == CEPH_SNAPDIR && *p == '_') {
>  		dir = parse_longname(parent, p, &name_len);
> @@ -349,14 +343,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 62e99e65250d..539954e2ea38 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -422,17 +422,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 54b3421501e9..7dc854631b55 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);

Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff mbox series

Patch

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 09346b91215a..76a4a7e60270 100644
--- a/fs/ceph/crypto.c
+++ b/fs/ceph/crypto.c
@@ -260,23 +260,17 @@  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;
-
+	buf[elen] = '\0';
 	/* Handle the special case of snapshot names that start with '_' */
 	if (ceph_snap(dir) == CEPH_SNAPDIR && *p == '_') {
 		dir = parse_longname(parent, p, &name_len);
@@ -349,14 +343,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 62e99e65250d..539954e2ea38 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -422,17 +422,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 54b3421501e9..7dc854631b55 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);