From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pavel Tikhomirov Date: Wed, 5 Aug 2026 18:41:53 +0200 Subject: Re: [Devel] [PATCH vz10 v8 1/1] fs: enforce container device-mount policy in the common mount path In-Reply-To: <20260804125408.448664-1-vasileios.almpanis@virtuozzo.com> References: <20260804125408.448664-1-vasileios.almpanis@virtuozzo.com> Message-ID: <7a3e7cf9-8890-4ff8-9137-557e20654c0b@virtuozzo.com> List-Id: Reviewed-by: Pavel Tikhomirov On 8/4/26 14:53, Vasileios Almpanis wrote: > In a container the per-device ve_devmnt policy restricts which options > a device may be mounted with and force-inserts a set of hidden options. > The check used to run inside the option-string parser > (vfs_parse_monolithic_sep) and, for remount, in a separate helper. Two > things escaped it: > > - MS_* flags from the legacy mount(2)/fsconfig(2) API are folded into > fc->sb_flags and never appear in the option string, so a container > could set MS_RDONLY, MS_SYNCHRONOUS, MS_MANDLOCK, ... outside its > allowed set. > > - The check sat in filesystem-selectable callbacks (->parse_monolithic, > ->mount), so a filesystem not routing through them evaded the policy, > and a skipped hidden-option insertion dropped a container's mandated > options without error. > > Enforce the policy in the fs-agnostic common mount path instead: > vfs_get_tree() for a new mount and reconfigure_super() for a remount. > The device is taken from the mounted superblock, so fc->source cannot be > raced to target another device, and fc->sb_flags is vetted alongside the > option string. On a new mount the forced options must also be present, > so a filesystem that skipped inserting them has its mount refused rather > than silently losing them. > > The parse-time ve_devmnt_process() call is kept as a best-effort early > reject, so a disallowed device or option is refused before the > filesystem's fill_super() runs. > > Fix a bug where a containers that use the legacy mount(2) syscall are able > to reconfigure a mount and change the superblock flags, for example > from RO to RW. Compute the effective superblock flags and emit rw incase > SB_RDONLY is missing in vfs_format_sb_flags. > > Explicitly deny mounts for device backed legacy filesystems. So even > if FS_VIRTUALISED is added to them, mounting will not succeed since > legacy filesystems don't go through vfs_parse_monolithic_sep and the > policy is not applied to them. Keep this check until all filesystems > have migrated to the new mount API. > > https://virtuozzo.atlassian.net/browse/VSTOR-132330 > Fixes: 263467c864c5 ("ve/fs/devmnt: process mount options") > Signed-off-by: Vasileios Almpanis > Co-developed-by: Pavel Tikhomirov > Signed-off-by: Pavel Tikhomirov > > Feature: ve: ve generic structures > --- > > Changes since v7: > - Add ve_devmnt_deny_legacy called by ve_devmnt_verify_fc only for > mounts that required a block device and for non super VEs. With a > comment explaining the rationale behind it, and for it to be removed > in the future when all filesystem types have migrated to the new mount > API namely when our kernels base becomes newer than v6.15. This will > deny any mount in containers that are trying to use legacy filesystem > types like ext2 even if some developer adds the FS_VIRTUALISED flag > and recomplies the kernel with a warning. > > Changes since v6: > - Enforce sb-flag changes on remount: emit "rw" when SB_RDONLY is cleared > and vet the effective flags, > (sb->s_flags & ~fc->sb_flags_mask) | (fc->sb_flags & fc->sb_flags_mask), > so a legacy mount(2) remount can no longer flip RO->RW unchecked. > - Constrain only devices listed in ve.mount_opts: an unlisted device > mounted with no userspace options is allowed rather than refused by the > synthesized ro/rw token (tracked via have_user_opts). > - Reset fc->ve_final_opts in vfs_dup_fs_context() to avoid a double-free. > - Factor per-option emit into __vfs_emit_flag(); checkpatch/style fixes. > > Changes since v5: > - Fix use-after-free: do_new_mount() freed the merged page that > legacy_get_tree()->mount() dereferences later; the fs now sees only > the caller-owned original data. > - Fix remount,dirsync failing with -EINVAL in containers: stop re-parsing > the formatted flag names (SB_DIRSYNC is not in MS_RMT_MASK). > - Check policy only: legacy fs parsers no longer get synthesized ro/sync > tokens, and the flag check reads fc->sb_flags symmetrically for mount > and fsconfig. > - Keep ve_check_mount_options() only on the remount path (pinned sb > device, covers legacy ->reconfigure); drop legacy_merge_mount_data() > and ve_prepare_mount_options(); add Fixes: tag and rewrite the message. > > Changes since v4: > - Emit only the positive sb-flag names, not the clear names (rw/async/ > ...); on legacy remount sb_flags_mask is MS_RMT_MASK, so the clear > names had wrongly rejected ordinary in-container remounts. > - NUL-terminate the options page when data is empty and no flags emitted. > - Fix __vfs_format_flags() comment (-E2BIG, not -ENOSPC). > > Changes since v3: > - Drop excess length check in legacy_merge_mount_data(). > > Changes since v2: > - Remove the legacy_merge_mount_data guard in fs/internal.h. > - Add __vfs_format_flags() helper, used by vfs_format_sb_flags(). > - Use -E2BIG (not -ENOSPC) for the buffer-too-small case. > > Changes since v1: > - Unify the comma-insert-copy pattern across call sites via an > append_entry() helper. > - Rework legacy_merge_mount_data() to allocate the page upfront and > append sb flags via vfs_format_sb_flags(), dropping flags_buf[128] > and the size arithmetic. > - Use -ENOSPC (not -EINVAL) for buffer-too-small; comment > FS_BINARY_MOUNTDATA; minor blank-line cleanups. > > fs/fs_context.c | 193 +++++++++++++++++++++++++++++++++++++ > fs/internal.h | 8 ++ > fs/namespace.c | 110 ++++++++++++++++----- > fs/super.c | 12 +++ > include/linux/fs_context.h | 2 + > include/linux/mount.h | 2 + > 6 files changed, 304 insertions(+), 23 deletions(-) > > diff --git a/fs/fs_context.c b/fs/fs_context.c > index 76f34f3d468e..b2bd21a42083 100644 > --- a/fs/fs_context.c > +++ b/fs/fs_context.c > @@ -81,6 +81,70 @@ static int vfs_parse_sb_flag(struct fs_context *fc, const char *key) > return -ENOPARAM; > } > > +/* > + * Emit option @name into @buff at *@off, prefixed with ',' if the buffer > + * already holds text. Advances *@off. Returns 0 or -E2BIG if @buff is full. > + */ > +static int __vfs_emit_flag(const char *name, char *buff, > + size_t size, size_t *off) > +{ > + ssize_t ret; > + > + if (*off) { > + if (*off + 1 >= size) > + return -E2BIG; > + buff[(*off)++] = ','; > + } > + > + ret = strscpy(buff + *off, name, size - *off); > + if (ret < 0) > + return -E2BIG; > + *off += ret; > + return 0; > +} > + > +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++) { > + int ret; > + > + if (!(flags & p->value)) > + continue; > + ret = __vfs_emit_flag(p->name, buff, size, off); > + if (ret) > + return ret; > + } > + return 0; > +} > + > +static int vfs_format_sb_flags(char *buff, size_t size, size_t *off, > + unsigned int sb_flags) > +{ > + int err; > + > + err = __vfs_format_flags(common_set_sb_flag, sb_flags, buff, size, off); > + if (err) > + return err; > + > + /* > + * "rw" has no flag bit of its own - it is simply the absence of > + * SB_RDONLY. Emit it explicitly so the ve_devmnt policy can allow or > + * deny read-write access as a first-class option; otherwise a mount or > + * remount that leaves the superblock read-write carries no token and > + * slips past the "every option must be allowed" check. > + * > + * @sb_flags is the effective post-operation flag word, so this reflects > + * the state the superblock actually ends up in. A remount that only > + * touches an unrelated flag (e.g. "sync") keeps its current SB_RDONLY > + * and so does not emit "rw". > + */ > + if (!(sb_flags & SB_RDONLY)) > + return __vfs_emit_flag("rw", buff, size, off); > + > + return 0; > +} > + > /** > * vfs_parse_fs_param_source - Handle setting "source" via parameter > * @fc: The filesystem context to modify > @@ -224,6 +288,119 @@ static inline int fscontext_lookup_bdev(struct fs_context *fc, dev_t *s_dev) > return -ENODEV; > } > > +/* > + * ve_devmnt_deny_legacy - refuse a device mount that escapes the policy > + * @fc: the mount context > + * > + * A legacy context, one for a filesystem with no ->init_fs_context passes > + * its mount data straight to ->mount()/->remount_fs() through > + * legacy_parse_monolithic(), so vfs_parse_monolithic_sep() never runs on it. > + * The container's forced options are then never inserted and fc->ve_final_opts > + * stays empty, which would leave ve_devmnt_verify_fc() vetting an empty option > + * string while the filesystem acts on the full one userspace passed. > + * > + * Returns 0 if the mount may go ahead, -EPERM if it must not. > + */ > +static int ve_devmnt_deny_legacy(struct fs_context *fc) > +{ > + struct ve_struct *ve = get_exec_env(); > + > + if (fc->ops != &legacy_fs_context_ops) > + return 0; > + > + ve_pr_warn_ratelimited(VE_LOG_BOTH, > + "VE%s: refusing to mount %s: filesystem has no fs_context support\n", > + ve_name(ve), fc->fs_type->name); > + return -EPERM; > +} > + > +/* > + * ve_devmnt_verify_fc - check a mount against the container device-mount policy > + * @fc: the mount context, with fc->root set > + * @new_mount: true at vfs_get_tree() (new mount), false at reconfigure_super() > + * > + * Vets the stashed userspace option string plus the synthesized SB_* flag > + * names against the mounted superblock's device. A device absent from the > + * policy that is mounted with no userspace options is allowed. Returns 0 when > + * permitted (or no check applies), or a negative errno. > + */ > +int ve_devmnt_verify_fc(struct fs_context *fc, bool new_mount) > +{ > + struct ve_struct *ve = get_exec_env(); > + unsigned int sb_flags; > + bool have_user_opts; > + size_t off = 0; > + char *page; > + int err; > + > + if (ve_is_super(ve)) > + return 0; > + > + if (!fc->fs_type || !(fc->fs_type->fs_flags & FS_REQUIRES_DEV)) > + return 0; > + > + /* > + * Keep this check until all filesystems have migrated to the new > + * mount API > + */ > + err = ve_devmnt_deny_legacy(fc); > + if (err) > + return err; > + > + /* > + * 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 0; > + > + if (WARN_ON_ONCE(!fc->root)) > + return -EINVAL; > + > + page = (char *)__get_free_page(GFP_KERNEL); > + if (!page) > + return -ENOMEM; > + > + /* > + * Track whether userspace actually supplied options. @page below also > + * gets the synthesized ro/rw flag token, so its length cannot answer > + * this; ve_final_opts holds only the userspace string. > + */ > + have_user_opts = fc->ve_final_opts && *fc->ve_final_opts; > + if (have_user_opts) { > + ssize_t ret = strscpy(page, fc->ve_final_opts, PAGE_SIZE); > + > + if (ret < 0) { > + err = -E2BIG; > + goto out; > + } > + off = ret; > + } > + > + /* > + * On a remount fc->sb_flags holds only the bits being changed, so > + * combine them with the current superblock flags to get the state the > + * sb will actually have - the same value reconfigure_super() writes > + * back. On a new mount fc->sb_flags is already the full flag word. > + */ > + sb_flags = fc->sb_flags; > + if (!new_mount) > + sb_flags = (fc->root->d_sb->s_flags & ~fc->sb_flags_mask) | > + (fc->sb_flags & fc->sb_flags_mask); > + > + err = vfs_format_sb_flags(page, PAGE_SIZE, &off, sb_flags); > + if (err) > + goto out; > + > + page[off] = '\0'; > + err = ve_devmnt_verify(ve, fc->root->d_sb->s_dev, page, new_mount, > + have_user_opts); > + > +out: > + free_page((unsigned long)page); > + return err; > +} > + > static int fscontext_init_lazy_opts(struct fs_context *fc) > { > struct ve_struct *ve = get_exec_env(); > @@ -389,10 +566,22 @@ int vfs_parse_monolithic_sep(struct fs_context *fc, void *data, > return -ENODEV; > } > > + /* Early reject and hidden-option insertion; verified for real later. */ > ret = ve_devmnt_process(ve, bd_dev, (void **) &options, > fc->purpose == FS_CONTEXT_FOR_RECONFIGURE); > if (ret) > return ret; > + > + /* Stash what the filesystem parses; checked in the common mount path. */ > + if (options) { > + kfree(fc->ve_final_opts); > + fc->ve_final_opts = kstrdup(options, GFP_KERNEL); > + if (!fc->ve_final_opts) { > + if (options != options_orig) > + free_page((unsigned long)options); > + return -ENOMEM; > + } > + } > } > > /* > @@ -614,6 +803,7 @@ struct fs_context *vfs_dup_fs_context(struct fs_context *src_fc) > fc->s_fs_info = NULL; > fc->source = NULL; > fc->security = NULL; > + fc->ve_final_opts = NULL; > get_filesystem(fc->fs_type); > get_net(fc->net_ns); > get_user_ns(fc->user_ns); > @@ -742,6 +932,7 @@ void put_fs_context(struct fs_context *fc) > put_filesystem(fc->fs_type); > if (fc->lazy_opts) > free_page((unsigned long)fc->lazy_opts); > + kfree(fc->ve_final_opts); > kfree(fc->source); > kfree(fc); > } > @@ -962,6 +1153,8 @@ void vfs_clean_context(struct fs_context *fc) > free_page((unsigned long)fc->lazy_opts); > fc->lazy_opts = NULL; > } > + kfree(fc->ve_final_opts); > + fc->ve_final_opts = NULL; > kfree(fc->source); > fc->source = NULL; > fc->exclusive = false; > diff --git a/fs/internal.h b/fs/internal.h > index 3647ce69b2c7..f1892959db48 100644 > --- a/fs/internal.h > +++ b/fs/internal.h > @@ -46,6 +46,14 @@ 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 *); > +#ifdef CONFIG_VE > +extern int ve_devmnt_verify_fc(struct fs_context *fc, bool new_mount); > +#else > +static inline int ve_devmnt_verify_fc(struct fs_context *fc, bool new_mount) > +{ > + return 0; > +} > +#endif > 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 43493f779c59..4d4dc5290350 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -3258,6 +3258,92 @@ int ve_devmnt_process(struct ve_struct *ve, dev_t dev, void **data_pp, int remou > return err; > } > > +/* Return 0 if every option in @options is listed in @a or @b, else -EPERM. */ > +static int ve_devmnt_options_subset(char *options, char *a, char *b) > +{ > + char *copy, *cur, *p; > + int err = 0; > + > + if (!options || !*options) > + return 0; > + if (!a && !b) > + return -EPERM; > + > + copy = cur = kstrdup(options, GFP_KERNEL); > + if (!copy) > + return -ENOMEM; > + > + while ((p = strsep(&cur, ",")) != NULL) { > + if (!*p) > + continue; > + if ((!a || !strstr_separated(a, p, ',')) && > + (!b || !strstr_separated(b, p, ','))) { > + err = -EPERM; > + break; > + } > + } > + > + kfree(copy); > + return err; > +} > + > +/* > + * ve_devmnt_verify - enforce the container device-mount policy for @dev > + * @ve: the container > + * @dev: device taken from the mounted superblock (not from a raceable path) > + * @opts: mount options plus the SB_* flag names to vet > + * @new_mount: true for a new mount, false for a remount > + * @have_user_opts: true if userspace supplied any mount options. @opts always > + * carries the kernel-synthesized ro/rw flag token, so it is never empty > + * and cannot answer this on its own. > + * > + * Every supplied option must be allowed or forced. On a new mount the forced > + * ("hidden") options must also be present: a filesystem that skipped inserting > + * them is refused rather than silently dropping a container's mandated option > + */ > +int ve_devmnt_verify(struct ve_struct *ve, dev_t dev, char *opts, bool new_mount, > + bool have_user_opts) > +{ > + struct ve_devmnt *devmnt; > + char *allowed = NULL, *hidden = NULL; > + bool found = false; > + int err = 0; > + > + if (ve->is_pseudosuper) > + return 0; > + > + mutex_lock(&ve->devmnt_mutex); > + list_for_each_entry(devmnt, &ve->devmnt_list, link) { > + if (devmnt->dev == dev) { > + allowed = devmnt->allowed_options; > + hidden = devmnt->hidden_options; > + found = true; > + break; > + } > + } > + > + /* > + * Enforce for a listed device, or for any mount carrying userspace > + * options. An unlisted device with no userspace options is unconstrained > + * here, so the synthesized ro/rw token in @opts does not deny it. > + */ > + if (found || have_user_opts) { > + /* every supplied option must be either allowed or forced */ > + err = ve_devmnt_options_subset(opts, allowed, hidden); > + > + /* on a new mount every forced option must have reached the fs */ > + if (!err && new_mount) > + err = ve_devmnt_options_subset(hidden, opts, NULL); > + } > + mutex_unlock(&ve->devmnt_mutex); > + > + if (err == -EPERM) > + ve_pr_warn_ratelimited(VE_LOG_BOTH, > + "VE%s: mount options not permitted for device %u:%u\n", > + ve_name(ve), MAJOR(dev), MINOR(dev)); > + return err; > +} > + > static inline int ve_mount_allowed(void) > { > struct ve_struct *ve = get_exec_env(); > @@ -3308,23 +3394,6 @@ static inline void ve_mount_nr_inc(struct mount *mnt, struct ve_struct *ve) { } > static inline void ve_mount_nr_dec(struct mount *mnt) { } > #endif /* CONFIG_VE */ > > -static int ve_prepare_mount_options(struct fs_context *fc, void *data) > -{ > -#ifdef CONFIG_VE > - struct super_block *sb = fc->root->d_sb; > - struct ve_struct *ve = get_exec_env(); > - > - if (sb->s_bdev && data && !ve_is_super(ve)) { > - int err; > - > - err = ve_devmnt_process(ve, sb->s_bdev->bd_dev, &data, 1); > - if (err) > - return err; > - } > -#endif > - return 0; > -} > - > /* > * change filesystem flags. dir should be a physical root of filesystem. > * If you've mounted a non-root directory somewhere and want to do remount > @@ -3357,12 +3426,6 @@ 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; > - } > - > err = parse_monolithic_mount_data(fc, data); > if (!err) { > down_write(&sb->s_umount); > @@ -3816,6 +3879,7 @@ 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)); > + /* Container device-mount policy is enforced later, in vfs_get_tree(). */ > if (!err) > err = parse_monolithic_mount_data(fc, data); > if (!err && !mount_capable(fc)) > diff --git a/fs/super.c b/fs/super.c > index 1adebbf35803..c0c067eb2d8e 100644 > --- a/fs/super.c > +++ b/fs/super.c > @@ -1085,6 +1085,11 @@ int reconfigure_super(struct fs_context *fc) > if (retval) > return retval; > > + /* Enforce the container device-mount policy on the remount options. */ > + retval = ve_devmnt_verify_fc(fc, false); > + if (retval) > + return retval; > + > if (fc->sb_flags_mask & SB_RDONLY) { > #ifdef CONFIG_BLOCK > if (!(fc->sb_flags & SB_RDONLY) && sb->s_bdev && > @@ -1924,6 +1929,13 @@ int vfs_get_tree(struct fs_context *fc) > return error; > } > > + /* Enforce the container device-mount policy against the real device. */ > + error = ve_devmnt_verify_fc(fc, true); > + if (unlikely(error)) { > + fc_drop_locked(fc); > + return error; > + } > + > /* > * filesystems should never set s_maxbytes larger than MAX_LFS_FILESIZE > * but s_maxbytes was an unsigned long long for many releases. Throw > diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h > index 1801aed1da67..2ca586e2cc2a 100644 > --- a/include/linux/fs_context.h > +++ b/include/linux/fs_context.h > @@ -93,6 +93,8 @@ struct fs_context { > struct file_system_type *fs_type; > void *fs_private; /* The filesystem's context */ > void *lazy_opts; /* mount options which can't be checked at fsconfig() time */ > + /* option string handed to the fs, for the ve_devmnt policy check */ > + char *ve_final_opts; > void *sget_key; > struct dentry *root; /* The root and superblock */ > struct user_namespace *user_ns; /* The user namespace for this mount */ > diff --git a/include/linux/mount.h b/include/linux/mount.h > index 0cbc6f6893c0..ab0ea4f7afc6 100644 > --- a/include/linux/mount.h > +++ b/include/linux/mount.h > @@ -127,5 +127,7 @@ extern int cifs_root_data(char **dev, char **opts); > > struct ve_struct; > extern int ve_devmnt_process(struct ve_struct *, dev_t, void **, int); > +extern int ve_devmnt_verify(struct ve_struct *ve, dev_t dev, char *opts, > + bool new_mount, bool have_user_opts); > > #endif /* _LINUX_MOUNT_H */ -- Best regards, Pavel Tikhomirov Senior Software Developer, Virtuozzo.