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 required 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 size limits by omitting generic_write_checks(). - [High] famfs_rw_prep() truncates the 64-bit file size to 32 bits on 32-bit 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, which 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 pointer. --
--- Patch [5]: [PATCH V12 05/12] famfs: Introduce file_operations read/write --- 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 inode? 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 expose 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 = 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 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 break bounds checking and make data past the 32-bit wrapped boundary inaccessible? > + size_t count = iov_iter_count(ubuf); > + size_t max_count; > + ssize_t 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: Medium] When writing at or past EOF (ki_pos >= 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 = 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-byte 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 = 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); > + rc = famfs_rw_prep(iocb, from); [Severity: High] Does famfs_dax_write_iter() bypass POSIX append semantics and file size limits 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 = 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 escalate privileges on SUID/SGID files they have write access to? > + inode_unlock(inode); > + return rc; > +} > + > +const struct file_operations famfs_file_operations = { > + .owner = THIS_MODULE, > + [ ... ] > + /* Generic Operations */ > + .fsync = noop_fsync, > + .splice_read = 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 zero fills them). Will userspace applications using splice() or sendfile() receive a stream of zeroes instead of the actual DAX memory contents? > + .splice_write = iter_file_splice_write, > + .llseek = generic_file_llseek, > +}; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/0100019fc572ca94-ec363dd7-3a77-484b-b4b7-f2503a0931a6-000...@email.amazonses.com?part=5
