linux-kernel - Re: [PATCH 3/3] selinux: remove duplicate cond

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 1 Feb 2022 15:09:53 -0500
From:   Paul Moore <paul@...l-moore.com>
To:     vbendel@...hat.com
Cc:     stephen.smalley.work@...il.com, eparis@...isplace.org,
        omosnace@...hat.com, selinux@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] selinux: remove duplicate cond_list clean up calls

On Fri, Jan 28, 2022 at 3:29 PM <vbendel@...hat.com> wrote:
> From: Vratislav Bendel <vbendel@...hat.com>
>
> On error path from cond_read_list() and duplicate_policydb_cond_list()
> the *_destroy() functions get called a second time in caller functions.
> Remove the first calls and let the callers clean it.
>
> Suggested-by: Ondrej Mosnacek <omosnace@...hat.com>
> Signed-off-by: Vratislav Bendel <vbendel@...hat.com>
> ---
>  security/selinux/ss/conditional.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index 8bc16ad3af9e..c333daaeceab 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -432,19 +432,16 @@ int cond_read_list(struct policydb *p, void *fp)
>
>         rc = avtab_alloc(&(p->te_cond_avtab), p->te_avtab.nel);
>         if (rc)
> -               goto err;
> +               return rc;
>
>         p->cond_list_len = len;
>
>         for (i = 0; i < len; i++) {
>                 rc = cond_read_node(p, &p->cond_list[i], fp);
>                 if (rc)
> -                       goto err;
> +                       return rc;
>         }
>         return 0;
> -err:
> -       cond_list_destroy(p);
> -       return rc;
>  }

I tend to prefer functions that cleanup their own allocations on
error.  It makes it easier and quicker to reason about a function's
error handling.  I recognize in this case it may mean multiple calls
to cond_list_destroy(), but that should be safe (considering the
previous patches in this series), and we are on the error path anyway
so I'm not as worried about a few extra instructions.

-- 
paul-moore.com