Message-ID: <CAHC9VhREfdgiCji=uEeCrc4w1kPGfnWGKnJuUYKXwTApdneSjQ@mail.gmail.com>
Date:   Mon, 27 Mar 2023 18:08:54 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Markus Elfring <Markus.Elfring@....de>
Cc:     kernel-janitors@...r.kernel.org, selinux@...r.kernel.org,
        Christian Göttsche <cgzones@...glemail.com>,
        Eric Paris <eparis@...isplace.org>,
        Michal Orzel <michalorzel.eng@...il.com>,
        Ondrej Mosnacek <omosnace@...hat.com>,
        Ruiqi Gong <gongruiqi1@...wei.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Xiu Jianfeng <xiujianfeng@...wei.com>, cocci@...ia.fr,
        LKML <linux-kernel@...r.kernel.org>,
        Ruiqi Gong <ruiqi.gong@...com>
Subject: Re: [PATCH v2] selinux: Adjust implementation of security_get_bools()

On Mon, Mar 27, 2023 at 5:37 PM Paul Moore <paul@...l-moore.com> wrote:
> On Mon, Mar 27, 2023 at 3:00 AM Markus Elfring <Markus.Elfring@....de> wrote:
> > Date: Mon, 27 Mar 2023 08:50:56 +0200
> >
> > The label “err” was used to jump to another pointer check despite of
> > the detail in the implementation of the function “security_get_bools”
> > that it was determined already that a corresponding variable contained
> > a null pointer because of a failed memory allocation.
> >
> > Thus perform the following adjustments:
> >
> > 1. Convert the statement “policydb = &policy->policydb;” into
> >    a variable initialisation.
> >
> > 2. Replace the statement “goto out;” by “return -ENOMEM;”.
> >
> > 3. Return zero directly at two places.
> >
> > 4. Omit the variable “rc”.
> >
> > 5. Use more appropriate labels instead.
> >
> > 6. Reorder the assignment targets for two kcalloc() calls.
> >
> > 7. Reorder jump targets at the end.
> >
> > 8. Assign a value element only after a name assignment succeeded.
> >
> > 9. Delete an extra pointer check which became unnecessary
> >    with this refactoring.
> >
> >
> > This issue was detected by using the Coccinelle software.
> >
> > Signed-off-by: Markus Elfring <elfring@...rs.sourceforge.net>
> > ---
> >  security/selinux/ss/services.c | 52 ++++++++++++++--------------------
> >  1 file changed, 22 insertions(+), 30 deletions(-)
>
> Hmm, for some odd reason I don't see this patch in the SELinux mailing
> list archive or the patchwork; replying here without comment (that
> will come later) to make sure this hits the SELinux list.

For some reason the generated diff below is pretty messy, so I'm just
going to put some of my comments here:

Given the fairly extensive refactoring here, and the frequency with
which @len, @names, and @values are used in the function, I might
simply create local variables for each and only assign them to the
parameter pointers at the end when everything completes successfully
(you could still reset @len at the start if you wanted).  If nothing
else it will make the function easier to read, and I think it will
simplify the code a bit too.

I would probably also keep the combined @names/@...ues cleanup under
one jump label; this function isn't complicated enough to warrant that
many jump labels for error conditions.

> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index f14d1ffe54c5..702282954bf9 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -2964,53 +2964,45 @@ int security_fs_use(struct super_block *sb)
> >  int security_get_bools(struct selinux_policy *policy,
> >                        u32 *len, char ***names, int **values)
> >  {
> > -       struct policydb *policydb;
> > +       struct policydb *policydb = &policy->policydb;
> >         u32 i;
> > -       int rc;
> > -
> > -       policydb = &policy->policydb;
> >
> >         *names = NULL;
> >         *values = NULL;
> > -
> > -       rc = 0;
> >         *len = policydb->p_bools.nprim;
> >         if (!*len)
> > -               goto out;
> > -
> > -       rc = -ENOMEM;
> > -       *names = kcalloc(*len, sizeof(char *), GFP_ATOMIC);
> > -       if (!*names)
> > -               goto err;
> > +               return 0;
> >
> > -       rc = -ENOMEM;
> >         *values = kcalloc(*len, sizeof(int), GFP_ATOMIC);
> >         if (!*values)
> > -               goto err;
> > +               goto reset_len;
> >
> > -       for (i = 0; i < *len; i++) {
> > -               (*values)[i] = policydb->bool_val_to_struct[i]->state;
> > +       *names = kcalloc(*len, sizeof(char *), GFP_ATOMIC);
> > +       if (!*names)
> > +               goto free_values;
> >
> > -               rc = -ENOMEM;
> > +       for (i = 0; i < *len; i++) {
> >                 (*names)[i] = kstrdup(sym_name(policydb, SYM_BOOLS, i),
> >                                       GFP_ATOMIC);
> >                 if (!(*names)[i])
> > -                       goto err;
> > -       }
> > -       rc = 0;
> > -out:
> > -       return rc;
> > -err:
> > -       if (*names) {
> > -               for (i = 0; i < *len; i++)
> > -                       kfree((*names)[i]);
> > -               kfree(*names);
> > +                       goto free_names;
> > +
> > +               (*values)[i] = policydb->bool_val_to_struct[i]->state;
> >         }
> > -       kfree(*values);
> > -       *len = 0;
> > +       return 0;
> > +
> > +free_names:
> > +       for (i = 0; i < *len; i++)
> > +               kfree((*names)[i]);
> > +
> > +       kfree(*names);
> >         *names = NULL;
> > +free_values:
> > +       kfree(*values);
> >         *values = NULL;
> > -       goto out;
> > +reset_len:
> > +       *len = 0;
> > +       return -ENOMEM;
> >  }
> >
> >
> > --
> > 2.40.0
>
> --
> paul-moore.com



-- 
paul-moore.com

Powered by blists - more mailing lists