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@xxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
