Re: [PATCH v6 11/43] btrfs: add inode encryption contexts

[Chris Mason <clm@meta.com>]

Daniel Vacek <neelx@suse.com> wrote:
> From: Omar Sandoval <osandov@osandov.com>
> 
> fscrypt stores a context item with encrypted inodes that contains the
> related encryption information.  fscrypt provides an arbitrary blob for
> the filesystem to store, and it does not clearly fit into an existing
> structure, so this goes in a new item type.

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 2d6114c2937b897d844735aa16d5369e3ec6d135
Author: Omar Sandoval <osandov@osandov.com>

btrfs: add inode encryption contexts

fscrypt stores a context item with encrypted inodes that contains the
related encryption information.  fscrypt provides an arbitrary blob for
the filesystem to store, and it does not clearly fit into an existing
structure, so this goes in a new item type.

> diff --git a/fs/btrfs/fscrypt.c b/fs/btrfs/fscrypt.c
> index 6cfba7d94e726..e9b024d671a22 100644
> --- a/fs/btrfs/fscrypt.c
> +++ b/fs/btrfs/fscrypt.c

[ ... ]

> @@ -1,10 +1,126 @@
>  // SPDX-License-Identifier: GPL-2.0
>
> +#include <linux/iversion.h>
>  #include "ctree.h"
> +#include "accessors.h"
>  #include "btrfs_inode.h"
> +#include "disk-io.h"
> +#include "fs.h"
>  #include "fscrypt.h"
> +#include "ioctl.h"
> +#include "messages.h"
> +#include "transaction.h"
> +#include "xattr.h"
> +
> +static int btrfs_fscrypt_get_context(struct inode *inode, void *ctx, size_t len)
> +{
> +	struct btrfs_key key = {
> +		.objectid = btrfs_ino(BTRFS_I(inode)),
> +		.type = BTRFS_FSCRYPT_INODE_CTX_KEY,
> +		.offset = 0,
> +	};
> +	struct btrfs_path *path;
> +	struct extent_buffer *leaf;
> +	unsigned long ptr;
> +	int ret;
> +
> +
> +	path = btrfs_alloc_path();
> +	if (!path)
> +		return -ENOMEM;
> +
> +	ret = btrfs_search_slot(NULL, BTRFS_I(inode)->root, &key, path, 0, 0);
> +	if (ret) {
> +		len = -ENOENT;
> +		goto out;
> +	}
> +
> +	leaf = path->nodes[0];
> +	ptr = btrfs_item_ptr_offset(leaf, path->slots[0]);
> +	/* fscrypt provides max context length, but it could be less */
> +	len = min_t(size_t, len, btrfs_item_size(leaf, path->slots[0]));
> +	read_extent_buffer(leaf, ctx, ptr, len);
> +
> +out:
> +	btrfs_free_path(path);
> +	return len;
> +}
> +
> +static int btrfs_fscrypt_set_context(struct inode *inode, const void *ctx,
> +				     size_t len, void *fs_data)
> +{
> +	struct btrfs_trans_handle *trans = fs_data;
> +	struct btrfs_key key = {
> +		.objectid = btrfs_ino(BTRFS_I(inode)),
> +		.type = BTRFS_FSCRYPT_INODE_CTX_KEY,
> +		.offset = 0,
> +	};
> +	struct btrfs_path *path = NULL;
> +	struct extent_buffer *leaf;
> +	unsigned long ptr;
> +	int ret;
> +
> +	if (!trans)
> +		trans = btrfs_start_transaction(BTRFS_I(inode)->root, 2);
> +	if (IS_ERR(trans))
> +		return PTR_ERR(trans);
> +
> +	path = btrfs_alloc_path();
> +	if (!path) {
> +		ret = -ENOMEM;
> +		goto out_err;
> +	}
> +
> +	ret = btrfs_search_slot(trans, BTRFS_I(inode)->root, &key, path, 0, 1);
> +	if (ret < 0)
> +		goto out_err;
> +
> +	if (ret > 0) {
> +		btrfs_release_path(path);
> +		ret = btrfs_insert_empty_item(trans, BTRFS_I(inode)->root, path, &key, len);
> +		if (ret)
> +			goto out_err;
> +	}
> +
> +	leaf = path->nodes[0];
> +	ptr = btrfs_item_ptr_offset(leaf, path->slots[0]);
> +
> +	len = min_t(size_t, len, btrfs_item_size(leaf, path->slots[0]));
> +	write_extent_buffer(leaf, ctx, ptr, len);
> +	btrfs_mark_buffer_dirty(trans, leaf);
> +	btrfs_release_path(path);
> +
> +	if (fs_data)
> +		return ret;

Does this code leak the path when fs_data is non-NULL?

In btrfs_fscrypt_set_context(), a path is allocated via btrfs_alloc_path()
but when fs_data is provided (during inode creation with a transaction
handle), the function returns here without calling btrfs_free_path(path).

The btrfs_release_path() call above only releases internal references to
extent buffers and locks but does not free the path structure itself.

This leak occurs on every encrypted inode creation. The fix would be to add
btrfs_free_path(path) before the return.

> +
> +	BTRFS_I(inode)->flags |= BTRFS_INODE_ENCRYPT;
> +	btrfs_sync_inode_flags_to_i_flags(BTRFS_I(inode));
> +	inode_inc_iversion(inode);
> +	inode_set_ctime_current(inode);
> +	ret = btrfs_update_inode(trans, BTRFS_I(inode));
> +	if (ret)
> +		goto out_abort;
> +	btrfs_free_path(path);
> +	btrfs_end_transaction(trans);
> +	return 0;
> +out_abort:
> +	btrfs_abort_transaction(trans, ret);
> +out_err:
> +	if (!fs_data)
> +		btrfs_end_transaction(trans);
> +	btrfs_free_path(path);
> +	return ret;
> +}

[ ... ]