[Severity: Low]
This fixes mount options escaping the ve_devmnt_process() checks, which
has been possible since the devmnt option filtering was introduced.
Should this carry a Fixes: tag so it gets backported together with the
feature, e.g.:
Fixes: 50a959cca34e4 ("ve/fs/devmnt: process mount options")
> diff --git a/fs/fs_context.c b/fs/fs_context.c
> index 76f34f3d468ea..fdaefc2276917 100644
> --- a/fs/fs_context.c
> +++ b/fs/fs_context.c
[ ... ]
> +/*
> + * For legacy mount(2), MS_* mount flags are folded into fc->sb_flags and are
> + * not present in the monolithic data string. Build a page with user data
> + * followed by those flags for ve_devmnt checks in vfs_parse_monolithic_sep.
> + *
> + * Returns @data when nothing needs to be added, a new page otherwise, or
> + * ERR_PTR() on failure. The caller must free_page() when the result !=
> @data.
> + */
[Severity: Low]
This isn't a bug, but the comment doesn't quite match the code: for a
container mount of a non-binary blockdev filesystem a new page is
returned even when there is nothing to add (no matching flags set,
empty or NULL @data). The callers only test result != @data, so they
still do the right thing, but the "Returns @data when nothing needs to
be added" wording suggests a narrower contract than the one
implemented. Returning @data when no flag names were appended would
also keep NULL data NULL, see the last comment below.
> +void *legacy_merge_mount_data(struct fs_context *fc, void *data)
> +{
> + struct ve_struct *ve = get_exec_env();
> + size_t off = 0;
> + char *page;
> + int err;
> +
> + if (ve_is_super(ve))
> + return data;
> +
> + if (!fc->fs_type || !(fc->fs_type->fs_flags & FS_REQUIRES_DEV))
> + return data;
[ ... ]
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 566f11a222fcf..0864fbd98131f 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
[ ... ]
> @@ -3357,13 +3358,17 @@ static int do_remount(struct path *path, int
> ms_flags, int sb_flags,
> */
> fc->oldapi = true;
>
> - err = ve_prepare_mount_options(fc, data);
> - if (err) {
> - put_fs_context(fc);
> - return err;
> + mnt_data = legacy_merge_mount_data(fc, data);
> + if (IS_ERR(mnt_data)) {
> + err = PTR_ERR(mnt_data);
> + goto err;
> }
>
> - err = parse_monolithic_mount_data(fc, data);
> + err = ve_prepare_mount_options(fc, mnt_data);
> + if (err)
> + goto free_mnt_data;
> +
> + err = parse_monolithic_mount_data(fc, mnt_data);
[Severity: Medium]
Doesn't this break remount with MS_DIRSYNC inside a container?
MS_DIRSYNC passes through path_mount() into do_remount()'s sb_flags:
sb_flags = flags & (SB_RDONLY |
SB_SYNCHRONOUS |
SB_MANDLOCK |
SB_DIRSYNC |
...
so legacy_merge_mount_data() appends "dirsync" to the options. Parsing
that string sets the corresponding bit in fc->sb_flags_mask:
fs/fs_context.c:vfs_parse_sb_flag()
fc->sb_flags |= token;
fc->sb_flags_mask |= token;
but MS_DIRSYNC is not part of MS_RMT_MASK, so reconfigure_super() now
rejects the whole request:
fs/super.c:reconfigure_super()
if (fc->sb_flags_mask & ~MS_RMT_MASK)
return -EINVAL;
Before this change (and still today on the host) MS_DIRSYNC was silently
ignored on remount, which is the historic legacy mount(2) behavior.
util-linux translates a "dirsync" option into MS_DIRSYNC, so remounting
a dirsync-mounted filesystem inside a container now fails with -EINVAL
where it used to succeed.
Would formatting only the remountable subset, e.g.
fc->sb_flags & fc->sb_flags_mask, on the reconfigure path avoid this?
[ ... ]
> @@ -3816,8 +3825,17 @@ static int do_new_mount(struct path *path, const char
> *fstype, int sb_flags,
> subtype, strlen(subtype));
> if (!err && name)
> err = vfs_parse_fs_string(fc, "source", name, strlen(name));
> - if (!err)
> - err = parse_monolithic_mount_data(fc, data);
> + if (!err) {
> + void *mnt_data = legacy_merge_mount_data(fc, data);
> +
> + if (IS_ERR(mnt_data)) {
> + err = PTR_ERR(mnt_data);
> + } else {
> + err = parse_monolithic_mount_data(fc, mnt_data);
> + if (mnt_data != data)
> + free_page((unsigned long)mnt_data);
> + }
> + }
[Severity: High]
Can this free the page while it is still referenced by the fs_context?
For a filesystem without init_fs_context (legacy mount API),
parse_monolithic_mount_data() resolves to legacy_parse_monolithic(),
which keeps the pointer instead of copying the data:
fs/fs_context.c:legacy_parse_monolithic()
ctx->legacy_data = data;
so the merged page is freed here and then used again further down the
same path:
do_new_mount()
parse_monolithic_mount_data(fc, mnt_data)
legacy_parse_monolithic() ctx->legacy_data = mnt_data
free_page(mnt_data)
vfs_get_tree(fc)
legacy_get_tree()
fc->fs_type->mount(..., ctx->legacy_data)
mount_bdev()
ve_devmnt_process() reads, and ve_devmnt_insert()
memmoves into, the freed page
fill_super() parses the freed page
The remount path is not affected because there legacy_reconfigure()
runs before the free_page() call, but it has a related problem: the
filesystem sees the appended flag names. Legacy parsers never had to
accept "ro", "sync" etc. as strings (the VFS consumed them as flags),
and e.g. ext2's parse_options() fails on unknown options, so a
container mount/remount with MS_RDONLY would be rejected by the
filesystem even with the lifetime issue fixed.
Every in-tree legacy blockdev filesystem (ext2, f2fs, jfs, ocfs2, hfs,
hfsplus, ufs, ...) is disabled in configs/kernel-6.12.0-x86_64.config,
so neither problem can trigger on the shipped configuration, but
nothing in the code prevents it once such a filesystem is enabled or
loaded from out of tree.
Since filesystems whose parse_monolithic is not generic_parse_monolithic
get their devmnt checks in mount_bdev()/ve_prepare_mount_options()
anyway, should legacy_merge_mount_data() (or its callers) skip
filesystems that use legacy_fs_context_ops?
[ ... ]
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
