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 <[email protected]> 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-000...@email.amazonses.com?part=5
