[Devel] [PATCH VZ10 v5 1/1] fs: namespace: transform mount flags to comma separated values
Vasileios Almpanis
vasileios.almpanis at virtuozzo.com
Tue Jul 7 19:24:20 MSK 2026
Okay, I'll go a different direction with v6. The filesystem parser will
receive exactly the original data it got before this patch. The
synthesized MS_* flag names are used only for the ve_devmnt check, never
handed to the fs or merged into sb_flags_mask.
The check moves into vfs_parse_monolithic_sep(), reusing the block
device it already resolves for the option string. This fixes both
findings at once:
1. UAF is gone. The fs only ever sees the caller-owned original data, so
there is no merged page to free early and nothing dangling for
legacy_get_tree(). do_new_mount() no longer allocates or frees a page at
all.
2. No flag leak into legacy parsers, since ro/sync/... never reach
fill_super()/remount_fs().
Two nice side effects: the check is now genuinely symmetric for
fsconfig(2) (it reaches the same spot via the lazy_opts finalization),
and it closes a TOCTOU, since the flags are vetted against the same
device resolution as the options rather than a separate
lookup_bdev(fc->source). Legacy filesystems whose ->reconfigure never
reaches vfs_parse_monolithic_sep() keep ve_check_mount_options() on the
remount path, run against the pinned superblock device.
About point 3. its not exactly like that. Remount would pass but only in
old containers were the mount utility uses mount syscall and not
fsconfig. When fsconfig is used the remount would still be blocked if
the container doesn't have the correct options specified in ve.mount_opts
Will send v6 shortly.
On 7/7/26 1:41 PM, Konstantin Khorenko wrote:
> ● Review 8feef3f0950f5 "fs: namespace: transform mount flags to comma separated values"
>
> The idea of the patch is correct (close the ve_devmnt check bypass via MS_* flags), the implementation for new-API
> filesystems (ext4, xfs) looks right. But there is one structural use-after-free bug and a couple of remarks.
>
> 1. Use-after-free in do_new_mount() for legacy filesystems (main finding)
>
> legacy_parse_monolithic() does not copy the data, it stores the pointer (fs/fs_context.c:951):
>
> ctx->legacy_data = data;
>
> In do_new_mount() (fs/namespace.c:3828) the page is freed right after parsing:
>
> err = parse_monolithic_mount_data(fc, mnt_data);
> if (mnt_data != data)
> free_page((unsigned long)mnt_data); /* <-- ctx->legacy_data is now dangling */
> ...
> if (!err)
> err = vfs_get_tree(fc); /* legacy_get_tree -> fs_type->mount(..., ctx->legacy_data) */
>
> Further on, mount_bdev() runs ve_devmnt_process() on the freed page (including a write via memmove in ve_devmnt_insert()
> for hidden options) and passes it to fill_super(). That is, both reads and writes into freed memory.
>
> Conditions: a process in a CT + a legacy filesystem (without init_fs_context) with FS_REQUIRES_DEV and without
> FS_BINARY_MOUNTDATA. Such filesystems exist in the tree (ext2, f2fs, jfs, ocfs2, hfs/hfsplus, ufs, ...), but in the
> vzkernel config they are all disabled - i.e. in the product build the bug is currently unreachable. Nevertheless it is a
> landmine: enabling any such filesystem in the config, or a third-party legacy module, makes the UAF reachable from a
> container.
>
> In do_remount() it is done correctly - there free_mnt_data: comafter reconfigure_super(). In do_new_mount() it should be
> the same: free the page after vfs_get_tree() (before put_fs_context()), not right after parsing.
>
> 2. Merged flags leak into the legacy filesystem parser
>
> For new-API filesystems the added tokens "ro"/"sync"/"mand"/"dirsync"/"lazytime" are consumed by vfs_parse_sb_flag()
> before ->parse_param - harmless. But for legacy filesystems the merged string goes in its entirety to the filesystem's
> fill_super()/remount_fs(), whose parse_options does not know the "ro" option and will fail the mount with EINVAL. That is,
> mount -r /dev/xxx in a CT for a legacy filesystem will stop working even when the devmnt options are allowed. Same caveat
> about reachability as in item 1, but when fixing item 1 it is worth accounting for this: the merged string for legacy
> filesystems is better used only for the check, while the original data is passed to the filesystem.
>
> 3. Behavior change - to be documented (not a bug)
>
> - In a CT, a flag-only mount (mount -r, -o remount,ro) of a device that has no entry in devmnt_list used to pass before
> (data == NULL -> no checks), now the merged string is non-empty -> EPERM. This is precisely the goal of the patch, but
> existing deployments where "ro"/"sync"/"lazytime" are not listed in --devmnt allowed_options will get a denial after the
> update. It is worth explicitly warning QA/support and possibly the tooling (vzctl).
> - data == NULL now turns into an empty string "" for all dev mounts in a CT - by the trace (vfs_parse_monolithic_sep,
> security_sb_eat_lsm_opts, ve_devmnt_process case 1 with the [0] == '\0' check) this is safe.
>
> 4. Minor things
>
> - If fc->sb_flags contains none of the flags from the table and data is already non-empty, legacy_merge_mount_data() still
> allocates a page and copies data for nothing. It could return data immediately if !(fc->sb_flags &
> (SB_DIRSYNC|SB_LAZYTIME|SB_MANDLOCK|SB_RDONLY|SB_SYNCHRONOUS)).
> - The err: label in do_remount() matches the name of the err variable - legal, but reads poorly; the usual style is
> out:/out_free:.
> - Edge case: if the user option string is close to PAGE_SIZE, adding the flags now yields -E2BIG where the mount used to
> pass before. Extremely unlikely in practice, just noting it.
>
> --
> Best regards,
>
> Konstantin Khorenko,
> Virtuozzo Linux Kernel Team
>
> On 6/26/26 11:13, Vasileios Almpanis wrote:
>> In legacy mount callpaths, userspace might pass mount options as
>> flags. These flags escape our checks in ve_devmnt_process allowing
>> devices to be mounted inside containers with options not specified in
>> the allowed field. Introduce helpers that take these flags and
>> already existing tables of flag -> string representation to construct
>> a comma separated value string from them, and append them to userspace
>> provided data. Then pass this string to parse_monolithic_mount_data
>> enforcing the same checks symmetrically in both mount and fsconfig
>> syscalls.
>>
>> In the remount path, run legacy_merge_mount_data() before
>> ve_devmnt_process() so container device mount policy sees MS_* flags
>> from the legacy mount(2) API, not only the user-supplied option string.
>> Keep ve_prepare_mount_options() for legacy parsers that do not use
>> generic_parse_monolithic().
>>
>> https://virtuozzo.atlassian.net/browse/VSTOR-132330
>> Signed-off-by: Vasileios Almpanis <vasileios.almpanis at virtuozzo.com>
>>
>> Feature: ve: ve generic structures
>> ---
>> Changes since v4:
>> - Don't emit the negative/clear sb flag names (rw, async, nomand,
>> nolazytime) in vfs_format_sb_flags(); emit only the positive names
>> (common_set_sb_flag). On the legacy remount path sb_flags_mask is the
>> fixed MS_RMT_MASK, so the previous code appended the clear-names for
>> every unset remountable bit, and ve_devmnt_check() then rejected
>> ordinary remounts within the container not on the host(ve0).
>> - Fix uninitialized mount-options page: NUL-terminate the buffer before
>> returning it from legacy_merge_mount_data(). When data is empty/NULL
>> and no flags are emitted, off stayed 0 and the page from
>> __get_free_page() was returned non-terminated.
>> - Fix __vfs_format_flags() comment: it returns -E2BIG, not -ENOSPC.
>>
>> Changes since v3:
>> - Drop excess length check in legacy_merge_mount_data
>>
>> Changes since v2:
>> - Remove legacy_merge_mount_data guard in fs/internal.h. All helpers
>> don't use anything that would break build and just unchanged pointer
>> will be returned.
>> - Add __vfs_format_flags helper and use it in vfs_format_sb_flags
>> - Fix inconsistent error code: replace -ENOSPC with -E2BIG for the
>> buffer-too-small case
>>
>> Changes since v1:
>> - Replace open-coded flag loops with append_entry() helper (pointer-
>> advancing style) that unifies the comma-insert-copy pattern across
>> all three call sites
>> - Rework legacy_merge_mount_data() to allocate the page upfront, write
>> user data first then append sb flags via vfs_format_sb_flags(); this
>> eliminates the intermediate flags_buf[128], the total size calculation,
>> and the +1/+2 arithmetic
>> - Fix inconsistent error code: replace -EINVAL with -ENOSPC for the
>> buffer-too-small case
>> - Add comment on FS_BINARY_MOUNTDATA explaining why those filesystems
>> are skipped
>> - Add blank line before return in legacy_merge_mount_data()
>> - Remove excess blank line after parse_monolithic_mount_data() in
>> do_remount()
>>
>> fs/fs_context.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++
>> fs/internal.h | 1 +
>> fs/namespace.c | 32 ++++++++++++++----
>> 3 files changed, 116 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/fs_context.c b/fs/fs_context.c
>> index 76f34f3d468e..fdaefc227691 100644
>> --- a/fs/fs_context.c
>> +++ b/fs/fs_context.c
>> @@ -81,6 +81,96 @@ static int vfs_parse_sb_flag(struct fs_context *fc, const char *key)
>> return -ENOPARAM;
>> }
>>
>> +/*
>> + * Emit, into @buff at *@off, the comma-separated names of every entry in @p
>> + * whose bit is set in @flags. Advances *@off past the written text.
>> + * Returns 0 on success or -E2BIG if the buffer is too small.
>> + */
>> +static int __vfs_format_flags(const struct constant_table *p, unsigned int flags,
>> + char *buff, size_t size, size_t *off)
>> +{
>> + for (; p->name; p++) {
>> + ssize_t ret;
>> +
>> + if (!(flags & p->value))
>> + continue;
>> +
>> + if (*off) {
>> + if (*off + 1 >= size)
>> + return -E2BIG;
>> + buff[(*off)++] = ',';
>> + }
>> +
>> + ret = strscpy(buff + *off, p->name, size - *off);
>> + if (ret < 0)
>> + return -E2BIG;
>> + *off += ret;
>> + }
>> + return 0;
>> +}
>> +
>> +static int vfs_format_sb_flags(struct fs_context *fc, char *buff, size_t size,
>> + size_t *off)
>> +{
>> + return __vfs_format_flags(common_set_sb_flag, fc->sb_flags,
>> + buff, size, off);
>> +}
>> +
>> +/*
>> + * 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.
>> + */
>> +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;
>> +
>> + /*
>> + * Filesystems with binary mount data (e.g. btrfs) bypass option
>> + * string parsing entirely, so our checks cannot apply here.
>> + */
>> + if (fc->fs_type->fs_flags & FS_BINARY_MOUNTDATA)
>> + return data;
>> +
>> + page = (char *)__get_free_page(GFP_KERNEL);
>> + if (!page)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + if (data && *(char *)data) {
>> + ssize_t ret = strscpy(page, data, PAGE_SIZE);
>> +
>> + if (ret < 0) {
>> + err = -E2BIG;
>> + goto err_free;
>> + }
>> + off = ret;
>> + }
>> +
>> + err = vfs_format_sb_flags(fc, page, PAGE_SIZE, &off);
>> + if (err)
>> + goto err_free;
>> +
>> + page[off] = '\0';
>> +
>> + return page;
>> +
>> +err_free:
>> + free_page((unsigned long)page);
>> + return ERR_PTR(err);
>> +}
>> +
>> /**
>> * vfs_parse_fs_param_source - Handle setting "source" via parameter
>> * @fc: The filesystem context to modify
>> diff --git a/fs/internal.h b/fs/internal.h
>> index 1b5cb1cda2e4..e823b69dd077 100644
>> --- a/fs/internal.h
>> +++ b/fs/internal.h
>> @@ -46,6 +46,7 @@ extern void __init chrdev_init(void);
>> */
>> extern const struct fs_context_operations legacy_fs_context_ops;
>> extern int parse_monolithic_mount_data(struct fs_context *, void *);
>> +extern void *legacy_merge_mount_data(struct fs_context *fc, void *data);
>> extern void vfs_clean_context(struct fs_context *fc);
>> extern int finish_clean_context(struct fs_context *fc);
>>
>> diff --git a/fs/namespace.c b/fs/namespace.c
>> index 566f11a222fc..0864fbd98131 100644
>> --- a/fs/namespace.c
>> +++ b/fs/namespace.c
>> @@ -3337,6 +3337,7 @@ static int do_remount(struct path *path, int ms_flags, int sb_flags,
>> struct super_block *sb = path->mnt->mnt_sb;
>> struct mount *mnt = real_mount(path->mnt);
>> struct fs_context *fc;
>> + void *mnt_data = NULL;
>>
>> if (!check_mnt(mnt))
>> return -EINVAL;
>> @@ -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);
>> if (!err) {
>> down_write(&sb->s_umount);
>> err = -EPERM;
>> @@ -3380,6 +3385,10 @@ static int do_remount(struct path *path, int ms_flags, int sb_flags,
>>
>> mnt_warn_timestamp_expiry(path, &mnt->mnt);
>>
>> +free_mnt_data:
>> + if (mnt_data != data)
>> + free_page((unsigned long)mnt_data);
>> +err:
>> put_fs_context(fc);
>> return err;
>> }
>> @@ -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);
>> + }
>> + }
>> if (!err && !mount_capable(fc))
>> err = -EPERM;
>> if (!err)
--
Best regards, Vasileios Almpanis
Software Developer, Virtuozzo.
More information about the Devel
mailing list