Re: [PATCH V12 08/12] famfs: iomap_begin and file-to-dax offset resolution

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

New issues:
- [Critical] Missing read memory barrier (`smp_rmb()`) in `famfs_daxdev_from_index` allows returning an uninitialized pointer, causing the DAX core to use an uninitialized iomap structure.
- [Critical] The iomap_begin handlers mistakenly assign the input operation `flags` to the output `iomap->flags`, unintentionally setting `IOMAP_F_NEW` during writes and causing data corruption.
- [High] The `len` parameter in `famfs_meta_to_dax_offset` and `famfs_meta_to_dax_offset_interleaved` is incorrectly typed as `off_t` instead of `loff_t`, causing integer truncation on 32-bit architectures.
- [High] In `famfs_meta_to_dax_offset_interleaved`, `iomap->length` is not bounded by the end of the interleaved extent, causing out-of-bounds physical mappings.
- [High] Time-Of-Check to Time-Of-Use (TOCTOU) race condition in `famfs_meta_to_dax_offset` allows a NULL pointer dereference when evaluating `inode->i_private`.
- [Low] 64-bit integer division and modulo operations in `famfs_meta_to_dax_offset_interleaved` cause unresolved symbol errors during linking on 32-bit architectures.

Pre-existing issues:
- [Critical] The `FAMFSIOC_MAP_CREATE` ioctl lacks privilege checks, allowing any user with write access to a file to map and access arbitrary physical memory on any registered DAX device.
- [High] `famfs_dax_write_iter` bypasses standard VFS write checks, failing to clear setuid/setgid bits and update file times upon modification.
--

--- Patch [8]: [PATCH V12 08/12] famfs: iomap_begin and file-to-dax offset resolution ---
commit b4e4f3f2df1ac1714b2f127455834eca42345a35
Author: John Groves <John@Groves.net>
Subject: famfs: iomap_begin and file-to-dax offset resolution

Add the iomap resolver that maps a file offset to a (daxdev, offset) pair:
famfs_meta_to_dax_offset() for simple extent lists and
famfs_meta_to_dax_offset_interleaved() for striped files, backed by the
per-daxdev health check (famfs_dax_err) and table lookup
(famfs_daxdev_for_index), plus famfs_iomap_begin() and famfs_iomap_ops.

> diff --git a/fs/famfs/famfs_file.c b/fs/famfs/famfs_file.c
> index d710c8a0c923c..e7f271ce6d03d 100644
> --- a/fs/famfs/famfs_file.c
> +++ b/fs/famfs/famfs_file.c
> @@ -320,6 +320,298 @@ famfs_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
[ ... ]
> +static struct dax_device *
> +famfs_daxdev_from_index(struct famfs_fs_info *fsi, u64 dev_index, int *errp)
> +{
> +	struct famfs_dax_devlist *devlist = fsi->dax_devlist;
> +	struct famfs_daxdev *dd;
> +	int rc;
> +
> +	if (!devlist || dev_index >= devlist->nslots) {
> +		pr_debug("%s: dev_index %llu out of range\n",
> +			__func__, dev_index);
> +		*errp = -EIO;
> +		return NULL;
> +	}
> +	dd = &devlist->devlist[dev_index];
> +	rc = famfs_dax_err(dd);
> +	if (rc) {
> +		*errp = rc;
> +		return NULL;
> +	}
> +	*errp = 0;
> +	return dd->devp;
> +}

[Severity: Critical]
Does this code need a read memory barrier?

In famfs_daxdev_from_index(), dd->valid is read via famfs_dax_err(), and
then dd->devp is returned without an smp_rmb(). If a concurrent page fault
occurs while a DAX device is being installed via famfs_install_daxdev(),
CPU reordering could cause this to see valid == 1 but return a NULL device
pointer.

[ ... ]
> +static int
> +famfs_meta_to_dax_offset_interleaved(struct inode *inode, struct iomap *iomap,
> +			 loff_t file_offset, off_t len, unsigned int flags)

[Severity: High]
Can this cause integer truncation on 32-bit architectures?

The len parameter is typed as off_t (which is 32-bit signed on 32-bit
architectures) instead of loff_t. A large I/O request could result in a
negative value, which is then sign-extended back to a 64-bit value in
min_t(loff_t, len, ...), resulting in a massive iomap->length and causing
out-of-bounds mappings. Should this match the iomap_begin signature with
loff_t?

