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

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

[Gen Zhang]

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

Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
---
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702c..f329fc0 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2616,6 +2616,7 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
 	char *from = options;
 	char *to = options;
 	bool first = true;
+	int ret;
 
 	while (1) {
 		int len = opt_len(from);
@@ -2635,15 +2636,16 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
 						*q++ = c;
 				}
 				arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
+				if (!arg) {
+					ret = -ENOMEM;
+					goto free_opt;
+				}
 			}
 			rc = selinux_add_opt(token, arg, mnt_opts);
 			if (unlikely(rc)) {
+				ret = rc;
 				kfree(arg);
-				if (*mnt_opts) {
-					selinux_free_mnt_opts(*mnt_opts);
-					*mnt_opts = NULL;
-				}
-				return rc;
+				goto free_opt;
 			}
 		} else {
 			if (!first) {	// copy with preceding comma
@@ -2661,6 +2663,12 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
 	}
 	*to = '\0';
 	return 0;
+free_opt:
+	if (*mnt_opts) {
+		selinux_free_mnt_opts(*mnt_opts);
+		*mnt_opts = NULL;
+	}
+	return ret;
 }
 
 static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)

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

[Al Viro]

On Sat, Jun 01, 2019 at 10:15:26AM +0800, Gen Zhang wrote:
> In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> returns NULL when fails. So 'arg' should be checked. And 'mnt_opts' 
> should be freed when error.

What's the latter one for?  On failure we'll get to put_fs_context()
pretty soon, so
        security_free_mnt_opts(&fc->security);
will be called just fine.  Leaving it allocated on failure is fine...

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

[Al Viro]

On Sat, Jun 01, 2019 at 03:25:27AM +0100, Al Viro wrote:
> On Sat, Jun 01, 2019 at 10:15:26AM +0800, Gen Zhang wrote:
> > In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> > returns NULL when fails. So 'arg' should be checked. And 'mnt_opts' 
> > should be freed when error.
> 
> What's the latter one for?  On failure we'll get to put_fs_context()
> pretty soon, so
>         security_free_mnt_opts(&fc->security);
> will be called just fine.  Leaving it allocated on failure is fine...

Actually, right now in mainline it is not (btrfs_mount_root() has
an odd call of security_sb_eat_lsm_opts()); eventually we will be
down to just the callers in ->parse_monolithic() instances, at which
point the above will become correct.  At the moment it is not, so
consider the objection withdrawn (and I really need to get some sleep,
seeing how long did it take me to recall the context... ;-/)

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

[Gen Zhang]

On Sat, Jun 01, 2019 at 03:25:27AM +0100, Al Viro wrote:
> On Sat, Jun 01, 2019 at 10:15:26AM +0800, Gen Zhang wrote:
> > In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> > returns NULL when fails. So 'arg' should be checked. And 'mnt_opts' 
> > should be freed when error.
> 
> What's the latter one for?  On failure we'll get to put_fs_context()
> pretty soon, so
>         security_free_mnt_opts(&fc->security);
> will be called just fine.  Leaving it allocated on failure is fine...
Paul Moore <paul@paul-moore.com> wrote:
>It seems like we should also check for, and potentially free *mnt_opts
>as the selinux_add_opt() error handling does just below this change,
>yes?  If that is the case we might want to move that error handling
>code to the bottom of the function and jump there on error.
I am not familiar with this part. So could you please show the function
call sequence?

Thanks
Gen

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

[Gen Zhang]

On Sat, Jun 01, 2019 at 03:34:49AM +0100, Al Viro wrote:
> On Sat, Jun 01, 2019 at 03:25:27AM +0100, Al Viro wrote:
> > On Sat, Jun 01, 2019 at 10:15:26AM +0800, Gen Zhang wrote:
> > > In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> > > returns NULL when fails. So 'arg' should be checked. And 'mnt_opts' 
> > > should be freed when error.
> > 
> > What's the latter one for?  On failure we'll get to put_fs_context()
> > pretty soon, so
> >         security_free_mnt_opts(&fc->security);
> > will be called just fine.  Leaving it allocated on failure is fine...
> 
> Actually, right now in mainline it is not (btrfs_mount_root() has
> an odd call of security_sb_eat_lsm_opts()); eventually we will be
> down to just the callers in ->parse_monolithic() instances, at which
> point the above will become correct.  At the moment it is not, so
> consider the objection withdrawn (and I really need to get some sleep,
> seeing how long did it take me to recall the context... ;-/)
Thanks for your comments. And have a good dream.

Thanks
Gen

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

[Ondrej Mosnacek]

On Sat, Jun 1, 2019 at 4:15 AM Gen Zhang <blackgod016574@gmail.com> wrote:
> In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> returns NULL when fails. So 'arg' should be checked. And 'mnt_opts'
> should be freed when error.
>
> Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>

It looks like you're new to the kernel development community, so let
me give you a bit of friendly advice for the future :)

You don't need to repost the patch when people give you
Acked-by/Reviewed-by/Tested-by (unless there is a different reason to
respin/repost the patches). The maintainer goes over the replies when
applying the final patch and adds Acked-by/Reviewed-by/... on his/her
own.

