From: Scott Mayhew <smayhew@redhat.com>
To: Paul Moore <paul@paul-moore.com>
Cc: selinux@vger.kernel.org, linux-nfs@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat()
Date: Wed, 26 Jan 2022 15:41:58 -0500 [thread overview]
Message-ID: <YfGyFhA0ZQPagROG@aion.usersys.redhat.com> (raw)
In-Reply-To: <CAHC9VhRoWbnV-cs2HzmiTEd7_kP914stdVpN9Tm2-6uua2-ELA@mail.gmail.com>
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
>
next prev parent reply other threads:[~2022-01-26 20:42 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-20 21:49 [PATCH RFC v2 0/2] selinux: parse sids earlier to avoid doing memory allocations under spinlock Scott Mayhew
2022-01-20 21:49 ` [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat() Scott Mayhew
2022-01-24 21:27 ` Paul Moore
2022-01-25 17:30 ` Scott Mayhew
2022-01-25 17:45 ` Paul Moore
2022-01-25 18:51 ` Scott Mayhew
2022-01-25 22:32 ` Paul Moore
2022-01-26 20:41 ` Scott Mayhew [this message]
2022-01-28 1:53 ` Paul Moore
2022-01-27 9:54 ` Ondrej Mosnacek
2022-01-28 2:25 ` Paul Moore
2022-01-31 12:46 ` Ondrej Mosnacek
2022-01-31 16:16 ` Paul Moore
2022-02-01 14:38 ` Ondrej Mosnacek
2022-02-01 16:19 ` Paul Moore
2022-01-20 21:49 ` [PATCH RFC v2 2/2] selinux: try to use preparsed sid before calling parse_sid() Scott Mayhew
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YfGyFhA0ZQPagROG@aion.usersys.redhat.com \
--to=smayhew@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=paul@paul-moore.com \
--cc=selinux@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.