[ ... ]
> +		/* Is the data is in this striped extent? */
> +		if (local_offset < ext_size) {
> +			u64 chunk_num       = local_offset / chunk_size;
> +			u64 chunk_offset    = local_offset % chunk_size;
> +			u64 stripe_num      = chunk_num / nstrips;
> +			u64 strip_num       = chunk_num % nstrips;

[Severity: Low]
Could these 64-bit integer division and modulo operations cause unresolved
symbol errors during linking on 32-bit architectures?

The code uses standard / and % operators on 64-bit integers (local_offset,
chunk_size, chunk_num), which emits calls to __divdi3 and __umoddi3. Should
this use the kernel division helpers like div64_u64() or do_div()?

[ ... ]
> +			iomap->addr    = strip->ext_offset + strip_offset;
> +			iomap->offset  = file_offset;
> +			iomap->length  = min_t(loff_t, len, chunk_remainder);
> +			iomap->length  = min_t(loff_t, iomap->length,
> +					       strip->ext_len - strip_offset);

[Severity: High]
Does this properly bound the physical mapping length?

The calculation restricts iomap->length to chunk_remainder and the strip's
length, but fails to restrict it to ext_size - local_offset. If a read/write
operation happens near the end of an interleaved extent where ext_size is not
a perfect multiple of the chunk size, the returned length could physically
exceed the logical boundaries of the extent.

> +			iomap->dax_dev = daxdev;
> +			iomap->type    = IOMAP_MAPPED;
> +			iomap->flags   = flags;

[Severity: Critical]
Does assigning the input operational flags directly to the output mapping
flags overwrite valid data?

The flags argument to iomap_begin contains input flags (e.g., IOMAP_WRITE
which is 1 << 0). In the iomap output namespace, 1 << 0 is IOMAP_F_NEW.
Setting IOMAP_F_NEW tells the DAX core that the memory was newly allocated
and requires zeroing on partial writes, overwriting valid existing data with
zeroes.

[ ... ]
> +static int
> +famfs_meta_to_dax_offset(struct inode *inode, struct iomap *iomap,
> +			 loff_t file_offset, off_t len, unsigned int flags)

[Severity: High]
Can this cause integer truncation on 32-bit architectures?

The len parameter is typed as off_t (which is 32-bit signed on 32-bit
architectures) instead of loff_t. A large I/O request could result in a
negative value, which is then sign-extended back to a 64-bit value in
min_t(loff_t, len, ...), resulting in a massive iomap->length and causing
out-of-bounds mappings. Should this match the iomap_begin signature with
loff_t?

> +{
> +	struct famfs_fs_info  *fsi = inode->i_sb->s_fs_info;
> +	struct famfs_file_meta *meta = inode->i_private;
> +	loff_t local_offset = file_offset;
> +	int rc;
> +	int i;
> +
> +	if (fsi->deverror || famfs_file_invalid(inode))
> +		goto err_out;
> +
> +	if (meta->fm_extent_type == FAMFS_IOC_EXT_INTERLEAVE)

[Severity: High]
Is there a Time-Of-Check to Time-Of-Use (TOCTOU) race condition here?

The local meta pointer is fetched from inode->i_private locklessly. It then
calls famfs_file_invalid(inode), which re-fetches inode->i_private. If
another thread initialized the file in between via FAMFSIOC_MAP_CREATE,
famfs_file_invalid() sees the new metadata and returns 0 (success). The first
thread then proceeds to dereference its locally cached meta pointer (which is
still NULL), crashing the kernel.

[ ... ]
> +		if (local_offset < dax_ext_len) {
> +			loff_t ext_len_remainder = dax_ext_len - local_offset;
> +			struct dax_device *daxdev;
> +
> +			daxdev = famfs_daxdev_from_index(fsi,
> +						meta->se[i].dev_index, &rc);
> +			if (!daxdev) {
> +				meta->error = true;
> +				return rc;
> +			}

[Severity: Critical]
Are we returning an uninitialized iomap structure here?

Because famfs_daxdev_from_index() returns a 0 error code when valid == 1,
famfs_meta_to_dax_offset() treats a NULL daxdev as a success (rc = 0) but
bails out early, returning 0 without initializing iomap->addr or
iomap->length. The DAX core would then operate on garbage physical addresses.

[ ... ]
> +			iomap->addr    = dax_ext_offset + local_offset;
> +			iomap->offset  = file_offset;
> +			iomap->length  = min_t(loff_t, len, ext_len_remainder);
> +			iomap->dax_dev = daxdev;
> +			iomap->type    = IOMAP_MAPPED;
> +			iomap->flags   = flags;

[Severity: Critical]
Does assigning the input operational flags directly to the output mapping
flags overwrite valid data?

The flags argument to iomap_begin contains input flags (e.g., IOMAP_WRITE
which is 1 << 0). In the iomap output namespace, 1 << 0 is IOMAP_F_NEW.
Setting IOMAP_F_NEW tells the DAX core that the memory was newly allocated
and requires zeroing on partial writes, overwriting valid existing data with
zeroes.

[ ... ]
> @@ -501,7 +793,7 @@ famfs_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  		return rc;
>  	}
>  
> -	rc = dax_iomap_rw(iocb, from, NULL /*&famfs_iomap_ops*/);
> +	rc = dax_iomap_rw(iocb, from, &famfs_iomap_ops);
>  	inode_unlock(inode);
>  	return rc;
>  }

[Severity: High]
This is a pre-existing issue, but does famfs_dax_write_iter() bypass standard
VFS write checks?

It calls dax_iomap_rw() directly without invoking generic_write_checks() or
file_update_time(). This allows users to modify the contents of a setuid
binary without stripping the setuid bit, which is a classic privilege
escalation vector.


[Severity: Critical]
This is a pre-existing issue, but does FAMFSIOC_MAP_CREATE lack a privilege
check?

In famfs_file_init_dax() in fs/famfs/famfs_file.c:

	if (!famfs_opt_enabled(fsi, FAMFS_OPT_MAP_CREATE))
		return -EPERM;

Any unprivileged user with write access to a file could map and access
arbitrary physical memory on any registered DAX device. Should there be a
capable(CAP_SYS_ADMIN) or CAP_SYS_RAWIO check before parsing arbitrary physical
memory offsets? With the introduction of the iomap operations in this patch,
this allows the DAX core to perform I/O against unvalidated offsets.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019fc572ca94-ec363dd7-3a77-484b-b4b7-f2503a0931a6-000000@email.amazonses.com?part=8