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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > > 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