Re: [PATCH v3 7/9] fuse: add extent map data structure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] fuse: NULL pointer dereference in fuse_notify_map() due to missing backing_id_64 check
- [High] fuse: type confusion and wild pointer dereference in fuse_ext_map_is_dax()
- [Medium] fuse: duplicate backing_id insertion due to rhashtable_insert_fast usage
--

commit 7c30614b0de0feab07ac16cfbaac90b3eedab05c
Author: Miklos Szeredi <mszeredi@redhat.com>

fuse: add extent map data structure

Add support for creating and managing extent maps that map file regions to
dax device regions. Introduce FUSE_NOTIFY_BACKING_MAP for populating extent
maps from userspace.

> diff --git a/fs/fuse/backing.c b/fs/fuse/backing.c
> index 0ed850ddf8cde..b7ebc951dc313 100644
> --- a/fs/fuse/backing.c
> +++ b/fs/fuse/backing.c
[ ... ]
> @@ -84,7 +88,7 @@ static const struct rhashtable_params fuse_backing_prm = {
>  	.key_len = sizeof_field(struct fuse_backing, backing_id),
>  };
>  
> -static int fuse_backing_add_64(struct fuse_conn *fc, struct fuse_backing *fb)
> +int fuse_backing_add_64(struct fuse_conn *fc, struct fuse_backing *fb)
>  {
>  	return rhashtable_insert_fast(&fc->backing_64_ht, &fb->hash_node, fuse_backing_prm);
>  }

[Severity: Medium]
Will this blindly insert duplicate backing IDs into the hash table?

Calling rhashtable_insert_fast() passes NULL as the key to the internal
__rhashtable_insert_fast(), which short-circuits the duplicate comparison
logic. Since the newly added FUSE_NOTIFY_BACKING_MAP accepts user-provided
backing IDs and inserts them here, could an untrusted FUSE server send
multiple requests with the same backing ID and cause inconsistent state or
unbounded memory accumulation?

Should this use rhashtable_lookup_insert_fast() instead to properly check
for uniqueness?

> diff --git a/fs/fuse/ext_map.c b/fs/fuse/ext_map.c
> new file mode 100644
> index 0000000000000..39d7bb521873c
> --- /dev/null
> +++ b/fs/fuse/ext_map.c
[ ... ]
> +bool fuse_ext_map_is_dax(struct fuse_backing *fb)
> +{
> +	struct fuse_iext *fie;
> +
> +	if (WARN_ON(RB_EMPTY_ROOT(&fb->extents)))
> +		return false;
> +
> +	fie = rb_entry(fb->extents.rb_node, typeof(*fie), rb);
> +	return fie->backing->type == FUSE_BACKING_DAXDEV;
> +}

[Severity: High]
Does this safely handle other backing types passed in via the fb pointer?

The extents field is part of an anonymous union overlaying dax_dev. If this
function is called on a FUSE_BACKING_DAXDEV backing, it will treat the
dax_dev pointer as an rb_root. If dax_dev is non-NULL, RB_EMPTY_ROOT()
returns false, and the code then casts the pointer via rb_entry() and
dereferences it.

Can this cause an out-of-bounds read or a wild pointer dereference? Should
there be a check to ensure fb->type == FUSE_BACKING_EXTMAP before attempting
to access fb->extents?

> diff --git a/fs/fuse/notify.c b/fs/fuse/notify.c
> index 93e916a16ac93..7c427f88bbc52 100644
> --- a/fs/fuse/notify.c
> +++ b/fs/fuse/notify.c
[ ... ]
> +static int fuse_notify_map(struct fuse_conn *fc, unsigned int size,
> +			   struct fuse_copy_state *cs)
> +{
> +	struct fuse_notify_backing_map_out outarg;
> +	struct fuse_extent *ext __free(kvfree) = NULL;
> +	int err;
[ ... ]
> +	if (outarg.flags & ~FUSE_BACKING_MAP_CREATE)
> +		return -EINVAL;
> +
> +	if (!IS_ENABLED(CONFIG_FUSE_PASSTHROUGH))
> +		return -EOPNOTSUPP;
> +
> +	ext = kvmalloc_objs(*ext, outarg.num_extents);
> +	if (!ext)
> +		return -ENOMEM;
> +
> +	err = fuse_copy_one(cs, ext, size);
> +	if (err)
> +		return err;
> +
> +	return fuse_ext_map_populate(fc, &outarg, ext);
> +}

[Severity: High]
Is a check for fc->backing_id_64 missing here before attempting to populate
the extent map?

If the backing_id_64 feature was not negotiated with the FUSE server during
connection initialization, fc->backing_64_ht will remain uninitialized
(zero-filled) because fuse_backing_files_init_64() is never called.

When fuse_ext_map_populate() subsequently calls fuse_backing_add_64(), it
will use rhashtable_insert_fast() on an uninitialized hash table. Could this
result in a guaranteed NULL pointer dereference on ht->tbl, allowing a buggy
or malicious FUSE server to crash the kernel?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006180115.1425232-1-mszeredi@redhat.com?part=7