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 <[email protected]>

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.

_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel

Reply via email to