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