Re: [PATCH V11 5/9] famfs_fuse: register fs-dax daxdevs via FUSE_DEV_IOC_DAXDEV_OPEN

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Unconditional lock acquisition of a conditionally initialized semaphore causes a kernel crash on FUSE unmounts.
- [High] Missing bounds check on `se_devindex` leads to Undefined Behavior and validation bypass for out-of-bounds indices.
- [High] Race condition between famfs_teardown() and dax_holder_notify_failure() causes a NULL pointer dereference.
- [Medium] FUSE_DEV_IOC_DAXDEV_OPEN does not enforce that the reserved flags field is zero.
- [Medium] Missing memory barriers around bitfield initialization leads to data races and acquiring an uninitialized semaphore.
--

--- Patch [5]: [PATCH V11 5/9] famfs_fuse: register fs-dax daxdevs via FUSE_DEV_IOC_DAXDEV_OPEN ---
commit 40109f7c2918c43160e80f34d0381f258354bbf2
Author: John Groves <john@groves.net>

famfs_fuse: register fs-dax daxdevs via FUSE_DEV_IOC_DAXDEV_OPEN

This commit introduces a new ioctl, FUSE_DEV_IOC_DAXDEV_OPEN, to register
devdax devices backing an fs-dax (famfs) filesystem. It reuses the
fuse_backing_map structure and handles device resolution and table storage.

> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 5763a7cd3b37f..3e6aa15e0346e 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
[ ... ]
> @@ -2330,6 +2330,34 @@ static long fuse_dev_ioctl_backing_close(struct file *file, __u32 __user *argp)
>  	return fuse_backing_close(fud->chan->conn, backing_id);
>  }
>  
> +static long fuse_dev_ioctl_daxdev_open(struct file *file,
> +				       struct fuse_backing_map __user *argp)
> +{
> +	struct fuse_dev *fud = fuse_get_dev(file);
> +	struct fuse_backing_map map;
> +
> +	if (IS_ERR(fud))
> +		return PTR_ERR(fud);
> +
> +	if (!IS_ENABLED(CONFIG_FUSE_FAMFS_DAX))
> +		return -EOPNOTSUPP;
> +
> +	/*
> +	 * The famfs-mode gate (fc->famfs_iomap) lives in famfs_daxdev_open(),
> +	 * which has the full fuse_conn definition. Here, re-check CAP_SYS_RAWIO
> +	 * against the task performing the registration: famfs mode being enabled
> +	 * only attests that the session founder held it at FUSE_INIT, and the
> +	 * fuse device fd may have been passed to a less-privileged process.
> +	 */
> +	if (!capable(CAP_SYS_RAWIO))
> +		return -EPERM;
> +
> +	if (copy_from_user(&map, argp, sizeof(map)))
> +		return -EFAULT;
> +
> +	return famfs_daxdev_open(fud->chan->conn, &map);
> +}
> +

[Severity: Medium]
Should there be a check to enforce that map.flags is zero?

If userspace passes uninitialized garbage in the flags field and it is
silently accepted, wouldn't that permanently burn the flags field and
prevent future ABI extensions using this field?

> diff --git a/fs/fuse/famfs.c b/fs/fuse/famfs.c
> index 8f7ee7d6151bb..a2a7dd631dc0f 100644
> --- a/fs/fuse/famfs.c
> +++ b/fs/fuse/famfs.c
[ ... ]
> @@ -22,6 +23,331 @@
>  #include "famfs_kfmap.h"
>  #include "fuse_i.h"
>  
> +static void famfs_set_daxdev_err(
> +	struct fuse_conn *fc, struct dax_device *dax_devp);
> +
> +static int
> +famfs_dax_notify_failure(struct dax_device *dax_devp, u64 offset,
> +			u64 len, int mf_flags)
> +{
> +	struct fuse_conn *fc = dax_holder(dax_devp);
> +
> +	famfs_set_daxdev_err(fc, dax_devp);
> +
> +	return 0;
> +}

[Severity: High]
Can dax_holder() return NULL here and cause a crash?

