Re: [RFC] odd check in ceph_encode_encrypted_dname()

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2025-02-14 at 02:47 +0000, Al Viro wrote:
> AFAICS, this
> 
>         /* 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);
>         }
> 
> could drop the (elen > 0) part of the test.  elen comes from
>         elen = ceph_base64_encode(cryptbuf, len, buf);
> and that can't return a non-positive unless the second argument is 0 or
> above 1G.  The latter is flat-out impossible - right before that call
> we have
>         /* hash the end if the name is long enough */
>         if (len > CEPH_NOHASH_NAME_MAX) {
>                 u8 hash[SHA256_DIGEST_SIZE];
>                 u8 *extra = cryptbuf + CEPH_NOHASH_NAME_MAX;
> 
>                 /*
>                  * hash the extra bytes and overwrite crypttext beyond that
>                  * point with it
>                  */
>                 sha256(extra, len - CEPH_NOHASH_NAME_MAX, hash);
>                 memcpy(extra, hash, SHA256_DIGEST_SIZE);
>                 len = CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE;
>         }
> which obviously caps it with CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE,
> i.e. (180 - SHA256_DIGEST_SIZE) + SHA256_DIGEST_SIZE.
> 
> The former would have to come from
>         if (!fscrypt_fname_encrypted_size(dir, iname.len, NAME_MAX, &len)) {
>                 elen = -ENAMETOOLONG;
>                 goto out;
>         }
> and since fscrypt_fname_encrypted_size() must've returned true, we have
> len no less than FSCRYPT_FNAME_MIN_MSG_LEN, i.e. it's 16 or greater.
> 
> That stuff went into the tree in dd66df0053ef8 "ceph: add support for encrypted
> snapshot names" and as far as I can tell, everything above had been applicable
> back then too.
> 
> Am I missing something subtle here?  Can elen be non-positive at that point?
> 

No, I think you nailed it.

-- 
Jeff Layton <jlayton@kernel.org>