If you *do* need to respin a path for which you have received A/R/T,
then you need to distinguish between two cases:
1. Only trivial changes to the patch (only fixed typos, edited commit
message, removed empty line, etc. - for example, v1 -> v2 of this
patch falls into this category) - in this case you can collect the
A/R/T yourself and add them to the new version. This saves the
maintainer and the reviewers from redundant work, since the patch is
still semantically the same and the A/R/T from the last version still
apply.
2. Non-trivial changes to the patch (as is the case for this patch) -
in this case your patch needs to be reviewed again and you should
disregard all A/R/T from the previous version. You can easily piss
someone off if you add their Reviewed-by to a patch they haven't
actually reviewed, so be careful ;-)

(Someone please correct me if I got it wrong - this is what I gathered
so far from my experience.)

Good luck in your future work!

> Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
> ---
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3ec702c..f329fc0 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2616,6 +2616,7 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
>         char *from = options;
>         char *to = options;
>         bool first = true;
> +       int ret;
>
>         while (1) {
>                 int len = opt_len(from);
> @@ -2635,15 +2636,16 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
>                                                 *q++ = c;
>                                 }
>                                 arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
> +                               if (!arg) {
> +                                       ret = -ENOMEM;
> +                                       goto free_opt;
> +                               }
>                         }
>                         rc = selinux_add_opt(token, arg, mnt_opts);
>                         if (unlikely(rc)) {
> +                               ret = rc;
>                                 kfree(arg);
> -                               if (*mnt_opts) {
> -                                       selinux_free_mnt_opts(*mnt_opts);
> -                                       *mnt_opts = NULL;
> -                               }
> -                               return rc;
> +                               goto free_opt;
>                         }
>                 } else {
>                         if (!first) {   // copy with preceding comma
> @@ -2661,6 +2663,12 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
>         }
>         *to = '\0';
>         return 0;
> +free_opt:
> +       if (*mnt_opts) {
> +               selinux_free_mnt_opts(*mnt_opts);
> +               *mnt_opts = NULL;
> +       }
> +       return ret;
>  }
>
>  static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)

-- 
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_sb_eat_lsm_opts()

[Ondrej Mosnacek]

On Sat, Jun 1, 2019 at 4:15 AM Gen Zhang <blackgod016574@gmail.com> wrote:
> In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> returns NULL when fails. So 'arg' should be checked. And 'mnt_opts'
> should be freed when error.
>
> Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
> Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
> ---
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3ec702c..f329fc0 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2616,6 +2616,7 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
>         char *from = options;
>         char *to = options;
>         bool first = true;
> +       int ret;

I'd suggest just moving the declaration of 'rc' here and simply reuse
that variable. Otherwise the patch looks good to me.

>
>         while (1) {
>                 int len = opt_len(from);
> @@ -2635,15 +2636,16 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
>                                                 *q++ = c;
>                                 }
>                                 arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
> +                               if (!arg) {
> +                                       ret = -ENOMEM;
> +                                       goto free_opt;
> +                               }
>                         }
>                         rc = selinux_add_opt(token, arg, mnt_opts);
>                         if (unlikely(rc)) {
> +                               ret = rc;
>                                 kfree(arg);
> -                               if (*mnt_opts) {
> -                                       selinux_free_mnt_opts(*mnt_opts);
> -                                       *mnt_opts = NULL;
> -                               }
> -                               return rc;
> +                               goto free_opt;
>                         }
>                 } else {
>                         if (!first) {   // copy with preceding comma
> @@ -2661,6 +2663,12 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
>         }
>         *to = '\0';
>         return 0;
> +free_opt:
> +       if (*mnt_opts) {
> +               selinux_free_mnt_opts(*mnt_opts);
> +               *mnt_opts = NULL;
> +       }
> +       return ret;
>  }
>
>  static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)

-- 
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_sb_eat_lsm_opts()

[Paul Moore]

On Mon, Jun 3, 2019 at 3:27 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Sat, Jun 1, 2019 at 4:15 AM Gen Zhang <blackgod016574@gmail.com> wrote:
> > In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> > returns NULL when fails. So 'arg' should be checked. And 'mnt_opts'
> > should be freed when error.
> >
> > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> > Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
> > Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
> > ---
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 3ec702c..f329fc0 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2616,6 +2616,7 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
> >         char *from = options;
> >         char *to = options;
> >         bool first = true;
> > +       int ret;
>
> I'd suggest just moving the declaration of 'rc' here and simply reuse
> that variable. Otherwise the patch looks good to me.

Agreed.  Creating "ret" only makes the patch larger and doesn't add any value.

I try to avoid making broad statements, but if you are unsure about
which approach to take when fixing a problem, start with the smallest
patch you can write.  Even if it turns out not to be the "best"
solution upstream, it will be easier to review, discuss, and
potentially port to other/older kernels.

