| Message ID | 20220120214948.3637895-2-smayhew@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: parse sids earlier to avoid doing memory allocations under spinlock | expand |
On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <smayhew@redhat.com> wrote: > > selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and > shouldn't be performing any memory allocations. Fix this by parsing the > sids at the same time we're chopping up the security mount options > string and then using the pre-parsed sids when doing the comparison. > > Fixes: cc274ae7763d ("selinux: fix sleeping function called from invalid context") > Fixes: 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an existing mount") > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > --- > security/selinux/hooks.c | 112 ++++++++++++++++++++++++++------------- > 1 file changed, 76 insertions(+), 36 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 5b6895e4fc29..f27ca9e870c0 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -342,6 +342,11 @@ static void inode_free_security(struct inode *inode) > > struct selinux_mnt_opts { > const char *fscontext, *context, *rootcontext, *defcontext; > + u32 fscontext_sid; > + u32 context_sid; > + u32 rootcontext_sid; > + u32 defcontext_sid; > + unsigned short preparsed; > }; Is the preparsed field strictly necessary? Can't we just write the code to assume that if a given SID field is not SECSID_NULL then it is valid/preparsed? > @@ -598,12 +603,11 @@ static int bad_option(struct superblock_security_struct *sbsec, char flag, > return 0; > } > > -static int parse_sid(struct super_block *sb, const char *s, u32 *sid, > - gfp_t gfp) > +static int parse_sid(struct super_block *sb, const char *s, u32 *sid) > { > int rc = security_context_str_to_sid(&selinux_state, s, > - sid, gfp); > - if (rc) > + sid, GFP_KERNEL); > + if (rc && sb != NULL) > pr_warn("SELinux: security_context_str_to_sid" > "(%s) failed for (dev %s, type %s) errno=%d\n", > s, sb->s_id, sb->s_type->name, rc); It seems like it would still be useful to see the warning even when sb is NULL, wouldn't you say? How about something like this: if (rc) pr_warn("SELinux: blah blah blah (dev %s, type %s) blah blah\n", (sb ? sb->s_id : "?"), (sb ? sb->s_type->name : "?")); > @@ -976,6 +976,9 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) > { > struct selinux_mnt_opts *opts = *mnt_opts; > bool is_alloc_opts = false; > + bool preparse_sid = false; > + u32 sid; > + int rc; > > if (token == Opt_seclabel) > /* eaten and completely ignored */ > @@ -991,26 +994,57 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) > is_alloc_opts = true; > } > > + if (selinux_initialized(&selinux_state)) > + preparse_sid = true; Since there is no looping in selinux_add_opt, and you can only specify one token/option for a given call to this function, it seems like we can do away with preparse_sid and just do the selinux_initialized(...) check directly in the code below, yes? > switch (token) { > case Opt_context: > if (opts->context || opts->defcontext) > goto err; > opts->context = s; > + if (preparse_sid) { > + rc = parse_sid(NULL, s, &sid); > + if (rc == 0) { > + opts->context_sid = sid; > + opts->preparsed |= CONTEXT_MNT; > + } > + } Is there a reason why we need a dedicated sid variable as opposed to passing opt->context_sid as the parameter? For example: rc = parse_sid(NULL, s, &opts->context_sid);
On Mon, 24 Jan 2022, Paul Moore wrote: > On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > > selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and > > shouldn't be performing any memory allocations. Fix this by parsing the > > sids at the same time we're chopping up the security mount options > > string and then using the pre-parsed sids when doing the comparison. > > > > Fixes: cc274ae7763d ("selinux: fix sleeping function called from invalid context") > > Fixes: 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an existing mount") > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > --- > > security/selinux/hooks.c | 112 ++++++++++++++++++++++++++------------- > > 1 file changed, 76 insertions(+), 36 deletions(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index 5b6895e4fc29..f27ca9e870c0 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -342,6 +342,11 @@ static void inode_free_security(struct inode *inode) > > > > struct selinux_mnt_opts { > > const char *fscontext, *context, *rootcontext, *defcontext; > > + u32 fscontext_sid; > > + u32 context_sid; > > + u32 rootcontext_sid; > > + u32 defcontext_sid; > > + unsigned short preparsed; > > }; > > Is the preparsed field strictly necessary? Can't we just write the > code to assume that if a given SID field is not SECSID_NULL then it is > valid/preparsed? The preparsed field isn't necessary. I'll change it. > > > @@ -598,12 +603,11 @@ static int bad_option(struct superblock_security_struct *sbsec, char flag, > > return 0; > > } > > > > -static int parse_sid(struct super_block *sb, const char *s, u32 *sid, > > - gfp_t gfp) > > +static int parse_sid(struct super_block *sb, const char *s, u32 *sid) > > { > > int rc = security_context_str_to_sid(&selinux_state, s, > > - sid, gfp); > > - if (rc) > > + sid, GFP_KERNEL); > > + if (rc && sb != NULL) > > pr_warn("SELinux: security_context_str_to_sid" > > "(%s) failed for (dev %s, type %s) errno=%d\n", > > s, sb->s_id, sb->s_type->name, rc); > > It seems like it would still be useful to see the warning even when sb > is NULL, wouldn't you say? How about something like this: > > if (rc) > pr_warn("SELinux: blah blah blah (dev %s, type %s) blah blah\n", > (sb ? sb->s_id : "?"), > (sb ? sb->s_type->name : "?")); I agree, that would be useful. > > > @@ -976,6 +976,9 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) > > { > > struct selinux_mnt_opts *opts = *mnt_opts; > > bool is_alloc_opts = false; > > + bool preparse_sid = false; > > + u32 sid; > > + int rc; > > > > if (token == Opt_seclabel) > > /* eaten and completely ignored */ > > @@ -991,26 +994,57 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) > > is_alloc_opts = true; > > } > > > > + if (selinux_initialized(&selinux_state)) > > + preparse_sid = true; > > Since there is no looping in selinux_add_opt, and you can only specify > one token/option for a given call to this function, it seems like we > can do away with preparse_sid and just do the selinux_initialized(...) > check directly in the code below, yes? Will do. > > > switch (token) { > > case Opt_context: > > if (opts->context || opts->defcontext) > > goto err; > > opts->context = s; > > + if (preparse_sid) { > > + rc = parse_sid(NULL, s, &sid); > > + if (rc == 0) { > > + opts->context_sid = sid; > > + opts->preparsed |= CONTEXT_MNT; > > + } > > + } > > Is there a reason why we need a dedicated sid variable as opposed to > passing opt->context_sid as the parameter? For example: > > rc = parse_sid(NULL, s, &opts->context_sid); We don't need a dedicated sid variable. Should I make similar changes in the second patch (get rid of the local sid variable in selinux_sb_remount() and the *context_sid variables in selinux_set_mnt_opts())? Thanks, Scott > > -- > paul moore > paul-moore.com >
On Tue, Jan 25, 2022 at 12:31 PM Scott Mayhew <smayhew@redhat.com> wrote: > On Mon, 24 Jan 2022, Paul Moore wrote: > > On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > > > > selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and > > > shouldn't be performing any memory allocations. Fix this by parsing the > > > sids at the same time we're chopping up the security mount options > > > string and then using the pre-parsed sids when doing the comparison. > > > > > > Fixes: cc274ae7763d ("selinux: fix sleeping function called from invalid context") > > > Fixes: 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an existing mount") > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > > --- > > > security/selinux/hooks.c | 112 ++++++++++++++++++++++++++------------- > > > 1 file changed, 76 insertions(+), 36 deletions(-) ... > > > switch (token) { > > > case Opt_context: > > > if (opts->context || opts->defcontext) > > > goto err; > > > opts->context = s; > > > + if (preparse_sid) { > > > + rc = parse_sid(NULL, s, &sid); > > > + if (rc == 0) { > > > + opts->context_sid = sid; > > > + opts->preparsed |= CONTEXT_MNT; > > > + } > > > + } > > > > Is there a reason why we need a dedicated sid variable as opposed to > > passing opt->context_sid as the parameter? For example: > > > > rc = parse_sid(NULL, s, &opts->context_sid); > > We don't need a dedicated sid variable. Should I make similar changes > in the second patch (get rid of the local sid variable in > selinux_sb_remount() and the *context_sid variables in > selinux_set_mnt_opts())? Yes please, I should have explicitly mentioned that. Thanks.
On Tue, 25 Jan 2022, Paul Moore wrote: > On Tue, Jan 25, 2022 at 12:31 PM Scott Mayhew <smayhew@redhat.com> wrote: > > On Mon, 24 Jan 2022, Paul Moore wrote: > > > On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > > > > > > selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and > > > > shouldn't be performing any memory allocations. Fix this by parsing the > > > > sids at the same time we're chopping up the security mount options > > > > string and then using the pre-parsed sids when doing the comparison. > > > > > > > > Fixes: cc274ae7763d ("selinux: fix sleeping function called from invalid context") > > > > Fixes: 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an existing mount") > > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > > > --- > > > > security/selinux/hooks.c | 112 ++++++++++++++++++++++++++------------- > > > > 1 file changed, 76 insertions(+), 36 deletions(-) > > ... > > > > > switch (token) { > > > > case Opt_context: > > > > if (opts->context || opts->defcontext) > > > > goto err; > > > > opts->context = s; > > > > + if (preparse_sid) { > > > > + rc = parse_sid(NULL, s, &sid); > > > > + if (rc == 0) { > > > > + opts->context_sid = sid; > > > > + opts->preparsed |= CONTEXT_MNT; > > > > + } > > > > + } > > > > > > Is there a reason why we need a dedicated sid variable as opposed to > > > passing opt->context_sid as the parameter? For example: > > > > > > rc = parse_sid(NULL, s, &opts->context_sid); > > > > We don't need a dedicated sid variable. Should I make similar changes > > in the second patch (get rid of the local sid variable in > > selinux_sb_remount() and the *context_sid variables in > > selinux_set_mnt_opts())? > > Yes please, I should have explicitly mentioned that. Actually, delayed_superblock_init() calls selinux_set_mnt_opts() with mnt_opts == NULL, so there would have to be a lot of checks like if (opts && opts->fscontext_sid) { in the later parts of that function, which is kind of clunky. I can still do it if you want though. -Scott > > Thanks. > > -- > paul moore > paul-moore.com >
On Tue, Jan 25, 2022 at 1:51 PM Scott Mayhew <smayhew@redhat.com> wrote: > On Tue, 25 Jan 2022, Paul Moore wrote: > > On Tue, Jan 25, 2022 at 12:31 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > On Mon, 24 Jan 2022, Paul Moore wrote: > > > > On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > > > > > > > > selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and > > > > > shouldn't be performing any memory allocations. Fix this by parsing the > > > > > sids at the same time we're chopping up the security mount options > > > > > string and then using the pre-parsed sids when doing the comparison. > > > > > > > > > > Fixes: cc274ae7763d ("selinux: fix sleeping function called from invalid context") > > > > > Fixes: 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an existing mount") > > > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > > > > --- > > > > > security/selinux/hooks.c | 112 ++++++++++++++++++++++++++------------- > > > > > 1 file changed, 76 insertions(+), 36 deletions(-) > > > > ... > > > > > > > switch (token) { > > > > > case Opt_context: > > > > > if (opts->context || opts->defcontext) > > > > > goto err; > > > > > opts->context = s; > > > > > + if (preparse_sid) { > > > > > + rc = parse_sid(NULL, s, &sid); > > > > > + if (rc == 0) { > > > > > + opts->context_sid = sid; > > > > > + opts->preparsed |= CONTEXT_MNT; > > > > > + } > > > > > + } > > > > > > > > Is there a reason why we need a dedicated sid variable as opposed to > > > > passing opt->context_sid as the parameter? For example: > > > > > > > > rc = parse_sid(NULL, s, &opts->context_sid); > > > > > > We don't need a dedicated sid variable. Should I make similar changes > > > in the second patch (get rid of the local sid variable in > > > selinux_sb_remount() and the *context_sid variables in > > > selinux_set_mnt_opts())? > > > > Yes please, I should have explicitly mentioned that. > > Actually, delayed_superblock_init() calls selinux_set_mnt_opts() with > mnt_opts == NULL, so there would have to be a lot of checks like > > if (opts && opts->fscontext_sid) { > > in the later parts of that function, which is kind of clunky. I can > still do it if you want though. I might be misunderstanding your concern, but in selinux_set_mnt_opts() all of the "opts->XXX" if-conditionals are protected by being inside an if-statement that checks to ensure "opts" is not NULL. Am I missing something?
On Tue, 25 Jan 2022, Paul Moore wrote: > On Tue, Jan 25, 2022 at 1:51 PM Scott Mayhew <smayhew@redhat.com> wrote: > > On Tue, 25 Jan 2022, Paul Moore wrote: > > > On Tue, Jan 25, 2022 at 12:31 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > > On Mon, 24 Jan 2022, Paul Moore wrote: > > > > > On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > > > > > > > > > > selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and > > > > > > shouldn't be performing any memory allocations. Fix this by parsing the > > > > > > sids at the same time we're chopping up the security mount options > > > > > > string and then using the pre-parsed sids when doing the comparison. > > > > > > > > > > > > Fixes: cc274ae7763d ("selinux: fix sleeping function called from invalid context") > > > > > > Fixes: 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an existing mount") > > > > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > > > > > --- > > > > > > security/selinux/hooks.c | 112 ++++++++++++++++++++++++++------------- > > > > > > 1 file changed, 76 insertions(+), 36 deletions(-) > > > > > > ... > > > > > > > > > switch (token) { > > > > > > case Opt_context: > > > > > > if (opts->context || opts->defcontext) > > > > > > goto err; > > > > > > opts->context = s; > > > > > > + if (preparse_sid) { > > > > > > + rc = parse_sid(NULL, s, &sid); > > > > > > + if (rc == 0) { > > > > > > + opts->context_sid = sid; > > > > > > + opts->preparsed |= CONTEXT_MNT; > > > > > > + } > > > > > > + } > > > > > > > > > > Is there a reason why we need a dedicated sid variable as opposed to > > > > > passing opt->context_sid as the parameter? For example: > > > > > > > > > > rc = parse_sid(NULL, s, &opts->context_sid); > > > > > > > > We don't need a dedicated sid variable. Should I make similar changes > > > > in the second patch (get rid of the local sid variable in > > > > selinux_sb_remount() and the *context_sid variables in > > > > selinux_set_mnt_opts())? > > > > > > Yes please, I should have explicitly mentioned that. > > > > Actually, delayed_superblock_init() calls selinux_set_mnt_opts() with > > mnt_opts == NULL, so there would have to be a lot of checks like > > > > if (opts && opts->fscontext_sid) { > > > > in the later parts of that function, which is kind of clunky. I can > > still do it if you want though. > > I might be misunderstanding your concern, but in > selinux_set_mnt_opts() all of the "opts->XXX" if-conditionals are > protected by being inside an if-statement that checks to ensure "opts" > is not NULL. Am I missing something? Sorry for being unclear. The parts where the sids are (potentially) being parsed are inside an "if (opts) { ... }" block... but later in the function those sids are used in various tests/assignments. So if we wanted to eliminate the four local context_sid variables (using the variables in opts instead), then there would need to be additional checks to avoid dereferencing opts when it's NULL. In other words, if the local context_sid variables are kept, then the change to selinux_set_mnt_opts() would look like this (option A): ---8<--- @@ -676,36 +676,48 @@ static int selinux_set_mnt_opts(struct super_block *sb, */ if (opts) { if (opts->fscontext) { - rc = parse_sid(sb, opts->fscontext, &fscontext_sid); - if (rc) - goto out; + if (opts->fscontext_sid == SECSID_NULL ) { + rc = parse_sid(sb, opts->fscontext, &fscontext_sid); + if (rc) + goto out; + } else + fscontext_sid = opts->fscontext_sid; if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, fscontext_sid)) goto out_double_mount; sbsec->flags |= FSCONTEXT_MNT; } if (opts->context) { - rc = parse_sid(sb, opts->context, &context_sid); - if (rc) - goto out; + if (opts->context_sid == SECSID_NULL) { + rc = parse_sid(sb, opts->context, &context_sid); + if (rc) + goto out; + } else + context_sid = opts->context_sid; if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, context_sid)) goto out_double_mount; sbsec->flags |= CONTEXT_MNT; } if (opts->rootcontext) { - rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid); - if (rc) - goto out; + if (opts->rootcontext_sid == SECSID_NULL) { + rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid); + if (rc) + goto out; + } else + rootcontext_sid = opts->rootcontext_sid; if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, rootcontext_sid)) goto out_double_mount; sbsec->flags |= ROOTCONTEXT_MNT; } if (opts->defcontext) { - rc = parse_sid(sb, opts->defcontext, &defcontext_sid); - if (rc) - goto out; + if (opts->defcontext_sid == SECSID_NULL) { + rc = parse_sid(sb, opts->defcontext, &defcontext_sid); + if (rc) + goto out; + } else + defcontext_sid = opts->defcontext_sid; if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, defcontext_sid)) goto out_double_mount; ---8<--- and if the local context_sid variables are removed, the change to selinux_set_mnt_opts() would look like this (option B): ---8<--- @@ -627,8 +627,6 @@ static int selinux_set_mnt_opts(struct super_block *sb, struct dentry *root = sb->s_root; struct selinux_mnt_opts *opts = mnt_opts; struct inode_security_struct *root_isec; - u32 fscontext_sid = 0, context_sid = 0, rootcontext_sid = 0; - u32 defcontext_sid = 0; int rc = 0; mutex_lock(&sbsec->lock); @@ -676,38 +674,50 @@ static int selinux_set_mnt_opts(struct super_block *sb, */ if (opts) { if (opts->fscontext) { - rc = parse_sid(sb, opts->fscontext, &fscontext_sid); - if (rc) - goto out; + if (opts->fscontext_sid == SECSID_NULL ) { + rc = parse_sid(sb, opts->fscontext, + &opts->fscontext_sid); + if (rc) + goto out; + } if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, - fscontext_sid)) + opts->fscontext_sid)) goto out_double_mount; sbsec->flags |= FSCONTEXT_MNT; } if (opts->context) { - rc = parse_sid(sb, opts->context, &context_sid); - if (rc) - goto out; + if (opts->context_sid == SECSID_NULL) { + rc = parse_sid(sb, opts->context, + &opts->context_sid); + if (rc) + goto out; + } if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, - context_sid)) + opts->context_sid)) goto out_double_mount; sbsec->flags |= CONTEXT_MNT; } if (opts->rootcontext) { - rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid); - if (rc) - goto out; + if (opts->rootcontext_sid == SECSID_NULL) { + rc = parse_sid(sb, opts->rootcontext, + &opts->rootcontext_sid); + if (rc) + goto out; + } if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, - rootcontext_sid)) + opts->rootcontext_sid)) goto out_double_mount; sbsec->flags |= ROOTCONTEXT_MNT; } if (opts->defcontext) { - rc = parse_sid(sb, opts->defcontext, &defcontext_sid); - if (rc) - goto out; + if (opts->defcontext_sid == SECSID_NULL) { + rc = parse_sid(sb, opts->defcontext, + &opts->defcontext_sid); + if (rc) + goto out; + } if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, - defcontext_sid)) + opts->defcontext_sid)) goto out_double_mount; sbsec->flags |= DEFCONTEXT_MNT; } @@ -760,8 +770,8 @@ static int selinux_set_mnt_opts(struct super_block *sb, strcmp(sb->s_type->name, "ramfs") && strcmp(sb->s_type->name, "devpts") && strcmp(sb->s_type->name, "overlay")) { - if (context_sid || fscontext_sid || rootcontext_sid || - defcontext_sid) { + if (opts && (opts->context_sid || opts->fscontext_sid || + opts->rootcontext_sid || opts->defcontext_sid)) { rc = -EACCES; goto out; } @@ -779,12 +789,13 @@ static int selinux_set_mnt_opts(struct super_block *sb, } /* sets the context of the superblock for the fs being mounted. */ - if (fscontext_sid) { - rc = may_context_mount_sb_relabel(fscontext_sid, sbsec, cred); + if (opts && opts->fscontext_sid) { + rc = may_context_mount_sb_relabel(opts->fscontext_sid, + sbsec, cred); if (rc) goto out; - sbsec->sid = fscontext_sid; + sbsec->sid = opts->fscontext_sid; } /* @@ -792,42 +803,43 @@ static int selinux_set_mnt_opts(struct super_block *sb, * sets the label used on all file below the mountpoint, and will set * the superblock context if not already set. */ - if (kern_flags & SECURITY_LSM_NATIVE_LABELS && !context_sid) { + if (kern_flags & SECURITY_LSM_NATIVE_LABELS && + !(opts && opts->context_sid)) { sbsec->behavior = SECURITY_FS_USE_NATIVE; *set_kern_flags |= SECURITY_LSM_NATIVE_LABELS; } - if (context_sid) { - if (!fscontext_sid) { - rc = may_context_mount_sb_relabel(context_sid, sbsec, - cred); + if (opts && opts->context_sid) { + if (!opts->fscontext_sid) { + rc = may_context_mount_sb_relabel(opts->context_sid, + sbsec, cred); if (rc) goto out; - sbsec->sid = context_sid; + sbsec->sid = opts->context_sid; } else { - rc = may_context_mount_inode_relabel(context_sid, sbsec, - cred); + rc = may_context_mount_inode_relabel(opts->context_sid, + sbsec, cred); if (rc) goto out; } - if (!rootcontext_sid) - rootcontext_sid = context_sid; + if (!opts->rootcontext_sid) + opts->rootcontext_sid = opts->context_sid; - sbsec->mntpoint_sid = context_sid; + sbsec->mntpoint_sid = opts->context_sid; sbsec->behavior = SECURITY_FS_USE_MNTPOINT; } - if (rootcontext_sid) { - rc = may_context_mount_inode_relabel(rootcontext_sid, sbsec, - cred); + if (opts && opts->rootcontext_sid) { + rc = may_context_mount_inode_relabel(opts->rootcontext_sid, + sbsec, cred); if (rc) goto out; - root_isec->sid = rootcontext_sid; + root_isec->sid = opts->rootcontext_sid; root_isec->initialized = LABEL_INITIALIZED; } - if (defcontext_sid) { + if (opts && opts->defcontext_sid) { if (sbsec->behavior != SECURITY_FS_USE_XATTR && sbsec->behavior != SECURITY_FS_USE_NATIVE) { rc = -EINVAL; @@ -836,14 +848,14 @@ static int selinux_set_mnt_opts(struct super_block *sb, goto out; } - if (defcontext_sid != sbsec->def_sid) { - rc = may_context_mount_inode_relabel(defcontext_sid, + if (opts->defcontext_sid != sbsec->def_sid) { + rc = may_context_mount_inode_relabel(opts->defcontext_sid, sbsec, cred); if (rc) goto out; } - sbsec->def_sid = defcontext_sid; + sbsec->def_sid = opts->defcontext_sid; } out_set_opts: ---8<--- -Scott > > -- > paul-moore.com >
On Thu, Jan 20, 2022 at 10:50 PM Scott Mayhew <smayhew@redhat.com> wrote: > selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and > shouldn't be performing any memory allocations. Fix this by parsing the > sids at the same time we're chopping up the security mount options > string and then using the pre-parsed sids when doing the comparison. > > Fixes: cc274ae7763d ("selinux: fix sleeping function called from invalid context") > Fixes: 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an existing mount") > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > --- > security/selinux/hooks.c | 112 ++++++++++++++++++++++++++------------- > 1 file changed, 76 insertions(+), 36 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 5b6895e4fc29..f27ca9e870c0 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -342,6 +342,11 @@ static void inode_free_security(struct inode *inode) > > struct selinux_mnt_opts { > const char *fscontext, *context, *rootcontext, *defcontext; > + u32 fscontext_sid; > + u32 context_sid; > + u32 rootcontext_sid; > + u32 defcontext_sid; > + unsigned short preparsed; > }; > > static void selinux_free_mnt_opts(void *mnt_opts) > @@ -598,12 +603,11 @@ static int bad_option(struct superblock_security_struct *sbsec, char flag, > return 0; > } > > -static int parse_sid(struct super_block *sb, const char *s, u32 *sid, > - gfp_t gfp) > +static int parse_sid(struct super_block *sb, const char *s, u32 *sid) > { > int rc = security_context_str_to_sid(&selinux_state, s, > - sid, gfp); > - if (rc) > + sid, GFP_KERNEL); > + if (rc && sb != NULL) > pr_warn("SELinux: security_context_str_to_sid" > "(%s) failed for (dev %s, type %s) errno=%d\n", > s, sb->s_id, sb->s_type->name, rc); > @@ -673,8 +677,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, > */ > if (opts) { > if (opts->fscontext) { > - rc = parse_sid(sb, opts->fscontext, &fscontext_sid, > - GFP_KERNEL); > + rc = parse_sid(sb, opts->fscontext, &fscontext_sid); > if (rc) > goto out; > if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, > @@ -683,8 +686,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, > sbsec->flags |= FSCONTEXT_MNT; > } > if (opts->context) { > - rc = parse_sid(sb, opts->context, &context_sid, > - GFP_KERNEL); > + rc = parse_sid(sb, opts->context, &context_sid); > if (rc) > goto out; > if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, > @@ -693,8 +695,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, > sbsec->flags |= CONTEXT_MNT; > } > if (opts->rootcontext) { > - rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid, > - GFP_KERNEL); > + rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid); > if (rc) > goto out; > if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, > @@ -703,8 +704,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, > sbsec->flags |= ROOTCONTEXT_MNT; > } > if (opts->defcontext) { > - rc = parse_sid(sb, opts->defcontext, &defcontext_sid, > - GFP_KERNEL); > + rc = parse_sid(sb, opts->defcontext, &defcontext_sid); > if (rc) > goto out; > if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, > @@ -976,6 +976,9 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) > { > struct selinux_mnt_opts *opts = *mnt_opts; > bool is_alloc_opts = false; > + bool preparse_sid = false; > + u32 sid; > + int rc; > > if (token == Opt_seclabel) > /* eaten and completely ignored */ > @@ -991,26 +994,57 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) > is_alloc_opts = true; > } > > + if (selinux_initialized(&selinux_state)) > + preparse_sid = true; > + I wonder if we could make this all much simpler by *always* doing the label parsing in selinux_add_opt() and just returning an error when !selinux_initialized(&selinux_state). Before the new mount API, mount options were always passed directly to the mount(2) syscall, so it wasn't possible to pass any SELinux mount options before the SELinux policy was loaded. I don't see why we need to jump through hoops here just to support this pseudo-feature of stashing an unparsed label into an fs_context before policy is loaded... Userspace should never need to do that.
On Wed, Jan 26, 2022 at 3:42 PM Scott Mayhew <smayhew@redhat.com> wrote: > On Tue, 25 Jan 2022, Paul Moore wrote: > > On Tue, Jan 25, 2022 at 1:51 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > On Tue, 25 Jan 2022, Paul Moore wrote: > > > > On Tue, Jan 25, 2022 at 12:31 PM Scott Mayhew <smayhew@redhat.com> wrote: > > > > > On Mon, 24 Jan 2022, Paul Moore wrote: > > > > > > On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <smayhew@redhat.com> wrote: ... > Sorry for being unclear. The parts where the sids are (potentially) > being parsed are inside an "if (opts) { ... }" block... but later in the > function those sids are used in various tests/assignments. So if we > wanted to eliminate the four local context_sid variables (using the > variables in opts instead), then there would need to be additional > checks to avoid dereferencing opts when it's NULL. Okay, gotcha. I think keeping the local variables is the right thing to do. My apologies for not noticing that earlier.
On Thu, Jan 27, 2022 at 4:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > I wonder if we could make this all much simpler by *always* doing the > label parsing in selinux_add_opt() and just returning an error when > !selinux_initialized(&selinux_state). Before the new mount API, mount > options were always passed directly to the mount(2) syscall, so it > wasn't possible to pass any SELinux mount options before the SELinux > policy was loaded. I don't see why we need to jump through hoops here > just to support this pseudo-feature of stashing an unparsed label into > an fs_context before policy is loaded... Userspace should never need > to do that. I could agree with that, although part of my mind is a little nervous about the "userspace should *never* ..." because that always seems to bite us. Although I'm struggling to think of a case where userspace would need to set explicit SELinux mount options without having a policy loaded.
On Fri, Jan 28, 2022 at 3:28 AM Paul Moore <paul@paul-moore.com> wrote: > On Thu, Jan 27, 2022 at 4:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > I wonder if we could make this all much simpler by *always* doing the > > label parsing in selinux_add_opt() and just returning an error when > > !selinux_initialized(&selinux_state). Before the new mount API, mount > > options were always passed directly to the mount(2) syscall, so it > > wasn't possible to pass any SELinux mount options before the SELinux > > policy was loaded. I don't see why we need to jump through hoops here > > just to support this pseudo-feature of stashing an unparsed label into > > an fs_context before policy is loaded... Userspace should never need > > to do that. > > I could agree with that, although part of my mind is a little nervous > about the "userspace should *never* ..." because that always seems to > bite us. Although I'm struggling to think of a case where userspace > would need to set explicit SELinux mount options without having a > policy loaded. I get that, but IMO this is enough of an odd "use case" that I wouldn't worry too much. To be affected by this, someone would need to: 1. Use the new mount API, which: a) doesn't even have man pages yet, b) isn't even used by the mount(8) utility yet. 2. Call fsconfig(2) with a SELinux mount option before policy is loaded. 3. Call fsmount(2) with the same fd after policy is loaded. And racing with the policy load doesn't count - that could fail randomly with or without the change. I honestly can't imagine any realistic scenario where someone would do this... -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
On Mon, Jan 31, 2022 at 7:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Fri, Jan 28, 2022 at 3:28 AM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Jan 27, 2022 at 4:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > I wonder if we could make this all much simpler by *always* doing the > > > label parsing in selinux_add_opt() and just returning an error when > > > !selinux_initialized(&selinux_state). Before the new mount API, mount > > > options were always passed directly to the mount(2) syscall, so it > > > wasn't possible to pass any SELinux mount options before the SELinux > > > policy was loaded. I don't see why we need to jump through hoops here > > > just to support this pseudo-feature of stashing an unparsed label into > > > an fs_context before policy is loaded... Userspace should never need > > > to do that. > > > > I could agree with that, although part of my mind is a little nervous > > about the "userspace should *never* ..." because that always seems to > > bite us. Although I'm struggling to think of a case where userspace > > would need to set explicit SELinux mount options without having a > > policy loaded. > > I get that, but IMO this is enough of an odd "use case" that I > wouldn't worry too much ... I understand, but seeing as I'm the only one that defends these things with Linus and others lets do this: 1. Fix what we have now using Scott's patches once he incorporates the feedback. 2. Merge another patch (separate patch(set) please!) which does the parsing in selinux_add_opt(). ... this was if we have to revert #2 we still have the fixes in #1.
On Mon, Jan 31, 2022 at 5:16 PM Paul Moore <paul@paul-moore.com> wrote: > On Mon, Jan 31, 2022 at 7:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Fri, Jan 28, 2022 at 3:28 AM Paul Moore <paul@paul-moore.com> wrote: > > > On Thu, Jan 27, 2022 at 4:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > I wonder if we could make this all much simpler by *always* doing the > > > > label parsing in selinux_add_opt() and just returning an error when > > > > !selinux_initialized(&selinux_state). Before the new mount API, mount > > > > options were always passed directly to the mount(2) syscall, so it > > > > wasn't possible to pass any SELinux mount options before the SELinux > > > > policy was loaded. I don't see why we need to jump through hoops here > > > > just to support this pseudo-feature of stashing an unparsed label into > > > > an fs_context before policy is loaded... Userspace should never need > > > > to do that. > > > > > > I could agree with that, although part of my mind is a little nervous > > > about the "userspace should *never* ..." because that always seems to > > > bite us. Although I'm struggling to think of a case where userspace > > > would need to set explicit SELinux mount options without having a > > > policy loaded. > > > > I get that, but IMO this is enough of an odd "use case" that I > > wouldn't worry too much ... > > I understand, but seeing as I'm the only one that defends these things > with Linus and others lets do this: It's not all black and white: https://lore.kernel.org/lkml/Pine.LNX.4.64.0512291322560.3298@g5.osdl.org/ > 1. Fix what we have now using Scott's patches once he incorporates the feedback. > 2. Merge another patch (separate patch(set) please!) which does the > parsing in selinux_add_opt(). > > ... this was if we have to revert #2 we still have the fixes in #1. Sounds good to me. I can prepare the simplification patch. If anyone does come to complain, then by all means, let's revert it. -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
On Tue, Feb 1, 2022 at 9:38 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Mon, Jan 31, 2022 at 5:16 PM Paul Moore <paul@paul-moore.com> wrote: > > On Mon, Jan 31, 2022 at 7:46 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > On Fri, Jan 28, 2022 at 3:28 AM Paul Moore <paul@paul-moore.com> wrote: > > > > On Thu, Jan 27, 2022 at 4:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > I wonder if we could make this all much simpler by *always* doing the > > > > > label parsing in selinux_add_opt() and just returning an error when > > > > > !selinux_initialized(&selinux_state). Before the new mount API, mount > > > > > options were always passed directly to the mount(2) syscall, so it > > > > > wasn't possible to pass any SELinux mount options before the SELinux > > > > > policy was loaded. I don't see why we need to jump through hoops here > > > > > just to support this pseudo-feature of stashing an unparsed label into > > > > > an fs_context before policy is loaded... Userspace should never need > > > > > to do that. > > > > > > > > I could agree with that, although part of my mind is a little nervous > > > > about the "userspace should *never* ..." because that always seems to > > > > bite us. Although I'm struggling to think of a case where userspace > > > > would need to set explicit SELinux mount options without having a > > > > policy loaded. > > > > > > I get that, but IMO this is enough of an odd "use case" that I > > > wouldn't worry too much ... > > > > I understand, but seeing as I'm the only one that defends these things > > with Linus and others lets do this: > > It's not all black and white: > https://lore.kernel.org/lkml/Pine.LNX.4.64.0512291322560.3298@g5.osdl.org/ I made my statement above not to ask your opinion, but rather to make a point.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 5b6895e4fc29..f27ca9e870c0 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -342,6 +342,11 @@ static void inode_free_security(struct inode *inode) struct selinux_mnt_opts { const char *fscontext, *context, *rootcontext, *defcontext; + u32 fscontext_sid; + u32 context_sid; + u32 rootcontext_sid; + u32 defcontext_sid; + unsigned short preparsed; }; static void selinux_free_mnt_opts(void *mnt_opts) @@ -598,12 +603,11 @@ static int bad_option(struct superblock_security_struct *sbsec, char flag, return 0; } -static int parse_sid(struct super_block *sb, const char *s, u32 *sid, - gfp_t gfp) +static int parse_sid(struct super_block *sb, const char *s, u32 *sid) { int rc = security_context_str_to_sid(&selinux_state, s, - sid, gfp); - if (rc) + sid, GFP_KERNEL); + if (rc && sb != NULL) pr_warn("SELinux: security_context_str_to_sid" "(%s) failed for (dev %s, type %s) errno=%d\n", s, sb->s_id, sb->s_type->name, rc); @@ -673,8 +677,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, */ if (opts) { if (opts->fscontext) { - rc = parse_sid(sb, opts->fscontext, &fscontext_sid, - GFP_KERNEL); + rc = parse_sid(sb, opts->fscontext, &fscontext_sid); if (rc) goto out; if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, @@ -683,8 +686,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, sbsec->flags |= FSCONTEXT_MNT; } if (opts->context) { - rc = parse_sid(sb, opts->context, &context_sid, - GFP_KERNEL); + rc = parse_sid(sb, opts->context, &context_sid); if (rc) goto out; if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, @@ -693,8 +695,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, sbsec->flags |= CONTEXT_MNT; } if (opts->rootcontext) { - rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid, - GFP_KERNEL); + rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid); if (rc) goto out; if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, @@ -703,8 +704,7 @@ static int selinux_set_mnt_opts(struct super_block *sb, sbsec->flags |= ROOTCONTEXT_MNT; } if (opts->defcontext) { - rc = parse_sid(sb, opts->defcontext, &defcontext_sid, - GFP_KERNEL); + rc = parse_sid(sb, opts->defcontext, &defcontext_sid); if (rc) goto out; if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, @@ -976,6 +976,9 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) { struct selinux_mnt_opts *opts = *mnt_opts; bool is_alloc_opts = false; + bool preparse_sid = false; + u32 sid; + int rc; if (token == Opt_seclabel) /* eaten and completely ignored */ @@ -991,26 +994,57 @@ static int selinux_add_opt(int token, const char *s, void **mnt_opts) is_alloc_opts = true; } + if (selinux_initialized(&selinux_state)) + preparse_sid = true; + switch (token) { case Opt_context: if (opts->context || opts->defcontext) goto err; opts->context = s; + if (preparse_sid) { + rc = parse_sid(NULL, s, &sid); + if (rc == 0) { + opts->context_sid = sid; + opts->preparsed |= CONTEXT_MNT; + } + } break; case Opt_fscontext: if (opts->fscontext) goto err; opts->fscontext = s; + if (preparse_sid) { + rc = parse_sid(NULL, s, &sid); + if (rc == 0) { + opts->fscontext_sid = sid; + opts->preparsed |= FSCONTEXT_MNT; + } + } break; case Opt_rootcontext: if (opts->rootcontext) goto err; opts->rootcontext = s; + if (preparse_sid) { + rc = parse_sid(NULL, s, &sid); + if (rc == 0) { + opts->rootcontext_sid = sid; + opts->preparsed |= ROOTCONTEXT_MNT; + } + } break; case Opt_defcontext: if (opts->context || opts->defcontext) goto err; opts->defcontext = s; + if (preparse_sid) { + rc = parse_sid(NULL, s, &sid); + if (rc == 0) { + opts->defcontext_sid = sid; + opts->preparsed |= DEFCONTEXT_MNT; + } + } break; } @@ -2648,8 +2682,6 @@ static int selinux_sb_mnt_opts_compat(struct super_block *sb, void *mnt_opts) { struct selinux_mnt_opts *opts = mnt_opts; struct superblock_security_struct *sbsec = sb->s_security; - u32 sid; - int rc; /* * Superblock not initialized (i.e. no options) - reject if any @@ -2666,35 +2698,43 @@ static int selinux_sb_mnt_opts_compat(struct super_block *sb, void *mnt_opts) return (sbsec->flags & SE_MNTMASK) ? 1 : 0; if (opts->fscontext) { - rc = parse_sid(sb, opts->fscontext, &sid, GFP_NOWAIT); - if (rc) - return 1; - if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, sid)) + if (opts->preparsed & FSCONTEXT_MNT) { + if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, + opts->fscontext_sid)) + return 1; + } else { return 1; + } } if (opts->context) { - rc = parse_sid(sb, opts->context, &sid, GFP_NOWAIT); - if (rc) - return 1; - if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, sid)) + if (opts->preparsed & CONTEXT_MNT) { + if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, + opts->context_sid)) + return 1; + } else { return 1; + } } if (opts->rootcontext) { - struct inode_security_struct *root_isec; + if (opts->preparsed & ROOTCONTEXT_MNT) { + struct inode_security_struct *root_isec; - root_isec = backing_inode_security(sb->s_root); - rc = parse_sid(sb, opts->rootcontext, &sid, GFP_NOWAIT); - if (rc) - return 1; - if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, sid)) + root_isec = backing_inode_security(sb->s_root); + if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, + opts->rootcontext_sid)) + return 1; + } else { return 1; + } } if (opts->defcontext) { - rc = parse_sid(sb, opts->defcontext, &sid, GFP_NOWAIT); - if (rc) - return 1; - if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, sid)) + if (opts->preparsed & DEFCONTEXT_MNT) { + if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, + opts->defcontext_sid)) + return 1; + } else { return 1; + } } return 0; } @@ -2713,14 +2753,14 @@ static int selinux_sb_remount(struct super_block *sb, void *mnt_opts) return 0; if (opts->fscontext) { - rc = parse_sid(sb, opts->fscontext, &sid, GFP_KERNEL); + rc = parse_sid(sb, opts->fscontext, &sid); if (rc) return rc; if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, sid)) goto out_bad_option; } if (opts->context) { - rc = parse_sid(sb, opts->context, &sid, GFP_KERNEL); + rc = parse_sid(sb, opts->context, &sid); if (rc) return rc; if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, sid)) @@ -2729,14 +2769,14 @@ static int selinux_sb_remount(struct super_block *sb, void *mnt_opts) if (opts->rootcontext) { struct inode_security_struct *root_isec; root_isec = backing_inode_security(sb->s_root); - rc = parse_sid(sb, opts->rootcontext, &sid, GFP_KERNEL); + rc = parse_sid(sb, opts->rootcontext, &sid); if (rc) return rc; if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, sid)) goto out_bad_option; } if (opts->defcontext) { - rc = parse_sid(sb, opts->defcontext, &sid, GFP_KERNEL); + rc = parse_sid(sb, opts->defcontext, &sid); if (rc) return rc; if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, sid))
selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and shouldn't be performing any memory allocations. Fix this by parsing the sids at the same time we're chopping up the security mount options string and then using the pre-parsed sids when doing the comparison. Fixes: cc274ae7763d ("selinux: fix sleeping function called from invalid context") Fixes: 69c4a42d72eb ("lsm,selinux: add new hook to compare new mount to an existing mount") Signed-off-by: Scott Mayhew <smayhew@redhat.com> --- security/selinux/hooks.c | 112 ++++++++++++++++++++++++++------------- 1 file changed, 76 insertions(+), 36 deletions(-)