Re: [PATCH V8 05/10] famfs_fuse: GET_DAXDEV message and daxdev_table

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The FUSE server-provided string daxdev_out.name is not guaranteed to be null-terminated, causing out-of-bounds reads.
- [High] An untrusted device index from the FUSE payload is used in a bitwise shift without prior bounds checking, causing Undefined Behavior.
- [High] The return value of famfs_update_daxdev_table() is ignored, allowing file initialization to proceed with uninitialized DAX devices.
- [Medium] FUSE_GET_DAXDEV ignores the defined input payload struct and incorrectly uses the nodeid field.
- [Medium] Casting u64* to unsigned long* for for_each_set_bit() breaks on 32-bit big-endian systems.
- [Medium] Unprotected plain read of fc->dax_devlist creates a data race with its atomic initialization.
--

--- Patch [5]: [PATCH V8 05/10] famfs_fuse: GET_DAXDEV message and daxdev_table ---
commit 5e7c03f2362ee3cfd1a9cae3fbaaaeb19bced93d
Author: John Groves <[email protected]>

famfs_fuse: GET_DAXDEV message and daxdev_table

The new GET_DAXDEV message/response is added. The famfs_teardown() function 
is added as a primary teardown function for famfs. The command is triggered 
by the update_daxdev_table() call, if there are any daxdevs in the subject 
fmap that are not represented in the daxdev_table yet.

