[PATCH v4] selinux: lsm: fix a missing-check bug in selinux_sb_eat_lsm_o pts()

[PATCH v4] selinux: lsm: fix a missing-check bug in selinux_sb_eat_lsm_o pts()

[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>
Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
---
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3ec702c..13479cd 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2616,10 +2616,11 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
 	char *from = options;
 	char *to = options;
 	bool first = true;
+	int rc;
 
 	while (1) {
 		int len = opt_len(from);
-		int token, rc;
+		int token;
 		char *arg = NULL;
 
 		token = match_opt_prefix(from, len, &arg);
@@ -2635,15 +2636,15 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
 						*q++ = c;
 				}
 				arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
+				if (!arg) {
+					rc = -ENOMEM;
+					goto free_opt;
+				}
 			}
 			rc = selinux_add_opt(token, arg, mnt_opts);
 			if (unlikely(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 +2662,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 rc;
 }
 
 static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)

Re: [PATCH v4] selinux: lsm: fix a missing-check bug in selinux_sb_eat_lsm_o pts()

[Ondrej Mosnacek]

On Thu, Jun 6, 2019 at 10:55 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>
> Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")

My comments about the subject and an empty line before label apply
here as well, but Paul can fix both easily when applying, so:

Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>

> ---
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3ec702c..13479cd 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2616,10 +2616,11 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
>         char *from = options;
>         char *to = options;
>         bool first = true;
> +       int rc;
>
>         while (1) {
>                 int len = opt_len(from);
> -               int token, rc;
> +               int token;
>                 char *arg = NULL;
>
>                 token = match_opt_prefix(from, len, &arg);
> @@ -2635,15 +2636,15 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
>                                                 *q++ = c;
>                                 }
>                                 arg = kmemdup_nul(arg, q - arg, GFP_KERNEL);
> +                               if (!arg) {
> +                                       rc = -ENOMEM;
> +                                       goto free_opt;
> +                               }
>                         }
>                         rc = selinux_add_opt(token, arg, mnt_opts);
>                         if (unlikely(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 +2662,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 rc;
>  }
>
>  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 v4] selinux: lsm: fix a missing-check bug in selinux_sb_eat_lsm_o pts()

[Paul Moore]

On Fri, Jun 7, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Thu, Jun 6, 2019 at 10:55 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>
> > Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
>
> My comments about the subject and an empty line before label apply
> here as well, but Paul can fix both easily when applying ...

Since we've been discussing general best practices for submitting
patches in this thread (and the other related thread), I wanted to
(re)clarify my thoughts around maintainers fixing patches when merging
them upstream.

When in doubt, do not ever rely on the upstream maintainer fixing your
patch while merging it, and if problems do arise during review, it is
best to not ask the maintainer to fix them for you, but for you to fix
them instead (you are the patch author after all!).  Similarly, making
comments along the lines of "X can fix both easily when applying", is
also a bad thing to say when reviewing patches.  It's the patch
author's responsibility to fix the patch by address review comments,
not the maintainer.  I'll typically let you know if you don't need to
rework a patch(set).

That said, there are times when the maintainer will change the patch
during merging, most of which are due to resolving merge
conflicts/fuzz with changes already in the tree (that *is* the
maintainer's responsibility).  Speaking for myself, sometimes I will
also make some minor changes if the patch author is away, or
unreliable, or if there is a hard deadline near and I'm worried that
the updated patch might not be ready in time.  I'll also sometimes
make the changes directly if the patch is holding up a larger, more
important patch(set), but that is really rare.  I'm sure I've made
changes for other reasons in the past, and I'm sure I'll make changes
for other reasons in the future, but hopefully this will give you a
better idea of how the process works :)

-- 
paul moore
www.paul-moore.com

Re: [PATCH v4] selinux: lsm: fix a missing-check bug in selinux_sb_eat_lsm_o pts()

[Gen Zhang]

On Mon, Jun 10, 2019 at 04:20:28PM -0400, Paul Moore wrote:
> On Fri, Jun 7, 2019 at 4:41 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > On Thu, Jun 6, 2019 at 10:55 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>
> > > Fixes: 99dbbb593fe6 ("selinux: rewrite selinux_sb_eat_lsm_opts()")
> >
> > My comments about the subject and an empty line before label apply
> > here as well, but Paul can fix both easily when applying ...
> 
> Since we've been discussing general best practices for submitting
> patches in this thread (and the other related thread), I wanted to
> (re)clarify my thoughts around maintainers fixing patches when merging
> them upstream.
> 
> When in doubt, do not ever rely on the upstream maintainer fixing your
> patch while merging it, and if problems do arise during review, it is
> best to not ask the maintainer to fix them for you, but for you to fix
> them instead (you are the patch author after all!).  Similarly, making
> comments along the lines of "X can fix both easily when applying", is
> also a bad thing to say when reviewing patches.  It's the patch
> author's responsibility to fix the patch by address review comments,
> not the maintainer.  I'll typically let you know if you don't need to
> rework a patch(set).
> 
> That said, there are times when the maintainer will change the patch
> during merging, most of which are due to resolving merge
> conflicts/fuzz with changes already in the tree (that *is* the
> maintainer's responsibility).  Speaking for myself, sometimes I will
> also make some minor changes if the patch author is away, or
> unreliable, or if there is a hard deadline near and I'm worried that
> the updated patch might not be ready in time.  I'll also sometimes
> make the changes directly if the patch is holding up a larger, more
> important patch(set), but that is really rare.  I'm sure I've made
> changes for other reasons in the past, and I'm sure I'll make changes
> for other reasons in the future, but hopefully this will give you a
> better idea of how the process works :)
> 
> -- 
> paul moore
> www.paul-moore.com
Thanks for your comments. I will resend a patch after revising.

Thanks
Gen