selinux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ondrej Mosnacek <omosnace@redhat.com>
To: Scott Mayhew <smayhew@redhat.com>
Cc: SElinux list <selinux@vger.kernel.org>,
	linux-nfs <linux-nfs@vger.kernel.org>,
	Linux kernel mailing list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat()
Date: Thu, 27 Jan 2022 10:54:35 +0100	[thread overview]
Message-ID: <CAFqZXNv7=ROfyzZGojy2DQvY0xp4Dd5oHW_0KG6BLiD7A8zeKQ@mail.gmail.com> (raw)
In-Reply-To: <20220120214948.3637895-2-smayhew@redhat.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.

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.


  parent reply	other threads:[~2022-01-27  9:54 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
2022-01-28  1:53               ` Paul Moore
2022-01-27  9:54   ` Ondrej Mosnacek [this message]
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='CAFqZXNv7=ROfyzZGojy2DQvY0xp4Dd5oHW_0KG6BLiD7A8zeKQ@mail.gmail.com' \
    --to=omosnace@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=selinux@vger.kernel.org \
    --cc=smayhew@redhat.com \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).