> diff --git a/fs/fuse/famfs.c b/fs/fuse/famfs.c
> index ac52e54e2cb59..0e9415aa63397 100644
> --- a/fs/fuse/famfs.c
> +++ b/fs/fuse/famfs.c
> @@ -21,6 +21,228 @@
[ ... ]
> +static int
> +famfs_fuse_get_daxdev(struct fuse_mount *fm, const u64 index)
> +{
> +	struct fuse_daxdev_out daxdev_out = { 0 };
> +	struct fuse_conn *fc = fm->fc;
> +	struct famfs_daxdev *daxdev;
> +	int rc;
> +
> +	FUSE_ARGS(args);
> +
> +	/* Store the daxdev in our table */
> +	if (index >= fc->dax_devlist->nslots) {
> +		pr_err("%s: index(%lld) > nslots(%d)\n",
> +		       __func__, index, fc->dax_devlist->nslots);
> +		return -EINVAL;
> +	}
> +
> +	args.opcode = FUSE_GET_DAXDEV;
> +	args.nodeid = index;
> +
> +	args.in_numargs = 0;

[Severity: Medium]
The UAPI defines struct fuse_get_daxdev_in with daxdev_num for the input
payload. Does this implementation intentionally ignore that struct, set
in_numargs = 0, and repurpose the nodeid field (which is normally for VFS
inode numbers) to pass the device index instead? This appears to break the
protocol format.

> +
> +	args.out_numargs = 1;
> +	args.out_args[0].size = sizeof(daxdev_out);
> +	args.out_args[0].value = &daxdev_out;
> +
> +	/* Send GET_DAXDEV command */
> +	rc = fuse_simple_request(fm, &args);
> +	if (rc) {
[ ... ]
> +		return rc;
> +	}
> +
> +	scoped_guard(rwsem_write, &fc->famfs_devlist_sem) {
> +		daxdev = &fc->dax_devlist->devlist[index];
> +
> +		/* Abort if daxdev is now valid (races are possible here) */
> +		if (daxdev->valid) {
> +			pr_debug("%s: daxdev already known\n", __func__);
> +			return 0;
> +		}
> +
> +		/* Verify dev is valid and can be opened and gets the devno */
> +		rc = famfs_verify_daxdev(daxdev_out.name, &daxdev->devno);

[Severity: High]
Is the FUSE server-provided string daxdev_out.name guaranteed to be
null-terminated?

If a FUSE server sends a response where the 256-byte name field contains no
null bytes, passing it to kern_path() (via famfs_verify_daxdev()) and
kstrdup() could read past the end of the stack-allocated daxdev_out struct
until a null byte is found, potentially causing an out-of-bounds memory read.

[ ... ]
> +static int
> +famfs_update_daxdev_table(
> +	struct fuse_mount *fm,
> +	const struct famfs_file_meta *meta)
> +{
> +	struct famfs_dax_devlist *local_devlist;
> +	struct fuse_conn *fc = fm->fc;
> +	int indices_to_fetch[MAX_DAXDEVS];
> +	int n_to_fetch = 0;
> +	int err;
> +
> +	/* First time through we will need to allocate the dax_devlist */
> +	if (!fc->dax_devlist) {

[Severity: Medium]
Could this unprotected plain read of fc->dax_devlist race with the atomic
cmpxchg() publication later in the function? Should this use READ_ONCE() to
prevent compiler optimization issues or KCSAN warnings?

> +		local_devlist = kcalloc(1, sizeof(*fc->dax_devlist), GFP_KERNEL);
> +		if (!local_devlist)
> +			return -ENOMEM;
> +
> +		local_devlist->nslots = MAX_DAXDEVS;
> +
> +		local_devlist->devlist = kcalloc(MAX_DAXDEVS,
> +						 sizeof(struct famfs_daxdev),
> +						 GFP_KERNEL);
> +		if (!local_devlist->devlist) {
> +			kfree(local_devlist);
> +			return -ENOMEM;
> +		}
> +
> +		/* We don't need famfs_devlist_sem here because we use cmpxchg */
> +		if (cmpxchg(&fc->dax_devlist, NULL, local_devlist) != NULL) {
> +			kfree(local_devlist->devlist);
> +			kfree(local_devlist); /* another thread beat us to it */
> +		}
> +	}
> +
> +	/* Collect indices that need fetching while holding read lock */
> +	scoped_guard(rwsem_read, &fc->famfs_devlist_sem) {
> +		unsigned long i;
> +
> +		for_each_set_bit(i, (unsigned long *)&meta->dev_bitmap, MAX_DAXDEVS) {

[Severity: Medium]
Does casting a u64 * to unsigned long * here break on 32-bit big-endian
systems?

On those architectures, the loop might read the upper 32 bits (which are
zero) instead of the lower 32 bits, resulting in a failure to fetch any DAX
devices. Should meta->dev_bitmap use DECLARE_BITMAP to avoid endianness and
size mismatch bugs?

[ ... ]
> @@ -329,6 +551,9 @@ famfs_file_init_dax(
>  	if (rc)
>  		goto errout;
>  
> +	/* Make sure this fmap doesn't reference any unknown daxdevs */
> +	famfs_update_daxdev_table(fm, meta);
> +

[Severity: High]
The return value of famfs_update_daxdev_table() is an error code, but it is
completely ignored here.

If it fails (e.g., due to an ENOMEM allocation failure or if the daxdev fetch
fails), file initialization proceeds with uninitialized DAX devices. Could
this lead to invalid state and potential NULL pointer dereferences on
subsequent accesses?

>  	/* Publish the famfs metadata on fi->famfs_meta */
>  	inode_lock(inode);

[ ... ]

While reviewing famfs_fuse_meta_alloc() in the same file:

> 		for (i = 0; i < fmh->nextents; i++) {
> 			meta->se[i].dev_index  = se_in[i].se_devindex;
> 			meta->se[i].ext_offset = se_in[i].se_offset;
> 			meta->se[i].ext_len    = se_in[i].se_len;
> 
> 			/* Record bitmap of referenced daxdev indices */
> 			meta->dev_bitmap |= (1 << meta->se[i].dev_index);

[Severity: High]
Is it possible for the untrusted device index from the FUSE payload
(se_devindex) to cause undefined behavior here?

If a FUSE server provides an se_devindex >= 31, the expression
1 << meta->se[i].dev_index shifts a 32-bit signed integer by an unvalidated
amount before famfs_check_ext_alignment() validates the index.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5