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
