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