| Message ID | a6a954a1f1c7d612104279c62916f49e47ba5811.1697085884.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: reject unknown mount options correctly | expand |
On Thu 12 Oct 2023 at 15:14, Qu Wenruo <wqu@suse.com> wrote: > [BUG] > The following script would allow invalid mount options to be > specified > (although such invalid options would just be ignored): > > # mkfs.btrfs -f $dev > # mount $dev $mnt1 <<< Successful mount expected > # mount $dev $mnt2 -o junk <<< Failed mount expected > # echo $? > 0 > > [CAUSE] > During the mount progress, btrfs_mount_root() would go different > paths > depending on if there is already a mounted btrfs for it: > > s = sget(); > if (s->s_root) { > /* do the cleanups and reuse the existing super */ > } else { > /* do the real mount */ > error = btrfs_fill_super(); > } > > Inside btrfs_fill_super() we call open_ctree() and then > btrfs_parse_options(), which would reject all the invalid > options. > > But if we got the other path, we won't really call > btrfs_parse_options(), thus we just ignore the mount options > completely. > > [FIX] > Instead of pure cleanups, if we found an existing mounted btrfs, > we > still do a very basic mount options check, to reject unknown > mount > options. > > Inside btrfs_mount_root(), we have already called > security_sb_eat_lsm_opts(), which would have already stripe the > security > mount options, thus if we hit an error, it must be an invalid > one. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > This would be the proper fix for the recently reverted commit > 5f521494cc73 ("btrfs: reject unknown mount options early"). > I'm a noob about selinux though. Better to draft a new fstest case to avoid further regression? -- Su > With updated timing where the new check is after > security_sb_eat_lsm_opts(). > --- > fs/btrfs/super.c | 46 > ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index cc326969751f..4e4a2e4ba315 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -860,6 +860,50 @@ static int btrfs_parse_device_options(const > char *options, blk_mode_t flags) > return error; > } > > +/* > + * Check if the @options has any invalid ones. > + * > + * NOTE: this can only be called after > security_sb_eat_lsm_opts(). > + * > + * Return -ENOMEM if we failed to allocate the memory for the > string > + * Return -EINVAL if we found invalid mount options > + * Return 0 otherwise. > + */ > +static int btrfs_check_invalid_options(const char *options) > +{ > + substring_t args[MAX_OPT_ARGS]; > + char *opts, *orig, *p; > + int ret = 0; > + > + if (!options) > + return 0; > + > + opts = kstrdup(options, GFP_KERNEL); > + if (!opts) > + return -ENOMEM; > + orig = opts; > + > + while ((p = strsep(&opts, ",")) != NULL) { > + int token; > + > + if (!*p) > + continue; > + > + token = match_token(p, tokens, args); > + switch (token) { > + case Opt_err: > + btrfs_err(NULL, "unrecognized mount option '%s'", p); > + ret = -EINVAL; > + goto out; > + default: > + break; > + } > + } > +out: > + kfree(orig); > + return ret; > +} > + > /* > * Parse mount options that are related to subvolume id > * > @@ -1474,6 +1518,8 @@ static struct dentry > *btrfs_mount_root(struct file_system_type *fs_type, > btrfs_free_fs_info(fs_info); > if ((flags ^ s->s_flags) & SB_RDONLY) > error = -EBUSY; > + if (!error) > + error = btrfs_check_invalid_options(data); > } else { > snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev); > shrinker_debugfs_rename(&s->s_shrink, "sb-%s:%s", > fs_type->name,
On 2023/10/12 16:31, Su Yue wrote: > > On Thu 12 Oct 2023 at 15:14, Qu Wenruo <wqu@suse.com> wrote: > >> [BUG] >> The following script would allow invalid mount options to be specified >> (although such invalid options would just be ignored): >> >> # mkfs.btrfs -f $dev >> # mount $dev $mnt1 <<< Successful mount expected >> # mount $dev $mnt2 -o junk <<< Failed mount expected >> # echo $? >> 0 >> >> [CAUSE] >> During the mount progress, btrfs_mount_root() would go different paths >> depending on if there is already a mounted btrfs for it: >> >> s = sget(); >> if (s->s_root) { >> /* do the cleanups and reuse the existing super */ >> } else { >> /* do the real mount */ >> error = btrfs_fill_super(); >> } >> >> Inside btrfs_fill_super() we call open_ctree() and then >> btrfs_parse_options(), which would reject all the invalid options. >> >> But if we got the other path, we won't really call >> btrfs_parse_options(), thus we just ignore the mount options completely. >> >> [FIX] >> Instead of pure cleanups, if we found an existing mounted btrfs, we >> still do a very basic mount options check, to reject unknown mount >> options. >> >> Inside btrfs_mount_root(), we have already called >> security_sb_eat_lsm_opts(), which would have already stripe the security >> mount options, thus if we hit an error, it must be an invalid one. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> This would be the proper fix for the recently reverted commit >> 5f521494cc73 ("btrfs: reject unknown mount options early"). >> > I'm a noob about selinux though. Better to draft a new fstest case to > avoid further regression? For SELinux enabled environment, any test would fail, thus I wonder if we really need a dedicated one. But as an Arch fanboy, my VM completely failed to catch it, nor even has the needed user space tools by default... For the invalid options rejection, we could definitely have one test case for it. Thanks, Qu > > -- > Su > >> With updated timing where the new check is after >> security_sb_eat_lsm_opts(). >> --- >> fs/btrfs/super.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 46 insertions(+) >> >> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c >> index cc326969751f..4e4a2e4ba315 100644 >> --- a/fs/btrfs/super.c >> +++ b/fs/btrfs/super.c >> @@ -860,6 +860,50 @@ static int btrfs_parse_device_options(const char >> *options, blk_mode_t flags) >> return error; >> } >> >> +/* >> + * Check if the @options has any invalid ones. >> + * >> + * NOTE: this can only be called after security_sb_eat_lsm_opts(). >> + * >> + * Return -ENOMEM if we failed to allocate the memory for the string >> + * Return -EINVAL if we found invalid mount options >> + * Return 0 otherwise. >> + */ >> +static int btrfs_check_invalid_options(const char *options) >> +{ >> + substring_t args[MAX_OPT_ARGS]; >> + char *opts, *orig, *p; >> + int ret = 0; >> + >> + if (!options) >> + return 0; >> + >> + opts = kstrdup(options, GFP_KERNEL); >> + if (!opts) >> + return -ENOMEM; >> + orig = opts; >> + >> + while ((p = strsep(&opts, ",")) != NULL) { >> + int token; >> + >> + if (!*p) >> + continue; >> + >> + token = match_token(p, tokens, args); >> + switch (token) { >> + case Opt_err: >> + btrfs_err(NULL, "unrecognized mount option '%s'", p); >> + ret = -EINVAL; >> + goto out; >> + default: >> + break; >> + } >> + } >> +out: >> + kfree(orig); >> + return ret; >> +} >> + >> /* >> * Parse mount options that are related to subvolume id >> * >> @@ -1474,6 +1518,8 @@ static struct dentry *btrfs_mount_root(struct >> file_system_type *fs_type, >> btrfs_free_fs_info(fs_info); >> if ((flags ^ s->s_flags) & SB_RDONLY) >> error = -EBUSY; >> + if (!error) >> + error = btrfs_check_invalid_options(data); >> } else { >> snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev); >> shrinker_debugfs_rename(&s->s_shrink, "sb-%s:%s", >> fs_type->name,
On Thu, Oct 12, 2023 at 05:37:32PM +1030, Qu Wenruo wrote: > On 2023/10/12 16:31, Su Yue wrote: > > > > On Thu 12 Oct 2023 at 15:14, Qu Wenruo <wqu@suse.com> wrote: > > > >> [BUG] > >> The following script would allow invalid mount options to be specified > >> (although such invalid options would just be ignored): > >> > >> # mkfs.btrfs -f $dev > >> # mount $dev $mnt1 <<< Successful mount expected > >> # mount $dev $mnt2 -o junk <<< Failed mount expected > >> # echo $? > >> 0 > >> > >> [CAUSE] > >> During the mount progress, btrfs_mount_root() would go different paths > >> depending on if there is already a mounted btrfs for it: > >> > >> s = sget(); > >> if (s->s_root) { > >> /* do the cleanups and reuse the existing super */ > >> } else { > >> /* do the real mount */ > >> error = btrfs_fill_super(); > >> } > >> > >> Inside btrfs_fill_super() we call open_ctree() and then > >> btrfs_parse_options(), which would reject all the invalid options. > >> > >> But if we got the other path, we won't really call > >> btrfs_parse_options(), thus we just ignore the mount options completely. > >> > >> [FIX] > >> Instead of pure cleanups, if we found an existing mounted btrfs, we > >> still do a very basic mount options check, to reject unknown mount > >> options. > >> > >> Inside btrfs_mount_root(), we have already called > >> security_sb_eat_lsm_opts(), which would have already stripe the security > >> mount options, thus if we hit an error, it must be an invalid one. > >> > >> Signed-off-by: Qu Wenruo <wqu@suse.com> > >> --- > >> This would be the proper fix for the recently reverted commit > >> 5f521494cc73 ("btrfs: reject unknown mount options early"). > >> > > I'm a noob about selinux though. Better to draft a new fstest case to > > avoid further regression? > > For SELinux enabled environment, any test would fail, thus I wonder if > we really need a dedicated one. > > But as an Arch fanboy, my VM completely failed to catch it, nor even has > the needed user space tools by default... > > For the invalid options rejection, we could definitely have one test > case for it. We'll need coverage tests for mount option parsing before we do the conversion to fs_context. The fstests do a lot of mounts so it is somehow tested but we'll need to test the parser regardless of the MKFS_OPTIONS, e.g. try to mount with the selnux options, combined with or without a subvolume, remounts etc.
On Thu, Oct 12, 2023 at 04:55:46PM +0200, David Sterba wrote: > On Thu, Oct 12, 2023 at 05:37:32PM +1030, Qu Wenruo wrote: > > On 2023/10/12 16:31, Su Yue wrote: > > >> > > >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > >> --- > > >> This would be the proper fix for the recently reverted commit > > >> 5f521494cc73 ("btrfs: reject unknown mount options early"). > > >> > > > I'm a noob about selinux though. Better to draft a new fstest case to > > > avoid further regression? > > > > For SELinux enabled environment, any test would fail, thus I wonder if > > we really need a dedicated one. > > > > But as an Arch fanboy, my VM completely failed to catch it, nor even has > > the needed user space tools by default... > > > > For the invalid options rejection, we could definitely have one test > > case for it. > > We'll need coverage tests for mount option parsing before we do the > conversion to fs_context. The conversion patches have been written and in testing, I think we don't need to fix the old parsing code. The problem is there but I don't see it as urgent, it's been there for a long time.
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index cc326969751f..4e4a2e4ba315 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -860,6 +860,50 @@ static int btrfs_parse_device_options(const char *options, blk_mode_t flags) return error; } +/* + * Check if the @options has any invalid ones. + * + * NOTE: this can only be called after security_sb_eat_lsm_opts(). + * + * Return -ENOMEM if we failed to allocate the memory for the string + * Return -EINVAL if we found invalid mount options + * Return 0 otherwise. + */ +static int btrfs_check_invalid_options(const char *options) +{ + substring_t args[MAX_OPT_ARGS]; + char *opts, *orig, *p; + int ret = 0; + + if (!options) + return 0; + + opts = kstrdup(options, GFP_KERNEL); + if (!opts) + return -ENOMEM; + orig = opts; + + while ((p = strsep(&opts, ",")) != NULL) { + int token; + + if (!*p) + continue; + + token = match_token(p, tokens, args); + switch (token) { + case Opt_err: + btrfs_err(NULL, "unrecognized mount option '%s'", p); + ret = -EINVAL; + goto out; + default: + break; + } + } +out: + kfree(orig); + return ret; +} + /* * Parse mount options that are related to subvolume id * @@ -1474,6 +1518,8 @@ static struct dentry *btrfs_mount_root(struct file_system_type *fs_type, btrfs_free_fs_info(fs_info); if ((flags ^ s->s_flags) & SB_RDONLY) error = -EBUSY; + if (!error) + error = btrfs_check_invalid_options(data); } else { snprintf(s->s_id, sizeof(s->s_id), "%pg", bdev); shrinker_debugfs_rename(&s->s_shrink, "sb-%s:%s", fs_type->name,
[BUG] The following script would allow invalid mount options to be specified (although such invalid options would just be ignored): # mkfs.btrfs -f $dev # mount $dev $mnt1 <<< Successful mount expected # mount $dev $mnt2 -o junk <<< Failed mount expected # echo $? 0 [CAUSE] During the mount progress, btrfs_mount_root() would go different paths depending on if there is already a mounted btrfs for it: s = sget(); if (s->s_root) { /* do the cleanups and reuse the existing super */ } else { /* do the real mount */ error = btrfs_fill_super(); } Inside btrfs_fill_super() we call open_ctree() and then btrfs_parse_options(), which would reject all the invalid options. But if we got the other path, we won't really call btrfs_parse_options(), thus we just ignore the mount options completely. [FIX] Instead of pure cleanups, if we found an existing mounted btrfs, we still do a very basic mount options check, to reject unknown mount options. Inside btrfs_mount_root(), we have already called security_sb_eat_lsm_opts(), which would have already stripe the security mount options, thus if we hit an error, it must be an invalid one. Signed-off-by: Qu Wenruo <wqu@suse.com> --- This would be the proper fix for the recently reverted commit 5f521494cc73 ("btrfs: reject unknown mount options early"). With updated timing where the new check is after security_sb_eat_lsm_opts(). --- fs/btrfs/super.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)