If a DAX memory failure occurs concurrently with a FUSE unmount, the DAX
core could execute dax_holder_notify_failure() while famfs_teardown()
concurrently calls fs_put_dax().

Since fs_put_dax() clears holder_data without waiting for the active SRCU
read-side critical section, wouldn't dax_holder() return NULL, which is
then passed to famfs_set_daxdev_err() where it is dereferenced?

[ ... ]
> +static int
> +famfs_check_daxdev_table(
> +	struct fuse_mount *fm,
> +	const struct famfs_file_meta *meta)
> +{
> +	struct fuse_conn *fc = fm->fc;
> +	int nmissing = 0;
> +	int err;
> +
> +	err = famfs_devlist_alloc(fc);
> +	if (err)
> +		return err;
> +
> +	/* Count missing daxdevs while holding the reader lock */
> +	scoped_guard(rwsem_read, &fc->famfs_devlist_sem) {
> +		unsigned long i;
> +
> +		for_each_set_bit(i, (unsigned long *)&meta->dev_bitmap,
> +				 MAX_DAXDEVS) {
> +			struct famfs_daxdev *dd = &fc->dax_devlist->devlist[i];

[Severity: High]
Does this validation loop miss out-of-bounds indices?

In famfs_fuse_meta_alloc(), the dev_bitmap is populated directly from the
user-supplied index:

    meta->se[i].dev_index  = se_in[i].se_devindex;
    ...
    meta->dev_bitmap |= BIT_ULL(meta->se[i].dev_index);

If an untrusted FUSE server sends an fmap response where se_devindex is
between 24 and 63, the bit is set in meta->dev_bitmap. However, this
validation loop only scans up to MAX_DAXDEVS (24). 

Would this silently ignore out-of-bounds indices, leaving nmissing as 0,
and accepting invalid metadata that could cause out-of-bounds array access
later? Additionally, if se_devindex is >= 64, wouldn't BIT_ULL() trigger
undefined behavior?

> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 78ffc5fd50d03..9fc37015fb111 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1021,6 +1021,9 @@ void fuse_conn_put(struct fuse_conn *fc)
>  		WARN_ON(atomic_read(&bucket->count) != 1);
>  		kfree(bucket);
>  	}
> +	if (IS_ENABLED(CONFIG_FUSE_FAMFS_DAX))
> +		famfs_teardown(fc);
> +

[Severity: Critical]
Does this unconditionally tear down a conditionally initialized structure?

When a regular user mounts a standard FUSE filesystem, if the server
does not negotiate FUSE_DAX_FMAP, famfs_init_devlist_sem(fc) is skipped
in process_init_reply(). 

However, when the filesystem is unmounted, this unconditionally calls
famfs_teardown() (which acquires fc->famfs_devlist_sem). Could this cause
a kernel panic or lockdep splat by locking an uninitialized rw_semaphore?

[ ... ]
> @@ -1427,8 +1430,10 @@ static void process_init_reply(struct fuse_args *args, int error)
>  				u64 in_flags = FIELD_PREP(GENMASK_ULL(63, 32), ia->in.flags2)
>  						| ia->in.flags;
>  
> -				if (in_flags & FUSE_DAX_FMAP)
> +				if (in_flags & FUSE_DAX_FMAP) {
> +					famfs_init_devlist_sem(fc);
>  					fc->famfs_iomap = 1;
> +				}
>  			}

[Severity: Medium]
Are there missing memory barriers here when publishing famfs_iomap?

The flag is published without a write barrier after initializing the
semaphore. In famfs_daxdev_open(), it is read locklessly:

    if (!fc->famfs_iomap)
        return -EOPNOTSUPP;

Without smp_store_release() here and smp_load_acquire() there, could a
reader observe famfs_iomap == 1 before the semaphore initialization is
visible, and proceed to lock an uninitialized semaphore?

Also, since famfs_iomap is a bitfield, could concurrent non-atomic
modifications to adjacent bitfields cause a data race?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019f7d9fbe81-6cb16662-2522-47ea-a152-fab0ee3d9b35-000000@email.amazonses.com?part=5