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 <[email protected]> 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/[email protected]?part=4
