[PATCH v3] selinux: lsm: fix a missing-check bug in selinux_add_mnt_opt( )

[PATCH v3] selinux: lsm: fix a missing-check bug in selinux_add_mnt_opt( )

[Gen Zhang]

In selinux_add_mnt_opt(), 'val' is allocated by kmemdup_nul(). It returns
NULL when fails. So 'val' should be checked. And 'mnt_opts' should be 
freed when error.

Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
Fixes: 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()")
---
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702c..4e4c1c6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1052,15 +1052,23 @@ static int selinux_add_mnt_opt(const char *option, const char *val, int len,
 	if (token == Opt_error)
 		return -EINVAL;
 
-	if (token != Opt_seclabel)
-		val = kmemdup_nul(val, len, GFP_KERNEL);
+	if (token != Opt_seclabel) {
+			val = kmemdup_nul(val, len, GFP_KERNEL);
+			if (!val) {
+				rc = -ENOMEM;
+				goto free_opt;
+			}
+	}
 	rc = selinux_add_opt(token, val, mnt_opts);
 	if (unlikely(rc)) {
 		kfree(val);
-		if (*mnt_opts) {
-			selinux_free_mnt_opts(*mnt_opts);
-			*mnt_opts = NULL;
-		}
+		goto free_opt;
+	}
+	return rc;
+free_opt:
+	if (*mnt_opts) {
+		selinux_free_mnt_opts(*mnt_opts);
+		*mnt_opts = NULL;
 	}
 	return rc;
 }

Re: [PATCH v3] selinux: lsm: fix a missing-check bug in selinux_add_mnt_opt( )

[Ondrej Mosnacek]

