linux-kernel - Re: [PATCH -next] selinux: Fix memleak in security_read

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]

Message-ID: <CAFqZXNswaUiHNZA65GCsgB7GNLm6tc2OFXsQrTWxBTfWviQ8+g@mail.gmail.com>
Date:   Wed, 15 Jun 2022 12:04:32 +0200
From:   Ondrej Mosnacek <omosnace@...hat.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     Xiu Jianfeng <xiujianfeng@...wei.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Eric Paris <eparis@...isplace.org>,
        Christian Göttsche <cgzones@...glemail.com>,
        Austin Kim <austin.kim@....com>, michalorzel.eng@...il.com,
        SElinux list <selinux@...r.kernel.org>,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH -next] selinux: Fix memleak in security_read_policy

On Wed, Jun 15, 2022 at 4:03 AM Paul Moore <paul@...l-moore.com> wrote:
> On Tue, Jun 14, 2022 at 9:25 AM Xiu Jianfeng <xiujianfeng@...wei.com> wrote:
> >
> > In this function, it directly returns the result of __security_read_policy
> > without freeing the allocated memory in *data, cause memory leak issue,
> > so free the memory if __security_read_policy failed.
> >
> > Signed-off-by: Xiu Jianfeng <xiujianfeng@...wei.com>
> > ---
> >  security/selinux/ss/services.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
>
> This is another case where there is not actually a memory leak as the
> only caller of security_read_policy() is sel_open_policy() which will
> free the buffer it passes to security_read_policy() on error.
>
> If you want you could add a comment to security_read_policy()
> indicating that the caller is responsible for freeing the memory.

Can we please not have two almost identical functions with different
cleanup conventions? Please let's either make both functions guarantee
cleanup on error or neither of them (adapting the caller(s) and
comments accordingly).

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.