Re: [PATCH V12 05/12] famfs: Introduce file_operations read/write

Thank you for your contribution! Sashiko AI review found 7 potential issue(=
s) to consider:
- [High] The custom write iterator famfs_dax_write_iter() omits the require=
d VFS file_modified() / file_remove_privs() hooks, bypassing setuid/setgid =
privilege stripping and timestamp updates.
- [High] famfs_dax_write_iter() bypasses POSIX append semantics and file si=
ze limits by omitting generic_write_checks().
- [High] famfs_rw_prep() truncates the 64-bit file size to 32 bits on 32-bi=
t architectures, breaking bounds checking and causing artificial IO limits.
- [High] Using filemap_splice_read on a DAX filesystem initialized with ram=
_aops will cause splice reads to silently return streams of zeroes.
- [Medium] Writes past the end of the file return 0 instead of an error, wh=
ich can cause infinite loops in userspace.
- [Low] file_accessed() is called unconditionally on reads, updating access=
 time even on errors or EOF.
- [Low] Kernel heap address leak via pr_debug using %llx on an inode pointe=
r.
--

--- Patch [5]: [PATCH V12 05/12] famfs: Introduce file_operations read/writ=
e ---
commit 025e683c34185b254d9bc60d68aabd4be0342a87
Author: John Groves <[email protected]>

famfs: Introduce file_operations read/write

This commit introduces fs/famfs/famfs_file.c and the famfs
file_operations for read/write. It sets up the groundwork for
DAX I/O, though it currently calls dax_iomap_rw with NULL iomap_ops.

> diff --git a/fs/famfs/famfs_file.c b/fs/famfs/famfs_file.c
> new file mode 100644
> index 0000000000000..e192b573c51f7
> --- /dev/null
> +++ b/fs/famfs/famfs_file.c
> @@ -0,0 +1,138 @@
[ ... ]
> +/* Reject I/O to files that aren't in a valid state */
> +static ssize_t
> +famfs_file_invalid(struct inode *inode)
> +{
> +	if (!IS_DAX(inode)) {
> +		pr_debug("%s: inode %llx IS_DAX is false\n",
> +			 __func__, (u64)inode);

[Severity: Low]
Does this explicit cast to u64 leak the raw kernel heap address of the inod=
e?

When dynamic debugging is enabled and famfs_file_invalid() is called on an
inode without S_DAX, bypassing the kernel's %p hashing protection could exp=
ose
the heap layout to dmesg. Could this assist attackers in bypassing KASLR?

> +		return -ENXIO;
> +	}
> +	return 0;
> +}
> +
> +static ssize_t
> +famfs_rw_prep(struct kiocb *iocb, struct iov_iter *ubuf)
> +{
> +	struct inode *inode =3D iocb->ki_filp->f_mapping->host;
> +	struct super_block *sb =3D inode->i_sb;
> +	struct famfs_fs_info *fsi =3D sb->s_fs_info;
> +	size_t i_size =3D i_size_read(inode);

[Severity: High]
Does this truncate the 64-bit loff_t file size to 32 bits on 32-bit
architectures?

When reading or writing a famfs file larger than 4GB on a 32-bit system,
i_size_read() will be truncated into the 32-bit size_t i_size. Will this br=
eak
bounds checking and make data past the 32-bit wrapped boundary inaccessible?

> +	size_t count =3D iov_iter_count(ubuf);
> +	size_t max_count;
> +	ssize_t rc;
[ ... ]
> +	/* Avoid unsigned underflow if position is past EOF */
> +	if (iocb->ki_pos >=3D i_size)
> +		max_count =3D 0;
> +	else
> +		max_count =3D i_size - iocb->ki_pos;
> +
> +	if (count > max_count)
> +		iov_iter_truncate(ubuf, max_count);
> +
> +	if (!iov_iter_count(ubuf))
> +		return 0;

[Severity: Medium]
When writing at or past EOF (ki_pos >=3D i_size), famfs_rw_prep() sets max_=
count
to 0 and returns 0. Will famfs_dax_write_iter() bubble this 0 up to the
syscall instead of returning an error code like -ENOSPC or -EFBIG?

Userspace tools often interpret a 0-byte write return as a temporary
condition and retry indefinitely. Could this cause unprivileged applications
to hang in infinite loops?

> +
> +	return rc;
> +}
> +
> +static ssize_t
> +famfs_dax_read_iter(struct kiocb *iocb, struct iov_iter	*to)
> +{
[ ... ]
> +	rc =3D dax_iomap_rw(iocb, to, NULL /*&famfs_iomap_ops */);
> +	inode_unlock_shared(inode);
> +
> +	file_accessed(iocb->ki_filp);

[Severity: Low]
Should file_accessed() only be called if the read successfully transferred
data (rc > 0)?

Unconditionally calling file_accessed() in famfs_dax_read_iter() will update
access times and generate metadata traffic even for failed reads or zero-by=
te
EOF reads.

> +	return rc;
> +}
> +
> +/**
> + * famfs_dax_write_iter()
> + *
> + * We need our own write-iter in order to prevent append
> + *
> + * @iocb:
> + * @from: iterator describing the user memory source for the write
> + */
> +static ssize_t
> +famfs_dax_write_iter(struct kiocb *iocb, struct iov_iter *from)
> +{
> +	struct inode *inode =3D iocb->ki_filp->f_mapping->host;
> +	struct famfs_fs_info *fsi =3D inode->i_sb->s_fs_info;
> +	ssize_t rc;
> +
> +	if (!famfs_opt_enabled(fsi, FAMFS_OPT_WRITE))
> +		return -EPERM;
> +
> +	/* dax_iomap_rw() requires i_rwsem held (exclusive for write) */
> +	inode_lock(inode);
> +	rc =3D famfs_rw_prep(iocb, from);

[Severity: High]
Does famfs_dax_write_iter() bypass POSIX append semantics and file size lim=
its
by omitting generic_write_checks() here?

Without generic_write_checks() before preparing the IO, any user process
opening a file with O_APPEND or a restricted RLIMIT_FSIZE can write directly
to the current offset and bypass those limits.

> +	if (rc || !iov_iter_count(from)) {
> +		inode_unlock(inode);
> +		return rc;
> +	}
> +
> +	rc =3D dax_iomap_rw(iocb, from, NULL /*&famfs_iomap_ops*/);

[Severity: High]
Does this bypass setuid/setgid privilege stripping and timestamp updates?

The famfs_dax_write_iter() function modifies the file via dax_iomap_rw()
without calling the required VFS hooks like file_modified() or
file_remove_privs(). Could this omission allow unprivileged users to escala=
te
privileges on SUID/SGID files they have write access to?

> +	inode_unlock(inode);
> +	return rc;
> +}
> +
> +const struct file_operations famfs_file_operations =3D {
> +	.owner             =3D THIS_MODULE,
> +
[ ... ]
> +	/* Generic Operations */
> +	.fsync		   =3D noop_fsync,
> +	.splice_read	   =3D filemap_splice_read,

[Severity: High]
Is filemap_splice_read the correct operation for a DAX filesystem?

Because famfs inodes lack the S_DAX flag, VFS falls back to using
filemap_splice_read. This function queries the page cache and allocates new
pages using ram_aops.read_folio (which resolves to simple_read_folio and ze=
ro
fills them). Will userspace applications using splice() or sendfile() recei=
ve
a stream of zeroes instead of the actual DAX memory contents?

> +	.splice_write	   =3D iter_file_splice_write,
> +	.llseek		   =3D generic_file_llseek,
> +};

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/0100019fc572ca94-ec=
[email protected]?part=3D5