On Sun, Jul 16, 2023 at 11:52:38PM -0400, Sweet Tea Dorminy wrote:
> From: Omar Sandoval <osandov@xxxxxxxxxxx>
>
> 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.
>
> The first part of supporting nokey names is allowing lookups by nokey
> name. Only a few entry points need to support these: deleting a
> directory, file, or subvolume -- each of these call
> fscrypt_setup_filename() with a '1' argument, indicating that the key is
> not required and therefore a nokey name may be provided. If a nokey name
> is provided, the fscrypt_name returned by fscrypt_setup_filename() will
> not have its disk_name field populated, but will have various other
> fields set.
>
> This change alters the relevant codepaths to pass a complete
> fscrypt_name anywhere that it might contain a nokey name. When it does
> contain a nokey name, the first time the name is successfully matched to
> a stored name populates the disk name field of the fscrypt_name,
> allowing the caller to use the normal disk name codepaths afterward.
> Otherwise, the matching functionality is in close analogue to the
> function fscrypt_match_name().
>
> Functions where most callers are providing a fscrypt_str are duplicated
> and adapted for a fscrypt_name, and functions where most callers are
> providing a fscrypt_name are changed to so require at all callsites.
>
> Signed-off-by: Omar Sandoval <osandov@xxxxxxxxxxx>
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@xxxxxxxxxx>
> ---
> fs/btrfs/btrfs_inode.h | 2 +-
> fs/btrfs/delayed-inode.c | 30 +++++++-
> fs/btrfs/delayed-inode.h | 4 +-
> fs/btrfs/dir-item.c | 77 ++++++++++++++++++---
> fs/btrfs/dir-item.h | 13 +++-
> fs/btrfs/extent_io.c | 38 +++++++++++
> fs/btrfs/extent_io.h | 3 +
> fs/btrfs/fscrypt.c | 46 +++++++++++++
> fs/btrfs/fscrypt.h | 19 ++++++
> fs/btrfs/inode.c | 143 ++++++++++++++++++++++++++-------------
> fs/btrfs/root-tree.c | 8 ++-
> fs/btrfs/root-tree.h | 2 +-
> fs/btrfs/tree-log.c | 3 +-
> 13 files changed, 320 insertions(+), 68 deletions(-)
>
> diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
> index ec4a06a78aff..464059674ae5 100644
> --- a/fs/btrfs/btrfs_inode.h
> +++ b/fs/btrfs/btrfs_inode.h
> @@ -421,7 +421,7 @@ struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry);
> int btrfs_set_inode_index(struct btrfs_inode *dir, u64 *index);
> int btrfs_unlink_inode(struct btrfs_trans_handle *trans,
> struct btrfs_inode *dir, struct btrfs_inode *inode,
> - const struct fscrypt_str *name);
> + struct fscrypt_name *name);
> int btrfs_add_link(struct btrfs_trans_handle *trans,
> struct btrfs_inode *parent_inode, struct btrfs_inode *inode,
> const struct fscrypt_str *name, int add_backref, u64 index);
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index 6b457b010cbc..919303d29b76 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -1497,6 +1497,7 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
>
> ret = __btrfs_add_delayed_item(delayed_node, delayed_item);
> if (unlikely(ret)) {
> + // TODO: It would be nice to print the base64encoded name here maybe?
Generally we don't leve TODO's around unless they're big, additionally wrong
comment format.
<snip>
> +/*
> + * This function is extremely similar to fscrypt_match_name() but uses an
> + * extent_buffer. Also, it edits the provided argument to populate the disk_name
> + * if we successfully match and previously were using a nokey name.
> + */
> +bool btrfs_fscrypt_match_name(struct fscrypt_name *fname,
> + struct extent_buffer *leaf, unsigned long de_name,
> + u32 de_name_len)
> +{
> + const struct fscrypt_nokey_name *nokey_name =
> + (const void *)fname->crypto_buf.name;
Cast it to the thing it's going to be, als this whol function needs more
newlines.
> + u8 digest[SHA256_DIGEST_SIZE];
> +
> + if (likely(fname->disk_name.name)) {
> + if (de_name_len != fname->disk_name.len)
> + return false;
> + return !memcmp_extent_buffer(leaf, fname->disk_name.name,
> + de_name, de_name_len);
> + }
> + if (de_name_len <= sizeof(nokey_name->bytes))
> + return false;
> + if (memcmp_extent_buffer(leaf, nokey_name->bytes, de_name,
> + sizeof(nokey_name->bytes)))
> + return false;
> + extent_buffer_sha256(leaf, de_name + sizeof(nokey_name->bytes),
> + de_name_len - sizeof(nokey_name->bytes), digest);
> + if (!memcmp(digest, nokey_name->sha256, sizeof(digest))) {
> + /*
> + * For no-key names, we use this opportunity to find the disk
> + * name, so future searches don't need to deal with nokey names
> + * and we know what the encrypted size is.
> + */
> + fname->disk_name.name = kmalloc(de_name_len, GFP_KERNEL | GFP_NOFS);
GFP_NOFS is sufficient.
> + if (!fname->disk_name.name)
> + fname->disk_name.name = ERR_PTR(-ENOMEM);
This part worries me, we use this code everywhere and it's just screaming for a
gotcha, I'd rather return an error in this case. Thanks,
Josef
|