On Thu, Jun 6, 2019 at 11:23 AM Gen Zhang <blackgod016574@gmail.com> wrote:
> In selinux_add_mnt_opt(), 'val' is allocated by kmemdup_nul(). It returns
> NULL when fails. So 'val' should be checked. And 'mnt_opts' should be
> freed when error.
>
> Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> Fixes: 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()")
> ---
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3ec702c..4e4c1c6 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1052,15 +1052,23 @@ static int selinux_add_mnt_opt(const char *option, const char *val, int len,
>         if (token == Opt_error)
>                 return -EINVAL;
>
> -       if (token != Opt_seclabel)
> -               val = kmemdup_nul(val, len, GFP_KERNEL);
> +       if (token != Opt_seclabel) {
> +                       val = kmemdup_nul(val, len, GFP_KERNEL);
> +                       if (!val) {
> +                               rc = -ENOMEM;
> +                               goto free_opt;
> +                       }
> +       }
>         rc = selinux_add_opt(token, val, mnt_opts);
>         if (unlikely(rc)) {
>                 kfree(val);
> -               if (*mnt_opts) {
> -                       selinux_free_mnt_opts(*mnt_opts);
> -                       *mnt_opts = NULL;
> -               }
> +               goto free_opt;
> +       }
> +       return rc;

At this point rc is guaranteed to be 0, so you can just 'return 0' for
clarity. Also, I visually prefer an empty line between a return
statement and a goto label, but I'm not sure what is the
general/maintainer's preference.

Also, you should drop the "lsm: " from the subject - it is redundant
and doesn't follow the SELinux convention. See `git log --oneline --
security/selinux/` for what the subjects usually look like - mostly we
just go with "selinux: <description>" (or "LSM: <description>" when
the changes affect the shared LSM interface).

> +free_opt:
> +       if (*mnt_opts) {
> +               selinux_free_mnt_opts(*mnt_opts);
> +               *mnt_opts = NULL;
>         }
>         return rc;
>  }

--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH v3] selinux: lsm: fix a missing-check bug in selinux_add_mnt_opt( )

[Gen Zhang]

On Fri, Jun 07, 2019 at 10:39:05AM +0200, Ondrej Mosnacek wrote:
> On Thu, Jun 6, 2019 at 11:23 AM Gen Zhang <blackgod016574@gmail.com> wrote:
> > In selinux_add_mnt_opt(), 'val' is allocated by kmemdup_nul(). It returns
> > NULL when fails. So 'val' should be checked. And 'mnt_opts' should be
> > freed when error.
> >
> > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> > Fixes: 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()")
> > ---
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 3ec702c..4e4c1c6 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1052,15 +1052,23 @@ static int selinux_add_mnt_opt(const char *option, const char *val, int len,
> >         if (token == Opt_error)
> >                 return -EINVAL;
> >
> > -       if (token != Opt_seclabel)
> > -               val = kmemdup_nul(val, len, GFP_KERNEL);
> > +       if (token != Opt_seclabel) {
> > +                       val = kmemdup_nul(val, len, GFP_KERNEL);
> > +                       if (!val) {
> > +                               rc = -ENOMEM;
> > +                               goto free_opt;
> > +                       }
> > +       }
> >         rc = selinux_add_opt(token, val, mnt_opts);
> >         if (unlikely(rc)) {
> >                 kfree(val);
> > -               if (*mnt_opts) {
> > -                       selinux_free_mnt_opts(*mnt_opts);
> > -                       *mnt_opts = NULL;
> > -               }
> > +               goto free_opt;
> > +       }
> > +       return rc;
> 
> At this point rc is guaranteed to be 0, so you can just 'return 0' for
> clarity. Also, I visually prefer an empty line between a return
> statement and a goto label, but I'm not sure what is the
> general/maintainer's preference.
Am I supposed to revise and send a patch v4 for this, or let the 
maintainer do this? :-)

Thanks
Gen
> 
> Also, you should drop the "lsm: " from the subject - it is redundant
> and doesn't follow the SELinux convention. See `git log --oneline --
> security/selinux/` for what the subjects usually look like - mostly we
> just go with "selinux: <description>" (or "LSM: <description>" when
> the changes affect the shared LSM interface).
Thanks for your comments.
> 
> > +free_opt:
> > +       if (*mnt_opts) {
> > +               selinux_free_mnt_opts(*mnt_opts);
> > +               *mnt_opts = NULL;
> >         }
> >         return rc;
> >  }
> 
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Software Engineer, Security Technologies
> Red Hat, Inc.

Re: [PATCH v3] selinux: lsm: fix a missing-check bug in selinux_add_mnt_opt( )

[Paul Moore]

On Fri, Jun 7, 2019 at 8:11 AM Gen Zhang <blackgod016574@gmail.com> wrote:
>
> On Fri, Jun 07, 2019 at 10:39:05AM +0200, Ondrej Mosnacek wrote:
> > On Thu, Jun 6, 2019 at 11:23 AM Gen Zhang <blackgod016574@gmail.com> wrote:
> > > In selinux_add_mnt_opt(), 'val' is allocated by kmemdup_nul(). It returns
> > > NULL when fails. So 'val' should be checked. And 'mnt_opts' should be
> > > freed when error.
> > >
> > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> > > Fixes: 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()")
> > > ---
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 3ec702c..4e4c1c6 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -1052,15 +1052,23 @@ static int selinux_add_mnt_opt(const char *option, const char *val, int len,
> > >         if (token == Opt_error)
> > >                 return -EINVAL;
> > >
> > > -       if (token != Opt_seclabel)
> > > -               val = kmemdup_nul(val, len, GFP_KERNEL);
> > > +       if (token != Opt_seclabel) {
> > > +                       val = kmemdup_nul(val, len, GFP_KERNEL);
> > > +                       if (!val) {
> > > +                               rc = -ENOMEM;
> > > +                               goto free_opt;
> > > +                       }
> > > +       }
> > >         rc = selinux_add_opt(token, val, mnt_opts);
> > >         if (unlikely(rc)) {
> > >                 kfree(val);
> > > -               if (*mnt_opts) {
> > > -                       selinux_free_mnt_opts(*mnt_opts);
> > > -                       *mnt_opts = NULL;
> > > -               }
> > > +               goto free_opt;
> > > +       }
> > > +       return rc;
> >
> > At this point rc is guaranteed to be 0, so you can just 'return 0' for
> > clarity. Also, I visually prefer an empty line between a return
> > statement and a goto label, but I'm not sure what is the
> > general/maintainer's preference.
>
> Am I supposed to revise and send a patch v4 for this, or let the
> maintainer do this? :-)

First a few things from my perspective: I don't really care too much
about the difference between returning "0" and "rc" here, one could
argue that "0" is cleaner and that "rc" is "safer".  To me it isn't a
big deal and generally isn't something I would even comment on unless
there was something else in the patch that needed addressing.  I care
a more about the style choice of having an empty line between the
return and the start of the goto targets (vertical whitespace before
the jump targets is good, please include it), but once again, I'm not
sure I would comment on that.  The patch subject line is a bit
confusing in that we already discussed when to use "selinux" and when
to use "lsm", but I imagine there might be some confusion about using
both so let me try and clear that up now: don't do it unless you have
a *really* good reason to do so :)  In this case it is all SELinux
code so there is no reason why you should be including the "lsm"
prefix.

You've been pretty responsive, so if you don't mind submitting a v4
with the changes mentioned above, that would be far more preferable to
me making the changes.  I have some other comments about maintainer
fixes to patches, but I'll save that for the other thread :)

-- 
paul moore
www.paul-moore.com

Re: [PATCH v3] selinux: lsm: fix a missing-check bug in selinux_add_mnt_opt( )

[Gen Zhang]

On Mon, Jun 10, 2019 at 03:31:50PM -0400, Paul Moore wrote:
> On Fri, Jun 7, 2019 at 8:11 AM Gen Zhang <blackgod016574@gmail.com> wrote:
> >
> > On Fri, Jun 07, 2019 at 10:39:05AM +0200, Ondrej Mosnacek wrote:
> > > On Thu, Jun 6, 2019 at 11:23 AM Gen Zhang <blackgod016574@gmail.com> wrote:
> > > > In selinux_add_mnt_opt(), 'val' is allocated by kmemdup_nul(). It returns
> > > > NULL when fails. So 'val' should be checked. And 'mnt_opts' should be
> > > > freed when error.
> > > >
> > > > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> > > > Fixes: 757cbe597fe8 ("LSM: new method: ->sb_add_mnt_opt()")
> > > > ---
> > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > > index 3ec702c..4e4c1c6 100644
> > > > --- a/security/selinux/hooks.c
> > > > +++ b/security/selinux/hooks.c
> > > > @@ -1052,15 +1052,23 @@ static int selinux_add_mnt_opt(const char *option, const char *val, int len,
> > > >         if (token == Opt_error)
> > > >                 return -EINVAL;
> > > >
> > > > -       if (token != Opt_seclabel)
> > > > -               val = kmemdup_nul(val, len, GFP_KERNEL);
> > > > +       if (token != Opt_seclabel) {
> > > > +                       val = kmemdup_nul(val, len, GFP_KERNEL);
> > > > +                       if (!val) {
> > > > +                               rc = -ENOMEM;
> > > > +                               goto free_opt;
> > > > +                       }
> > > > +       }
> > > >         rc = selinux_add_opt(token, val, mnt_opts);
> > > >         if (unlikely(rc)) {
> > > >                 kfree(val);
> > > > -               if (*mnt_opts) {
> > > > -                       selinux_free_mnt_opts(*mnt_opts);
> > > > -                       *mnt_opts = NULL;
> > > > -               }
> > > > +               goto free_opt;
> > > > +       }
> > > > +       return rc;
> > >
> > > At this point rc is guaranteed to be 0, so you can just 'return 0' for
> > > clarity. Also, I visually prefer an empty line between a return
> > > statement and a goto label, but I'm not sure what is the
> > > general/maintainer's preference.
> >
> > Am I supposed to revise and send a patch v4 for this, or let the
> > maintainer do this? :-)
> 
> First a few things from my perspective: I don't really care too much
> about the difference between returning "0" and "rc" here, one could
> argue that "0" is cleaner and that "rc" is "safer".  To me it isn't a
> big deal and generally isn't something I would even comment on unless
> there was something else in the patch that needed addressing.  I care
> a more about the style choice of having an empty line between the
> return and the start of the goto targets (vertical whitespace before
> the jump targets is good, please include it), but once again, I'm not
> sure I would comment on that.  The patch subject line is a bit
> confusing in that we already discussed when to use "selinux" and when
> to use "lsm", but I imagine there might be some confusion about using
> both so let me try and clear that up now: don't do it unless you have
> a *really* good reason to do so :)  In this case it is all SELinux
> code so there is no reason why you should be including the "lsm"
> prefix.
Thanks for your comments. I was uncertain of the meaning of "lsm". So I
used"selinux: lsm:". I am aware of that now.

Thanks
Gen
> 
> You've been pretty responsive, so if you don't mind submitting a v4
> with the changes mentioned above, that would be far more preferable to
> me making the changes.  I have some other comments about maintainer
> fixes to patches, but I'll save that for the other thread :)
> 
> -- 
> paul moore
> www.paul-moore.com