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 <[email protected]>
---
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;
+
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))
--
2.31.1
On Mon, 24 Jan 2022, Paul Moore wrote:
> On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]> wrote:
> On Mon, 24 Jan 2022, Paul Moore wrote:
> > On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.
--
paul moore
paul-moore.com
On Tue, 25 Jan 2022, Paul Moore wrote:
> On Tue, Jan 25, 2022 at 1:51 PM Scott Mayhew <[email protected]> wrote:
> > On Tue, 25 Jan 2022, Paul Moore wrote:
> > > On Tue, Jan 25, 2022 at 12:31 PM Scott Mayhew <[email protected]> wrote:
> > > > On Mon, 24 Jan 2022, Paul Moore wrote:
> > > > > On Thu, Jan 20, 2022 at 4:50 PM Scott Mayhew <[email protected]> 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 <[email protected]>
> > > > > > ---
> > > > > > 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 <[email protected]> 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 <[email protected]>
> ---
> 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.
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
On Thu, Jan 27, 2022 at 4:54 AM Ondrej Mosnacek <[email protected]> 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.
--
paul-moore.com
On Fri, Jan 28, 2022 at 3:28 AM Paul Moore <[email protected]> wrote:
> On Thu, Jan 27, 2022 at 4:54 AM Ondrej Mosnacek <[email protected]> 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 5:16 PM Paul Moore <[email protected]> wrote:
> On Mon, Jan 31, 2022 at 7:46 AM Ondrej Mosnacek <[email protected]> wrote:
> > On Fri, Jan 28, 2022 at 3:28 AM Paul Moore <[email protected]> wrote:
> > > On Thu, Jan 27, 2022 at 4:54 AM Ondrej Mosnacek <[email protected]> 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/[email protected]/
> 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 <[email protected]> wrote:
> On Mon, Jan 31, 2022 at 5:16 PM Paul Moore <[email protected]> wrote:
> > On Mon, Jan 31, 2022 at 7:46 AM Ondrej Mosnacek <[email protected]> wrote:
> > > On Fri, Jan 28, 2022 at 3:28 AM Paul Moore <[email protected]> wrote:
> > > > On Thu, Jan 27, 2022 at 4:54 AM Ondrej Mosnacek <[email protected]> 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/[email protected]/
I made my statement above not to ask your opinion, but rather to make a point.
--
paul-moore.com