Re: [PATCH v3 4/9] fuse: support 64 bit, server allocated backing ID

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] fuse: Uninitialized rhashtable destruction on failed init
- [High] fuse: TOCTOU race between fuse_backing_close and process_init_reply
- [Medium] fuse: rhashtable_insert_fast allows duplicate backing_id
--

commit 90eba0277f715071875b550790d8bf829971f8b3
Author: Miklos Szeredi <mszeredi@redhat.com>

fuse: support 64 bit, server allocated backing ID

This commit adds support for server-allocated 64-bit backing IDs alongside
the existing kernel-allocated 32-bit IDs. It introduces a new ioctl to
create these backing IDs and handles their removal.

> diff --git a/fs/fuse/backing.c b/fs/fuse/backing.c
> index 433fa3098d71f..bc40818778dff 100644
> --- a/fs/fuse/backing.c
> +++ b/fs/fuse/backing.c
[ ... ]
> +static const struct rhashtable_params fuse_backing_prm = {
> +	.head_offset = offsetof(struct fuse_backing, hash_node),
> +	.key_offset = offsetof(struct fuse_backing, backing_id),
> +	.key_len = sizeof_field(struct fuse_backing, backing_id),
> +};
>  
> +static 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]
Does this code allow duplicate backing_id values?

If a FUSE daemon provides an already-existing backing_id,
rhashtable_insert_fast() will insert the new object instead of rejecting
it with -EEXIST.

Should this use rhashtable_lookup_insert_fast() instead to safely enforce
uniqueness?

[ ... ]
>  int fuse_backing_close(struct fuse_conn *fc, int backing_id)
>  {
>  	struct fuse_backing *fb = NULL;
>  	int err;
>  
>  	pr_debug("%s: backing_id=%d\n", __func__, backing_id);
>  
> +	if (fc->backing_id_64)
> +		return -EINVAL;
> +

[Severity: High]
Can this TOCTOU race lead to memory corruption? Since fc->backing_id_64 is
read here outside of fc->lock, it can be observed as false concurrently with
process_init_reply() setting it to true:

CPU1 (fuse_dev_ioctl_backing_close):
    if (fc->backing_id_64) // reads false
        ...
    fuse_backing_id_remove()
        spin_lock(&fc->lock);
        idr_remove(&fc->backing_files_map, id);

CPU2 (process_init_reply):
    scoped_guard(spinlock, &fc->lock) {
        fc->backing_id_64 = true;
    }
    if (ok)
        fuse_backing_files_init_64(fc); // calls rhashtable_init()

Because fuse_backing_close() proceeds, it calls idr_remove() on the union
concurrently with rhashtable_init() in CPU2, corrupting the shared memory.

>  	/* TODO: relax CAP_SYS_ADMIN once backing files are visible to lsof */
>  	err = -EPERM;
>  	if (!fc->passthrough || !capable(CAP_SYS_ADMIN))
[ ... ]
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index cbb10e19e7e86..b5b51865d59f8 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
[ ... ]
>  			if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH) &&
> -			    (flags & FUSE_PASSTHROUGH) &&
> +			    (flags & (FUSE_PASSTHROUGH | FUSE_PASSTHROUGH_V2)) &&
>  			    arg->max_stack_depth > 0 &&
>  			    arg->max_stack_depth <= FILESYSTEM_MAX_STACK_DEPTH &&
>  			    !(flags & FUSE_WRITEBACK_CACHE))  {
>  				fc->passthrough = 1;
>  				fc->max_stack_depth = arg->max_stack_depth;
>  				fm->sb->s_stack_depth = arg->max_stack_depth;
> +				if (flags & FUSE_PASSTHROUGH_V2) {
> +					/* Prevent race with fuse_backing_open() */
> +					scoped_guard(spinlock, &fc->lock) {
> +						if (fc->backing_id_32)
> +							ok = false;
> +						else
> +							fc->backing_id_64 = true;
> +					}
> +					if (ok)
> +						fuse_backing_files_init_64(fc);
> +				}

[Severity: High]
Does this unconditionally set fc->backing_id_64 = true but skip initialization
if ok is false?

If an earlier validation check sets ok = false, this code still sets
fc->backing_id_64 = true but skips calling fuse_backing_files_init_64(fc).

When the FUSE connection is later torn down, fuse_backing_files_free() will
see fc->backing_id_64 == true and attempt to call rhashtable_free_and_destroy()
on an uninitialized rhashtable, causing a kernel crash.

>  			}
>  			if (flags & FUSE_NO_EXPORT_SUPPORT)
>  				fm->sb->s_export_op = &fuse_export_fid_operations;

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