Re: [PATCH V12 02/12] famfs: Module operations, fs_context, and mount

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing `.owner = THIS_MODULE` in `famfs_fs_type`.
- [High] Missing DAX device validation and reference counting risks Use-After-Free and TOCTOU races.
- [Medium] Silent ignoring of unrecognized and invalid mount parameters in `famfs_parse_param()`.
- [Medium] Bypassing MAC and DAC permission checks on the character device node during mount.
- [Low] Global symbol `lookup_daxdev` lacks a subsystem prefix.
--

--- Patch [2]: [PATCH V12 02/12] famfs: Module operations, fs_context, and mount ---
commit fdda77a79254c638517e00787a7301ff9eaab82f
Author: John Groves <john@groves.net>

famfs: Module operations, fs_context, and mount

Start building up from the famfs module operations. Register as a file
system, parse mount parameters, allocate superblock via famfs_get_tree(),
and lookup the host dax device.

> diff --git a/fs/famfs/famfs_inode.c b/fs/famfs/famfs_inode.c
> new file mode 100644
> index 0000000000000..c299a90912a5c
> --- /dev/null
> +++ b/fs/famfs/famfs_inode.c

[ ... ]

> +static void
> +famfs_fill_super(struct super_block *sb, struct fs_context *fc)
> +{
> +	sb->s_maxbytes		= MAX_LFS_FILESIZE;
> +	sb->s_blocksize		= PAGE_SIZE;
> +	sb->s_blocksize_bits	= PAGE_SHIFT;
> +	sb->s_magic		= FAMFS_SUPER_MAGIC;
> +	sb->s_op		= NULL /* famfs_super_ops */;
> +	sb->s_time_gran		= 1;
> +}
> +
> +int
> +lookup_daxdev(const char *pathname, dev_t *devno)

[Severity: Low]
Should this globally visible function have a subsystem-specific prefix like
famfs_lookup_daxdev to avoid polluting the global namespace?

> +{
> +	struct inode *inode;
> +	struct path path;
> +	int err;
> +
> +	if (!pathname || !*pathname)
> +		return -EINVAL;
> +
> +	err = kern_path(pathname, LOOKUP_FOLLOW, &path);
> +	if (err)
> +		return err;
> +
> +	inode = d_backing_inode(path.dentry);
> +	if (!S_ISCHR(inode->i_mode)) {
> +		err = -EINVAL;
> +		goto out_path_put;
> +	}
> +
> +	if (!may_open_dev(&path)) {
> +		err = -EACCES;
> +		goto out_path_put;
> +	}

[Severity: Medium]
Does this bypass MAC and DAC permission checks on the character device node?

By only resolving the path with kern_path() and checking may_open_dev(),
the code skips the actual file open operation. Would this allow a user with
mount capabilities to bypass security policies that restrict access to the
underlying device?

> +
> +	/* i_rdev is the char dev_t; fs_dax_get() confirms it is dax later */
> +	*devno = inode->i_rdev;

[Severity: High]
Is the underlying DAX device structure pinned to prevent it from being freed?

The comment mentions fs_dax_get() will confirm it is DAX later, but it
doesn't appear to be called anywhere in the codebase to acquire the
device. Could this allow the backing driver to be unloaded or hot-unplugged
while the filesystem is mounted, resulting in a use-after-free on
subsequent access?

> +
> +out_path_put:
> +	path_put(&path);
> +	return err;
> +}

[ ... ]

> +static int famfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
> +{
> +	struct famfs_fs_info *fsi = fc->s_fs_info;
> +	struct fs_parse_result result;
> +	int opt;
> +
> +	opt = fs_parse(fc, famfs_fs_parameters, param, &result);
> +	if (opt == -ENOPARAM) {
> +		opt = vfs_parse_fs_param_source(fc, param);
> +		if (opt != -ENOPARAM)
> +			return opt;
> +
> +		return 0;
> +	}

[Severity: Medium]
Does returning 0 here silently ignore unrecognized mount parameters?

The VFS layer relies on fs_parse() returning -ENOPARAM to indicate an
unhandled parameter so it can alert the user. Does catching the error and
returning 0 hide misconfigurations from userspace?

> +	if (opt < 0)
> +		return opt;
> +
> +	switch (opt) {
> +	case Opt_mode:
> +		fsi->mount_opts.mode = result.uint_32 & S_IALLUGO;
> +		break;
> +	case Opt_dax:
> +		if (strcmp(param->string, "always"))
> +			pr_debug("%s: invalid dax mode %s\n",
> +				  __func__, param->string);
> +		break;

[Severity: Medium]
Should this return an error code instead of breaking and eventually
returning 0 when an invalid DAX mode is provided?

> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static void famfs_kill_sb(struct super_block *sb)
> +{
> +	struct famfs_fs_info *fsi = sb->s_fs_info;
> +
> +	kill_char_super(sb);
> +
> +	kfree(fsi);
> +	sb->s_fs_info = NULL;
> +}
> +
> +#define MODULE_NAME "famfs"
> +static struct file_system_type famfs_fs_type = {
> +	.name		  = MODULE_NAME,
> +	.init_fs_context  = famfs_init_fs_context,
> +	.parameters	  = famfs_fs_parameters,
> +	.kill_sb	  = famfs_kill_sb,
> +	.fs_flags	  = FS_REQUIRES_DEV,
> +};

[Severity: High]
Is the .owner field missing from this file_system_type structure?

Without .owner = THIS_MODULE, the VFS will not increment the module
reference count when a famfs filesystem is mounted. Could this allow the
module to be unexpectedly unloaded via rmmod while active superblocks and
inodes are still in use, leading to a kernel oops and use-after-free
vulnerabilities?

> +
> +/******************************************************************************
> + * Module stuff
> + */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019fc572ca94-ec363dd7-3a77-484b-b4b7-f2503a0931a6-000000@email.amazonses.com?part=2