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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Ignored return value of kiocb_modified() in famfs_dax_write_iter bypasses security state updates.
- [High] Bypass of generic_write_checks() breaks O_APPEND semantics and incorrectly returns 0 on EOF, causing infinite write loops.
- [High] 64-bit file size is truncated to 32-bit size_t, breaking I/O operations on files larger than 4GB on 32-bit platforms.
- [Medium] famfs_dax_read_iter() and famfs_dax_write_iter() ignore IOCB_NOWAIT and use blocking lock acquisitions.
- [Medium] Kernel pointer leak via pr_debug by intentionally bypassing pointer hashing.
--

commit 31958b8d418a1061bd63bd9bed3ba0558707e229
Author: John Groves <john@groves.net>

    famfs: Introduce file_operations read/write

This commit introduces fs/famfs/famfs_file.c and the famfs
file_operations for read/write.

> diff --git a/fs/famfs/famfs_file.c b/fs/famfs/famfs_file.c
> new file mode 100644
> index 0000000000000..1369fe1824bc8
> --- /dev/null
> +++ b/fs/famfs/famfs_file.c
> @@ -0,0 +1,141 @@

[ ... ]

> +/* 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: Medium]
Does this cast to u64 and %llx format specifier bypass the kernel's pointer
hashing mechanism, potentially leaking raw kernel memory addresses?

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

[Severity: High]
Does truncating the 64-bit loff_t returned by i_size_read() into a 32-bit
size_t variable break I/O operations for files larger than 4GB on 32-bit
platforms?

> +	size_t count = iov_iter_count(ubuf);
> +	size_t max_count;
> +	ssize_t rc;
> +
> +	if (fsi->deverror)
> +		return -ENODEV;
> +
> +	rc = famfs_file_invalid(inode);
> +	if (rc)
> +		return rc;
> +
> +	/* Avoid unsigned underflow if position is past EOF */
> +	if (iocb->ki_pos >= i_size)
> +		max_count = 0;
> +	else
> +		max_count = i_size - iocb->ki_pos;
> +
> +	if (count > max_count)
> +		iov_iter_truncate(ubuf, max_count);
> +
> +	if (!iov_iter_count(ubuf))
> +		return 0;

[Severity: High]
When a write hits the end of the file, this logic returns 0 instead of a
proper error code like -ENOSPC or -EFBIG.  Can this cause infinite write
loops in standard userspace tools?

> +
> +	return rc;
> +}
> +
> +static ssize_t
> +famfs_dax_read_iter(struct kiocb *iocb, struct iov_iter	*to)
> +{
> +	struct inode *inode = iocb->ki_filp->f_mapping->host;
> +	ssize_t rc;
> +
> +	/* dax_iomap_rw() requires i_rwsem held (shared for read) */
> +	inode_lock_shared(inode);

[Severity: Medium]
Does this unconditional blocking lock acquisition ignore the IOCB_NOWAIT
flag, potentially stalling asynchronous I/O frameworks like io_uring?

> +	rc = famfs_rw_prep(iocb, to);
> +	if (rc || !iov_iter_count(to)) {
> +		inode_unlock_shared(inode);
> +		return rc;
> +	}
> +
> +	rc = dax_iomap_rw(iocb, to, NULL /*&famfs_iomap_ops */);
> +	inode_unlock_shared(inode);
> +
> +	if (rc > 0)
> +		file_accessed(iocb->ki_filp);
> +	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 = iocb->ki_filp->f_mapping->host;
> +	struct famfs_fs_info *fsi = 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);

[Severity: Medium]
Similar to famfs_dax_read_iter(), does this unconditional blocking lock
acquisition ignore the IOCB_NOWAIT flag, potentially stalling asynchronous
I/O frameworks?

> +	rc = famfs_rw_prep(iocb, from);

[Severity: High]
Does the bypass of generic_write_checks() here break O_APPEND semantics?
It appears iocb->ki_pos is not updated to EOF, causing the write to
overwrite existing data instead of appending.

> +	if (rc || !iov_iter_count(from)) {
> +		inode_unlock(inode);
> +		return rc;
> +	}
> +
> +	kiocb_modified(iocb); /* mtime/ctime + strip set[e]uid */

[Severity: High]
Does ignoring the return value of kiocb_modified() bypass error checking?
If stripping the setuid/setgid bit fails, could this allow a malicious
user to retain privileges on a file while modifying its contents?

> +
> +	rc = dax_iomap_rw(iocb, from, NULL /*&famfs_iomap_ops*/);
> +	inode_unlock(inode);
> +	return rc;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/010001a04e801a4e-8eb212cd-b263-4043-ab65-33e480d2d7d4-000000@email.amazonses.com?part=5