[PATCH -next] selinux: Fix memleak in security_read_policy

[PATCH -next] selinux: Fix memleak in security_read_policy

[Xiu Jianfeng]

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@huawei.com>
---
 security/selinux/ss/services.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 69b2734311a6..78afda6a36b8 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -4018,6 +4018,7 @@ static int __security_read_policy(struct selinux_policy *policy,
 int security_read_policy(struct selinux_state *state,
 			 void **data, size_t *len)
 {
+	int err;
 	struct selinux_policy *policy;
 
 	policy = rcu_dereference_protected(
@@ -4030,7 +4031,13 @@ int security_read_policy(struct selinux_state *state,
 	if (!*data)
 		return -ENOMEM;
 
-	return __security_read_policy(policy, *data, len);
+	err = __security_read_policy(policy, *data, len);
+	if (err) {
+		vfree(*data);
+		*data = NULL;
+		*len = 0;
+	}
+	return err;
 }
 
 /**
-- 
2.17.1

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

[Paul Moore]

On Tue, Jun 14, 2022 at 9:25 AM Xiu Jianfeng <xiujianfeng@huawei.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@huawei.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.

-- 
paul-moore.com

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

[Ondrej Mosnacek]

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

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

[Paul Moore]

On Wed, Jun 15, 2022 at 6:04 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Jun 15, 2022 at 4:03 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Jun 14, 2022 at 9:25 AM Xiu Jianfeng <xiujianfeng@huawei.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@huawei.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).

Priorities Ondrej, priorities.

Every patch posted to the list has a time and effort cost associated
with it, and between reviewing other more important patches and
working on a proper SCTP/SELinux fix, I simply don't have the cycles
to spend doing the back-and-forth on a patch like this to fix a memory
leak that doesn't exist.  It definitely isn't because I don't think
the code could be improved, it is just that there are only so many
hours in a day and I need to prioritize actual bugs and important new
features that people want merged.

... oh, and I need to reply to the complaints too, that's always the
highlight of my day.

-- 
paul-moore.com