Re: [PATCH V12 10/12] famfs: Add runtime operation-permission (opts) framework

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 on=
ly checks VM_WRITE.
- [High] rename() bypasses FAMFS_OPT_UNLINK and FAMFS_OPT_RMDIR restriction=
s 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;
>  };
> =20
> +/*
> + * 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 restrictio=
ns
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 obtai=
n 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.

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