Re: [PATCH v2] selinux: Adjust implementation of security_get_bools()

[Paul Moore]

On Mon, Mar 27, 2023 at 3:00 AM Markus Elfring <Markus.Elfring@xxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  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.

> 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