On 7/20/26 12:14 AM, Vladimir Riabchun wrote:
On 17.07.2026 14:17, Pavel Tikhomirov wrote:From: Vasileios Almpanis <[email protected]> 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 mandatedoptions 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.I checked and something seems uncovered: do_reconfigure_mnt via MS_REMOUNT|MS_BIND flags doesn't hit vfs_get_tree nor reconfigure_super, and yet may update flags.Am I missing something, maybe we forbid bind mounts somewhere?
Look like you are right. Verified that we can indeed create bind mounts, in CTs. We can also bind mount them to another directory and then reconfigure those bind mounts with other options (not stated in ve.mount_opts) and it will succeed. We could guard bind reconfigures with capable checks otherwise we need get block device and go through the same checks. Before I send the next patch I'm open to suggestions
Agree with this, ve_devmnt_verify_fc in my opinion should bail in case fc->ve_final_opts == NULL,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. https://virtuozzo.atlassian.net/browse/VSTOR-132330 Fixes: 263467c864c5 ("ve/fs/devmnt: process mount options") Signed-off-by: Vasileios Almpanis <[email protected]> Co-developed-by: Pavel Tikhomirov <[email protected]> Signed-off-by: Pavel Tikhomirov <[email protected]> Feature: ve: ve generic structures --- fs/fs_context.c | 108 +++++++++++++++++++++++++++++++++++++ fs/internal.h | 1 + fs/namespace.c | 97 +++++++++++++++++++++++++-------- fs/super.c | 12 +++++ include/linux/fs_context.h | 1 + include/linux/mount.h | 1 + 6 files changed, 197 insertions(+), 23 deletions(-) diff --git a/fs/fs_context.c b/fs/fs_context.c index 76f34f3d468ea..18ffe23ad4847 100644 --- a/fs/fs_context.c +++ b/fs/fs_context.c@@ -81,6 +81,41 @@ 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); +} + /** * vfs_parse_fs_param_source - Handle setting "source" via parameter * @fc: The filesystem context to modify@@ -224,6 +259,64 @@ static inline int fscontext_lookup_bdev(struct fs_context *fc, dev_t *s_dev)return -ENODEV; } +/*+ * 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 option string plus the MS_* superblock flag names against + * the mounted superblock's device. 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(); + 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; + + /* + * 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; + + if (fc->ve_final_opts && *fc->ve_final_opts) { + ssize_t ret = strscpy(page, fc->ve_final_opts, PAGE_SIZE); + + if (ret < 0) { + err = -E2BIG; + goto out; + } + off = ret; + } + + err = vfs_format_sb_flags(fc, page, PAGE_SIZE, &off); + if (err) + goto out; + + page[off] = '\0'; + err = ve_devmnt_verify(ve, fc->root->d_sb->s_dev, page, new_mount); + +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 +482,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);Some file systems override parse_monolithic, for example smb3_fs_context_parse_monolithic.They won't have ve_final_opts set, so checks may fail.
what do you think?
+ fc->ve_final_opts = kstrdup(options, GFP_KERNEL); + if (!fc->ve_final_opts) { + if (options != options_orig) + free_page((unsigned long)options); + return -ENOMEM; + } + } } /* @@ -742,6 +847,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 +1068,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 3647ce69b2c7a..e33d3ae70ccee 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 int ve_devmnt_verify_fc(struct fs_context *fc, bool new_mount); 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 0f4a3668e558d..3a457588e96d9 100644 --- a/fs/namespace.c +++ b/fs/namespace.c@@ -3258,6 +3258,79 @@ int ve_devmnt_process(struct ve_struct *ve, dev_t dev, void **data_pp, int remoureturn 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)All these arguments should be const.
Not all, just cur and p.
This is correct and consistent with what happens in ve_devmnt_process. Look at commit e7176b8050be ("fs: namespace: allow mounting blockdevices without extra options") For mount tries were device is not in device list (allowed and hidden doesn't exist) the check passes+{ + 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 MS_* flag names to vet + * @new_mount: true for a new mount, false for a remount + *+ * 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)+{ + struct ve_devmnt *devmnt; + char *allowed = NULL, *hidden = NULL; + int err; + + 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; + break; + } + }What if the device is not in devmnt_list and user passed no options? opts is empty and allowed and hidden are NULLs, so all the following checks pass.
+ + /* 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 filesystem */+ 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));Is there a way to know that we lack of hidden mount options? With this approach, they are not hidden anymore, they are equal to they are required now. And with left ve_devmnt_process "user should provide these flags" is not working properly - in do_new_mount parse_monolithic_mount_data goes before vfs_get_tree, so ve_devmnt_process will add hidden_options instead of user. Maybe we should rename it to required_options or keep them as actually hidden?
Hidden options are actually hidden. They are added by the container creator and are inserted to the user-passed options in ve_devmnt_insert. This mean that if hidden options is "nodev,nosuid" and container tries to mount this block device with "rw,relatime" then the new mount will be
"rw,relatime,nodev,nosuid" so the name is consistent with the purpose.
+ return err; +} + static inline int ve_mount_allowed(void) { struct ve_struct *ve = get_exec_env();@@ -3308,23 +3381,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 +3413,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 +3866,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 1adebbf358032..c0c067eb2d8e1 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. Throwdiff --git a/include/linux/fs_context.h b/include/linux/fs_context.h index 1801aed1da67c..3dabb5ba11511 100644 --- a/include/linux/fs_context.h +++ b/include/linux/fs_context.h @@ -93,6 +93,7 @@ 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 */ + char *ve_final_opts; /* option string handed to the fs, for the ve_devmnt policy check */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 0cbc6f6893c01..12898c96cc768 100644 --- a/include/linux/mount.h +++ b/include/linux/mount.h @@ -127,5 +127,6 @@ 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 *, dev_t, char *, bool); #endif /* _LINUX_MOUNT_H */-- Best regards, Riabchun Vladimir Linux Kernel Developer, Virtuozzo
-- Best regards, Vasileios Almpanis Software Developer, Virtuozzo. _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
