| Message ID | 20250215044714.GA3451131@ZenIV (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] prep for ceph_encode_encrypted_fname() fixes | expand |
On Sat, 2025-02-15 at 04:47 +0000, 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(-) > > diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c > index 3b3c4d8d401e..09346b91215a 100644 > --- a/fs/ceph/crypto.c > +++ b/fs/ceph/crypto.c > @@ -265,31 +265,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 > @@ -297,7 +294,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; > } > @@ -310,7 +307,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; > @@ -331,18 +330,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); Reviewed-by: Jeff Layton <jlayton@kernel.org>
diff --git a/fs/ceph/crypto.c b/fs/ceph/crypto.c index 3b3c4d8d401e..09346b91215a 100644 --- a/fs/ceph/crypto.c +++ b/fs/ceph/crypto.c @@ -265,31 +265,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 @@ -297,7 +294,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; } @@ -310,7 +307,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; @@ -331,18 +330,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);
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(-)