Re: [PATCH v6 13/43] btrfs: adapt readdir for encrypted and nokey names

[Chris Mason <clm@meta.com>]

Daniel Vacek <neelx@suse.com> wrote:
> From: Omar Sandoval <osandov@osandov.com>
> 
> Deleting an encrypted file must always be permitted, even if the user
> does not have the appropriate key. Therefore, for listing an encrypted
> directory, so-called 'nokey' names are provided, and these nokey names
> must be sufficient to look up and delete the appropriate encrypted
> files. See 'struct fscrypt_nokey_name' for more information on the
> format of these names.

Hi everyone,

I'm running the btrfs for-next branch, along with the encryption patch set
through my AI review prompts:

https://github.com/masoncl/review-prompts

I'm working on some btrfs specific instructions, but hopefully these reviews
are already useful.  I've kept the headers to make it easier to double check
that I'm actually replying to the right email, but please let me know if any
parts of this are incorrect:

commit e264482bd61ca8096981d81cc5e7c839284ac969
Author: Omar Sandoval <osandov@osandov.com>

btrfs: adapt readdir for encrypted and nokey names

This commit adapts the btrfs readdir implementation to handle encrypted
filenames and nokey names, enabling directory listing and deletion of
encrypted files even without the encryption key.

> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index 1739a0b29c49d..c19213fab3dda 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c

[ ... ]

> @@ -1820,7 +1823,25 @@ bool btrfs_readdir_delayed_dir_index(struct dir_context *ctx,
>  		d_type = fs_ftype_to_dtype(btrfs_dir_flags_to_ftype(di->type));
>  		btrfs_disk_key_to_cpu(&location, &di->location);
>
> -		over = !dir_emit(ctx, name, name_len, location.objectid, d_type);
> +		if (di->type & BTRFS_FT_ENCRYPTED) {
> +			int ret;
> +			struct fscrypt_str iname = FSTR_INIT(name, name_len);
> +
> +			fstr->len = fstr_len;
> +			/*
> +			 * The hash is only used when the encryption key is not
> +			 * available. But if we have delayed insertions, then we
> +			 * must have the encryption key available or we wouldn't
> +			 * have been able to create entries in the directory.
> +			 * So, we don't calculate the hash.
> +			 */
> +			ret = fscrypt_fname_disk_to_usr(inode, 0, 0, &iname, fstr);
> +			if (ret)
> +				return ret;
> +			over = !dir_emit(ctx, fstr->name, fstr->len, location.objectid, d_type);
> +		} else {
> +			over = !dir_emit(ctx, name, name_len, location.objectid, d_type);
> +		}

Does the return statement at line 1840 cause silent error loss?

The function btrfs_readdir_delayed_dir_index returns bool, but
'return ret;' returns an int error code from fscrypt_fname_disk_to_usr.
Since ret is negative on error (e.g., -EUCLEAN from fname_decrypt
failures), this converts to 'true' in bool context.

The caller btrfs_real_readdir at inode.c:6410 interprets true as "stop
iteration normally":

    if (btrfs_readdir_delayed_dir_index(inode, &fstr, ctx, &ins_list))
        goto nopos;

The nopos label at line 6417 sets ret = 0, which means readdir()
completes successfully with incomplete results when crypto operations
fail on encrypted directory entries.

[ ... ]

> diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
> index 085a83ae9e62f..6e10dd4a4e9e1 100644
> --- a/fs/btrfs/dir-item.c
> +++ b/fs/btrfs/dir-item.c

[ ... ]

> @@ -227,6 +228,47 @@ struct btrfs_dir_item *btrfs_lookup_dir_item(struct btrfs_trans_handle *trans,
>  	return di;
>  }
>
> +/*
> + * Lookup for a directory item by fscrypt_name.
> + *
> + * @trans:	The transaction handle to use.
> + * @root:	The root of the target tree.
> + * @path:	Path to use for the search.
> + * @dir:	The inode number (objectid) of the directory.
> + * @name:	The fscrypt_name associated to the directory entry
> + * @mod:	Used to indicate if the tree search is meant for a read only
> + *		lookup or for a deletion lookup, so its value should be 0 or
> + *		-1, respectively.
> + *
> + * Returns: NULL if the dir item does not exists, an error pointer if an error
> + * happened, or a pointer to a dir item if a dir item exists for the given name.
> + */
> +struct btrfs_dir_item *btrfs_lookup_dir_item_fname(struct btrfs_trans_handle *trans,
> +						   struct btrfs_root *root,
> +						   struct btrfs_path *path, u64 dir,
> +						   struct fscrypt_name *name, int mod)
> +{
> +	struct btrfs_key key;
> +	struct btrfs_dir_item *di = NULL;
> +	int ret = 0;
> +
> +	key.objectid = dir;
> +	key.type = BTRFS_DIR_ITEM_KEY;
> +	key.offset = btrfs_name_hash(name->disk_name.name, name->disk_name.len);
> +	/* XXX get the right hash for no-key names */

Can this dereference a NULL pointer for long nokey names?

When btrfs_unlink() calls fscrypt_setup_filename() with lookup=1 for a
nokey encrypted filename, fname->disk_name.name is NULL for long nokey
names (see fscrypt_setup_filename lines 370-375 in fs/crypto/fname.c).

The call chain is: btrfs_unlink -> btrfs_unlink_inode ->
__btrfs_unlink_inode (line 4396) -> btrfs_lookup_dir_item_fname.

This passes NULL to btrfs_name_hash() at line 257, which calls
crc32c(NULL, len), causing a kernel crash. The XXX comment at line 258
acknowledges this is unhandled.

Note: This issue is addressed in a later commit in the patch series
(fabad1c3ea821 'btrfs: handle nokey names').