Re: [PATCH v4 03/46] fscrypt: add a fscrypt_inode_open helper

[Eric Biggers <ebiggers@kernel.org>]

On Fri, Dec 01, 2023 at 05:11:00PM -0500, Josef Bacik wrote:
> We have fscrypt_file_open() which is meant to be called on files being
> opened so that their key is loaded when we start reading data from them.
> 
> However for btrfs send we are opening the inode directly without a filp,
> so we need a different helper to make sure we can load the fscrypt
> context for the inode before reading its contents.
> 
> We need a different helper as opposed to simply using
> fscrypt_has_permitted_context() directly because of '-o
> test_dummy_encryption', which allows for encrypted files to be created
> with !IS_ENCRYPTED set on the directory (the root directory in this
> case).  fscrypt_file_open() already does the appropriate check where it
> simply doesn't call fscrypt_has_permitted_context() if the parent
> directory isn't marked with IS_ENCRYPTED in order to facilitate this
> invariant when using '-o test_dummy_encryption'.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/crypto/hooks.c       | 42 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/fscrypt.h |  8 ++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
> index 52504dd478d3..a391a987c58f 100644
> --- a/fs/crypto/hooks.c
> +++ b/fs/crypto/hooks.c
> @@ -49,6 +49,48 @@ int fscrypt_file_open(struct inode *inode, struct file *filp)
>  }
>  EXPORT_SYMBOL_GPL(fscrypt_file_open);
>  
> +/**
> + * fscrypt_inode_open() - prepare to open a possibly-encrypted regular file
> + * @dir: the directory that contains this inode
> + * @inode: the inode being opened
> + *
> + * Currently, an encrypted regular file can only be opened if its encryption key
> + * is available; access to the raw encrypted contents is not supported.
> + * Therefore, we first set up the inode's encryption key (if not already done)
> + * and return an error if it's unavailable.
> + *
> + * We also verify that if the parent directory is encrypted, then the inode
> + * being opened uses the same encryption policy.  This is needed as part of the
> + * enforcement that all files in an encrypted directory tree use the same
> + * encryption policy, as a protection against certain types of offline attacks.
> + * Note that this check is needed even when opening an *unencrypted* file, since
> + * it's forbidden to have an unencrypted file in an encrypted directory.
> + *
> + * File systems should be using fscrypt_file_open in their open callback.  This
> + * is for file systems that may need to open inodes outside of the normal file
> + * open path, btrfs send for example.
> + *
> + * Return: 0 on success, -ENOKEY if the key is missing, or another -errno code
> + */
> +int fscrypt_inode_open(struct inode *dir, struct inode *inode)
> +{
> +	int err;
> +
> +	err = fscrypt_require_key(inode);
> +	if (err)
> +		return err;
> +
> +	if (IS_ENCRYPTED(dir) &&
> +	    !fscrypt_has_permitted_context(dir, inode)) {
> +		fscrypt_warn(inode,
> +			     "Inconsistent encryption context (parent directory: %lu)",
> +			     dir->i_ino);
> +		err = -EPERM;
> +	}
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(fscrypt_inode_open);

The comment and code is heavily copy+pasted from fscrypt_file_open(), which is
not great.  How about naming the new function __fscrypt_file_open(),
implementing fscrypt_file_open() on top of it, and making the comment describe
the differences vs. fscrypt_file_open()?

So fscrypt_file_open() would do:

	struct inode *dir;
	int err;

	dir = dget_parent(file_dentry(filp));

	err = __fscrypt_file_open(dir, filp);

	dput(dir);
	return err;

... and the comment for the new function would be something like:

/**
 * __fscrypt_file_open() - prepare for filesystem-internal access to a
 *			   possibly-encrypted regular file
 * @dir: the inode for the directory via which the file is being accessed
 * @inode: the inode being "opened"
 *
 * This is like fscrypt_file_open(), but instead of taking the 'struct file'
 * being opened it takes the parent directory explicitly.  This is intended for
 * use cases such as "send/receive" which involve the filesystem accessing file
 * contents without setting up a 'struct file'.
 *
 * Return: 0 on success, -ENOKEY if the key is missing, or another -errno code
 */
int __fscrypt_file_open(struct inode *dir, struct inode *inode)


Note, we need to be careful when describing "@dir".  It's not simply "the
directory that contains this inode".  An inode can be in multiple directories.

- Eric