> >
> >         while (1) {
> >                 int len = opt_len(from);
> > @@ -2635,15 +2636,16 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
> >                                                 *q++ = c;
> >                                 }
> >                                 arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
> > +                               if (!arg) {
> > +                                       ret = -ENOMEM;
> > +                                       goto free_opt;
> > +                               }
> >                         }
> >                         rc = selinux_add_opt(token, arg, mnt_opts);
> >                         if (unlikely(rc)) {
> > +                               ret = rc;
> >                                 kfree(arg);
> > -                               if (*mnt_opts) {
> > -                                       selinux_free_mnt_opts(*mnt_opts);
> > -                                       *mnt_opts = NULL;
> > -                               }
> > -                               return rc;
> > +                               goto free_opt;
> >                         }
> >                 } else {
> >                         if (!first) {   // copy with preceding comma
> > @@ -2661,6 +2663,12 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
> >         }
> >         *to = '\0';
> >         return 0;
> > +free_opt:
> > +       if (*mnt_opts) {
> > +               selinux_free_mnt_opts(*mnt_opts);
> > +               *mnt_opts = NULL;
> > +       }
> > +       return ret;
> >  }
> >
> >  static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)
>
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Software Engineer, Security Technologies
> Red Hat, Inc.

-- 
paul moore
www.paul-moore.com

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

[Paul Moore]

On Fri, May 31, 2019 at 10:45 PM Gen Zhang <blackgod016574@gmail.com> wrote:
> On Sat, Jun 01, 2019 at 03:25:27AM +0100, Al Viro wrote:
> > On Sat, Jun 01, 2019 at 10:15:26AM +0800, Gen Zhang wrote:
> > > In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> > > returns NULL when fails. So 'arg' should be checked. And 'mnt_opts'
> > > should be freed when error.
> >
> > What's the latter one for?  On failure we'll get to put_fs_context()
> > pretty soon, so
> >         security_free_mnt_opts(&fc->security);
> > will be called just fine.  Leaving it allocated on failure is fine...
> Paul Moore <paul@paul-moore.com> wrote:
> >It seems like we should also check for, and potentially free *mnt_opts
> >as the selinux_add_opt() error handling does just below this change,
> >yes?  If that is the case we might want to move that error handling
> >code to the bottom of the function and jump there on error.
> I am not familiar with this part. So could you please show the function
> call sequence?

I'm not sure I understand your question above, but I did review your
latest patch and agree with Ondrej's comment regarding the ret/rc
variable.  If you make that change I think we can merge this into
selinux/stable-5.2.

-- 
paul moore
www.paul-moore.com

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

[Paul Moore]

On Mon, Jun 3, 2019 at 3:23 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Sat, Jun 1, 2019 at 4:15 AM Gen Zhang <blackgod016574@gmail.com> wrote:
> > In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> > returns NULL when fails. So 'arg' should be checked. And 'mnt_opts'
> > should be freed when error.
> >
> > Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> > Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> It looks like you're new to the kernel development community, so let
> me give you a bit of friendly advice for the future :)
>
> You don't need to repost the patch when people give you
> Acked-by/Reviewed-by/Tested-by (unless there is a different reason to
> respin/repost the patches). The maintainer goes over the replies when
> applying the final patch and adds Acked-by/Reviewed-by/... on his/her
> own.
>
> If you *do* need to respin a path for which you have received A/R/T,
> then you need to distinguish between two cases:
> 1. Only trivial changes to the patch (only fixed typos, edited commit
> message, removed empty line, etc. - for example, v1 -> v2 of this
> patch falls into this category) - in this case you can collect the
> A/R/T yourself and add them to the new version. This saves the
> maintainer and the reviewers from redundant work, since the patch is
> still semantically the same and the A/R/T from the last version still
> apply.
> 2. Non-trivial changes to the patch (as is the case for this patch) -
> in this case your patch needs to be reviewed again and you should
> disregard all A/R/T from the previous version. You can easily piss
> someone off if you add their Reviewed-by to a patch they haven't
> actually reviewed, so be careful ;-)

I want to stress Ondrej's last point.  Carrying over an
Acked-by/Reviewed-by/Tested-by tag if you make anything more than the
most trivial change in a patch is *very* bad, and will likely result
in a loss of trust between you and the maintainer.  If you are unsure,
drop the A/R/T tag, there is *much* less harm in asking someone to
re-review a patch than falsely tagging a patch as reviewed by someone
when you have made substantial changes.

I suspect you may have already read the
Documentation/process/submitting-patches.rst file, but if you haven't
it is worth reading.  It covers many of the things that are discussed
elsewhere.

If you aren't already, you should get in the habit of doing the
following for each patch you post to the mailing list:
1. Make sure it compiles cleanly, or at least doesn't introduce any
new compiler warnings/errors.
2. Run ./scripts/checkpatch.pl and fix as many problems as you can; a
patch can still be accepted with checkpatch warnings/errors (and some
maintainers might dislike some of checkpatch's decisions), but it
helps a lot if you can fix all those.
3. At the very least make sure your kernel changes boot, if you can,
run the associated subsystem's test (if they have one) to verify that
there are no regressions (the SELinux kernel test suite is here:
https://github.com/SELinuxProject/selinux-testsuite)

Lastly, when in doubt, you can always ask the mailing list; the
SELinux list is a pretty friendly place :)

-- 
paul moore
www.paul-moore.com