Re: [PATCH -next] selinux: Fix memleak in security_read_policy

[Ondrej Mosnacek]

On Wed, Jun 15, 2022 at 4:03 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Tue, Jun 14, 2022 at 9:25 AM Xiu Jianfeng <xiujianfeng@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  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.