* [PATCH v3] selinux: lsm: fix a missing-check bug in selinux_add_mnt_opt( )
@ 2019-06-06 9:23 Gen Zhang
2019-06-07 8:39 ` Ondrej Mosnacek
0 siblings, 1 reply; 5+ messages in thread
From: Gen Zhang @ 2019-06-06 9:23 UTC (permalink / raw)
To: paul, sds, eparis; +Cc: selinux, linux-kernel
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;
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] selinux: lsm: fix a missing-check bug in selinux_add_mnt_opt( )
2019-06-06 9:23 [PATCH v3] selinux: lsm: fix a missing-check bug in selinux_add_mnt_opt( ) Gen Zhang
@ 2019-06-07 8:39 ` Ondrej Mosnacek
2019-06-07 12:11 ` Gen Zhang
0 siblings, 1 reply; 5+ messages in thread
From: Ondrej Mosnacek @ 2019-06-07 8:39 UTC (permalink / raw)
To: Gen Zhang
Cc: Paul Moore, Stephen Smalley, Eric Paris, selinux,
Linux kernel mailing list
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] selinux: lsm: fix a missing-check bug in selinux_add_mnt_opt( )
2019-06-07 8:39 ` Ondrej Mosnacek
@ 2019-06-07 12:11 ` Gen Zhang
2019-06-10 19:31 ` Paul Moore
0 siblings, 1 reply; 5+ messages in thread
From: Gen Zhang @ 2019-06-07 12:11 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: Paul Moore, Stephen Smalley, Eric Paris, selinux,
Linux kernel mailing list
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] selinux: lsm: fix a missing-check bug in selinux_add_mnt_opt( )
2019-06-07 12:11 ` Gen Zhang
@ 2019-06-10 19:31 ` Paul Moore
2019-06-11 3:04 ` Gen Zhang
0 siblings, 1 reply; 5+ messages in thread
From: Paul Moore @ 2019-06-10 19:31 UTC (permalink / raw)
To: Gen Zhang
Cc: Ondrej Mosnacek, Stephen Smalley, Eric Paris, selinux,
Linux kernel mailing list
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] selinux: lsm: fix a missing-check bug in selinux_add_mnt_opt( )
2019-06-10 19:31 ` Paul Moore
@ 2019-06-11 3:04 ` Gen Zhang
0 siblings, 0 replies; 5+ messages in thread
From: Gen Zhang @ 2019-06-11 3:04 UTC (permalink / raw)
To: Paul Moore
Cc: Ondrej Mosnacek, Stephen Smalley, Eric Paris, selinux,
Linux kernel mailing list
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-11 3:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-06 9:23 [PATCH v3] selinux: lsm: fix a missing-check bug in selinux_add_mnt_opt( ) Gen Zhang
2019-06-07 8:39 ` Ondrej Mosnacek
2019-06-07 12:11 ` Gen Zhang
2019-06-10 19:31 ` Paul Moore
2019-06-11 3:04 ` Gen Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox