Re: [PATCH 03/36] fscrypt: export fscrypt_fname_encrypt and fscrypt_fname_encrypted_size

Eric Biggers
2021-12-10T19:32:51Z

On Thu, Dec 09, 2021 at 10:36:14AM -0500, Jeff Layton wrote:
> For ceph, we want to use our own scheme for handling filenames that are
> are longer than NAME_MAX after encryption and base64 encoding. This

base64 => Base64. (base64 and base64url are types of Base64.)

> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 8fa23d525b5c..3be04b5aa570 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -130,6 +130,7 @@ int fscrypt_fname_encrypt(const struct inode *inode, const struct qstr *iname,
>
> return 0;
> }
> +EXPORT_SYMBOL(fscrypt_fname_encrypt);

The documentation for the @inode parameter could use a mention that the inode's
key must have already been set up. External callers could get that wrong.

Also, I'd prefer EXPORT_SYMBOL_GPL for anything that isn't generic functionality
like Base64 encoding/decoding.

> +/**
> + * fscrypt_fname_encrypted_size() - calculate length of encrypted filename
> + * @inode: parent inode of dentry name being encrypted

Likewise, this should mention that the inode's key must have already been set
up.

> + * Filenames must be padded out to at least the end of an fscrypt block before
> + * encrypting them.

That's not really correct. The padding amount depends on the padding flags, as
well as whether the filename gets truncated at max_len or not. Also there's not
really any such thing as an "fscrypt block". (FS_CRYPTO_BLOCK_SIZE, which is 16
bytes, is misnamed. It really should be two separate things like
FSCRYPT_MIN_FNAME_CTEXT_SIZE and FSCRYPT_CONTENTS_CTEXT_ALIGNMENT.)

How about just writing something like:

Filenames that are shorter than the maximum length may have their lengths
increased slightly by encryption, due to padding that is applied.

> + *
> + * Return: false if the orig_len is shorter than max_len. Otherwise, true and
> + * fill out encrypted_len_ret with the length (up to max_len).

false if orig_len is *greater* than max_len.

> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index 5b0a9e6478b5..51e42767dbd6 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -297,14 +297,11 @@ void fscrypt_generate_iv(union fscrypt_iv *iv, u64 lblk_num,
> const struct fscrypt_info *ci);
>
> /* fname.c */
> -int fscrypt_fname_encrypt(const struct inode *inode, const struct qstr *iname,
> - u8 *out, unsigned int olen);
> -bool fscrypt_fname_encrypted_size(const union fscrypt_policy *policy,
> - u32 orig_len, u32 max_len,
> - u32 *encrypted_len_ret);
> +bool __fscrypt_fname_encrypted_size(const union fscrypt_policy *policy,
> + u32 orig_len, u32 max_len,
> + u32 *encrypted_len_ret);

This is indented with spaces, not tabs.

- Eric