[PATCH RFC v2 0/2] selinux: parse sids earlier to avoid doing memory allocations under spinlock

[PATCH RFC v2 0/2] selinux: parse sids earlier to avoid doing memory allocations under spinlock

[Scott Mayhew]

selinux_sb_mnt_opts_compat() is called under the sb_lock spinlock and
shouldn't be performing any memory allocations. 

The first patch fixes this by parsing the sids at the same time the
context mount options are being parsed from the mount options string
and storing the parsed sids in the selinux_mnt_opts struct. 

The second patch adds logic to selinux_set_mnt_opts() and
selinux_sb_remount() that checks to see if a sid has already been
parsed before calling parse_sid(), and adds the parsed sids to the
data being copied in selinux_fs_context_dup().

Scott Mayhew (2):
  selinux: Fix selinux_sb_mnt_opts_compat()
  selinux: try to use preparsed sid before calling parse_sid()

 security/selinux/hooks.c | 181 ++++++++++++++++++++++++++++-----------
 1 file changed, 129 insertions(+), 52 deletions(-)

-- 
2.31.1

[PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat()

[Scott Mayhew]

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;
+
 	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

[PATCH RFC v2 2/2] selinux: try to use preparsed sid before calling parse_sid()

[Scott Mayhew]

Avoid unnecessary parsing of sids that have already been parsed via
selinux_sb_eat_lsm_opts().

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 security/selinux/hooks.c | 85 ++++++++++++++++++++++++++++------------
 1 file changed, 61 insertions(+), 24 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f27ca9e870c0..28ba5c8529fa 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -677,36 +677,52 @@ 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->preparsed & FSCONTEXT_MNT)
+				fscontext_sid = opts->fscontext_sid;
+			else {
+				rc = parse_sid(sb, opts->fscontext, &fscontext_sid);
+				if (rc)
+					goto out;
+			}
 			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->preparsed & CONTEXT_MNT)
+				context_sid = opts->context_sid;
+			else {
+				rc = parse_sid(sb, opts->context, &context_sid);
+				if (rc)
+					goto out;
+			}
 			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->preparsed & ROOTCONTEXT_MNT)
+				rootcontext_sid = opts->rootcontext_sid;
+			else {
+				rc = parse_sid(sb, opts->rootcontext, &rootcontext_sid);
+				if (rc)
+					goto out;
+			}
 			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->preparsed & DEFCONTEXT_MNT)
+				defcontext_sid = opts->defcontext_sid;
+			else {
+				rc = parse_sid(sb, opts->defcontext, &defcontext_sid);
+				if (rc)
+					goto out;
+			}
 			if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid,
 					defcontext_sid))
 				goto out_double_mount;
@@ -2753,32 +2769,48 @@ static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)
 		return 0;
 
 	if (opts->fscontext) {
-		rc = parse_sid(sb, opts->fscontext, &sid);
-		if (rc)
-			return rc;
+		if (opts->preparsed & FSCONTEXT_MNT)
+			sid = opts->fscontext_sid;
+		else {
+			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);
-		if (rc)
-			return rc;
+		if (opts->preparsed & CONTEXT_MNT)
+			sid = opts->context_sid;
+		else {
+			rc = parse_sid(sb, opts->context, &sid);
+			if (rc)
+				return rc;
+		}
 		if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, sid))
 			goto out_bad_option;
 	}
 	if (opts->rootcontext) {
 		struct inode_security_struct *root_isec;
 		root_isec = backing_inode_security(sb->s_root);
-		rc = parse_sid(sb, opts->rootcontext, &sid);
-		if (rc)
-			return rc;
+		if (opts->preparsed & ROOTCONTEXT_MNT)
+			sid = opts->rootcontext_sid;
+		else {
+			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);
-		if (rc)
-			return rc;
+		if (opts->preparsed & DEFCONTEXT_MNT)
+			sid = opts->defcontext_sid;
+		else {
+			rc = parse_sid(sb, opts->defcontext, &sid);
+			if (rc)
+				return rc;
+		}
 		if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, sid))
 			goto out_bad_option;
 	}
@@ -2877,6 +2909,11 @@ static int selinux_fs_context_dup(struct fs_context *fc,
 		if (!opts->defcontext)
 			return -ENOMEM;
 	}
+	opts->fscontext_sid = src->fscontext_sid;
+	opts->context_sid = src->context_sid;
+	opts->rootcontext_sid = src->rootcontext_sid;
+	opts->defcontext_sid = src->defcontext_sid;
+	opts->preparsed = src->preparsed;
 	return 0;
 }
 
-- 
2.31.1

Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat()

[Paul Moore]

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);

-- 
paul moore
paul-moore.com

Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat()

[Scott Mayhew]

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
> 

Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat()

[Paul Moore]

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.

-- 
paul moore
paul-moore.com

Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat()

[Scott Mayhew]

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
> 

Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat()

[Paul Moore]

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?

-- 
paul-moore.com

Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat()

[Scott Mayhew]

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
> 

Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat()

[Ondrej Mosnacek]

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.

Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat()

[Paul Moore]

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.

-- 
paul-moore.com

Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat()

[Paul Moore]

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.

-- 
paul-moore.com

Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat()

[Ondrej Mosnacek]

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.

Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat()

[Paul Moore]

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.

-- 
paul-moore.com

Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat()

[Ondrej Mosnacek]

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.

Re: [PATCH RFC v2 1/2] selinux: Fix selinux_sb_mnt_opts_compat()

[Paul Moore]

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.

-- 
paul-moore.com