Re: [RFC PATCH v7 15/24] ceph: add encrypted fname handling to ceph_mdsc_build_path

[Jeff Layton <jlayton@kernel.org>]

On Sun, 2021-07-11 at 17:53 -0500, Eric Biggers wrote:
> On Fri, Jun 25, 2021 at 09:58:25AM -0400, Jeff Layton wrote:
> > +/*
> > + * We want to encrypt filenames when creating them, but the encrypted
> > + * versions of those names may have illegal characters in them. To mitigate
> > + * that, we base64 encode them, but that gives us a result that can exceed
> > + * NAME_MAX.
> > + *
> > + * Follow a similar scheme to fscrypt itself, and cap the filename to a
> > + * smaller size. If the cleartext name is longer than the value below, then
> > + * sha256 hash the remaining bytes.
> > + *
> > + * 189 bytes => 252 bytes base64-encoded, which is <= NAME_MAX (255)
> > + */
> > +#define CEPH_NOHASH_NAME_MAX (189 - SHA256_DIGEST_SIZE)
> 
> Shouldn't this say "If the ciphertext name is longer than the value below", not
> "If the cleartext name is longer than the value below"?
> 
> It would also be helpful if the above comment mentioned that when the hashing is
> done, the real encrypted name is stored separately.
> 
> > +#if IS_ENABLED(CONFIG_FS_ENCRYPTION)
> > +static int encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf)
> > +{
> > +	u32 len;
> > +	int elen;
> > +	int ret;
> > +	u8 *cryptbuf;
> > +
> > +	WARN_ON_ONCE(!fscrypt_has_encryption_key(parent));
> > +
> > +	/*
> > +	 * convert cleartext dentry name to ciphertext
> > +	 * if result is longer than CEPH_NOKEY_NAME_MAX,
> > +	 * sha256 the remaining bytes
> > +	 *
> > +	 * See: fscrypt_setup_filename
> > +	 */
> > +	if (!fscrypt_fname_encrypted_size(parent, dentry->d_name.len, NAME_MAX, &len))
> > +		return -ENAMETOOLONG;
> > +
> > +	/* If we have to hash the end, then we need a full-length buffer */
> > +	if (len > CEPH_NOHASH_NAME_MAX)
> > +		len = NAME_MAX;
> > +
> > +	cryptbuf = kmalloc(len, GFP_KERNEL);
> > +	if (!cryptbuf)
> > +		return -ENOMEM;
> > +
> > +	ret = fscrypt_fname_encrypt(parent, &dentry->d_name, cryptbuf, len);
> > +	if (ret) {
> > +		kfree(cryptbuf);
> > +		return ret;
> > +	}
> > +
> > +	/* 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;
> > +	}
> 
> When the ciphertext name is longer than CEPH_NOHASH_NAME_MAX, why is the
> filename being padded all the way to NAME_MAX?  That can produce a totally
> different ciphertext from that produced by get_fscrypt_altname() in the next
> patch.
> 

Oh, I misunderstood the meaning of the last parameter to
fscrypt_fname_encrypt. I had thought that was the length of the target
buffer, but it's not -- it's the length of the resulting filename (which
we need to precompute). I'll fix that up.

> The logical thing to do would be to do the encryption in the same way as
> get_fscrypt_altname(), and then replace any bytes beyond CEPH_NOHASH_NAME_MAX
> with their hash.
> 

That might make more sense.

> > +
> > +	/* base64 encode the encrypted name */
> > +	elen = fscrypt_base64_encode(cryptbuf, len, buf);
> > +	kfree(cryptbuf);
> > +	dout("base64-encoded ciphertext name = %.*s\n", len, buf);
> > +	return elen;
> 
> The argument to dout() should be elen, not len.
> 

Will fix, thanks.
-- 
Jeff Layton <jlayton@kernel.org>