Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] mprotect() can bypass FAMFS_OPT_WRITE restrictions because mmap only checks VM_WRITE. - [High] rename() bypasses FAMFS_OPT_UNLINK and FAMFS_OPT_RMDIR restrictions for target files and directories. --
--- Patch [10]: [PATCH V12 10/12] famfs: Add runtime operation-permission (opts) framework --- commit e932914ca4460e6b742e80deb76ae5e6b7bac595 Author: John Groves <[email protected]> famfs: Add runtime operation-permission (opts) framework famfs denies most namespace, attribute and data operations by default because the userspace log, not the kernel, is authoritative for a famfs instance. Earlier commits already guard each such operation with a famfs_opt_enabled(fsi, FAMFS_OPT_x) check backed by a permissive stub. This commit defines the permission bitmap and makes those checks live. > diff --git a/include/uapi/linux/famfs_ioctl.h > b/include/uapi/linux/famfs_ioctl.h > index 751d8b033c2ee..efe6ef2639759 100644 > --- a/include/uapi/linux/famfs_ioctl.h > +++ b/include/uapi/linux/famfs_ioctl.h > @@ -100,6 +100,48 @@ struct famfs_ioc_daxdev { > __u32 flags; > }; > > +/* > + * Mount-wide operation permissions, queried and modified via the > + * FAMFSIOC_{GET,SET,CLEAR}_OPTS ioctls. A set bit means the operation is > + * permitted; a clear bit means it is rejected with -EPERM. famfs denies most > + * of these by default because the userspace log, not the kernel, is > + * authoritative for a famfs instance. > + */ > +#define FAMFS_OPT_CREATE (1ULL << 0) /* create a regular file */ > +#define FAMFS_OPT_MKDIR (1ULL << 1) /* mkdir > */ > +#define FAMFS_OPT_MKNOD (1ULL << 2) /* mknod a special file > */ > +#define FAMFS_OPT_SYMLINK (1ULL << 3) /* create a symlink */ > +#define FAMFS_OPT_LINK (1ULL << 4) /* hard link > */ > +#define FAMFS_OPT_UNLINK (1ULL << 5) /* unlink a mapped file */ > +#define FAMFS_OPT_RMDIR (1ULL << 6) /* rmdir > */ > +#define FAMFS_OPT_RENAME (1ULL << 7) /* rename */ [Severity: High] Does this bypass the FAMFS_OPT_UNLINK and FAMFS_OPT_RMDIR policy restrictions for target files and directories? If FAMFS_OPT_RENAME is enabled but FAMFS_OPT_UNLINK is disabled, a user can rename a file over an existing target mapped file. In fs/famfs/famfs_inode.c:famfs_rename(): if (!famfs_opt_enabled(fsi, FAMFS_OPT_RENAME)) return -EPERM; return simple_rename(idmap, old_dir, old_dentry, new_dir, new_dentry, flags); This delegates to simple_rename(), which silently replaces the target by calling simple_unlink() (or simple_rmdir()) on it. This directly drops the inode link count and bypasses the filesystem's famfs_unlink() and famfs_rmdir() hooks, completely avoiding the FAMFS_OPT_UNLINK check. > +#define FAMFS_OPT_CHMOD (1ULL << 8) /* setattr ATTR_MODE > */ > +#define FAMFS_OPT_CHOWN (1ULL << 9) /* setattr ATTR_UID / > ATTR_GID */ > +#define FAMFS_OPT_TRUNCATE (1ULL << 10) /* setattr ATTR_SIZE (resize) */ > +#define FAMFS_OPT_UTIMES (1ULL << 11) /* setattr ATTR_ATIME/ATTR_MTIME*/ > +#define FAMFS_OPT_WRITE (1ULL << 12) /* write file data > */ [Severity: High] Does this allow mprotect() to bypass FAMFS_OPT_WRITE restrictions and obtain a writable memory mapping to a file when it should be read-only? In fs/famfs/famfs_file.c:famfs_file_mmap(): if ((vma->vm_flags & VM_SHARED) && (vma->vm_flags & VM_WRITE) && !famfs_opt_enabled(fsi, FAMFS_OPT_WRITE)) return -EPERM; The permission check only validates VM_WRITE. If a user calls mmap with PROT_READ | MAP_SHARED, it sets VM_MAYWRITE but not VM_WRITE, bypassing the FAMFS_OPT_WRITE check. The user could then call mprotect(PROT_WRITE) to upgrade the mapping to writable without any filesystem callbacks. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/0100019fc572ca94-ec363dd7-3a77-484b-b4b7-f2503a0931a6-000...@email.amazonses.com?part=10
