2020-06-10 21:59:41

by Tom Rix

[permalink] [raw]
Subject: [PATCH v2 0/1] selinux: fix double free

From: Tom Rix <[email protected]>

repo: linux-next
tag: next-20200610

Running clang scan-view over linux-next uncovers many problem only a
few are memory related, this one looked like the most serious.

Changes from v1

Fix call to kfree
- kfree(names);
+ kfree(*names);


Tom Rix (1):
selinux: fix double free

security/selinux/ss/services.c | 4 ++++
1 file changed, 4 insertions(+)

--
2.18.1


2020-06-10 22:01:53

by Tom Rix

[permalink] [raw]
Subject: [PATCH v2 1/1] selinux: fix double free

From: Tom Rix <[email protected]>

Clang's static analysis tool reports these double free memory errors.

security/selinux/ss/services.c:2987:4: warning: Attempt to free released memory [unix.Malloc]
kfree(bnames[i]);
^~~~~~~~~~~~~~~~
security/selinux/ss/services.c:2990:2: warning: Attempt to free released memory [unix.Malloc]
kfree(bvalues);
^~~~~~~~~~~~~~

So improve the security_get_bools error handling by freeing these variables
and setting their return pointers to NULL and the return len to 0

Signed-off-by: Tom Rix <[email protected]>
---
security/selinux/ss/services.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 313919bd42f8..ef0afd878bfc 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2888,8 +2888,12 @@ int security_get_bools(struct selinux_state *state,
if (*names) {
for (i = 0; i < *len; i++)
kfree((*names)[i]);
+ kfree(*names);
}
kfree(*values);
+ *len = 0;
+ *names = NULL;
+ *values = NULL;
goto out;
}

--
2.18.1

2020-06-10 22:09:37

by Stephen Smalley

[permalink] [raw]
Subject: Re: [PATCH v2 1/1] selinux: fix double free

On Wed, Jun 10, 2020 at 5:57 PM <[email protected]> wrote:
>
> From: Tom Rix <[email protected]>
>
> Clang's static analysis tool reports these double free memory errors.
>
> security/selinux/ss/services.c:2987:4: warning: Attempt to free released memory [unix.Malloc]
> kfree(bnames[i]);
> ^~~~~~~~~~~~~~~~
> security/selinux/ss/services.c:2990:2: warning: Attempt to free released memory [unix.Malloc]
> kfree(bvalues);
> ^~~~~~~~~~~~~~
>
> So improve the security_get_bools error handling by freeing these variables
> and setting their return pointers to NULL and the return len to 0
>
> Signed-off-by: Tom Rix <[email protected]>

Acked-by: Stephen Smalley <[email protected]>

2020-06-11 02:15:53

by Paul Moore

[permalink] [raw]
Subject: Re: [PATCH v2 1/1] selinux: fix double free

On Wed, Jun 10, 2020 at 5:57 PM <[email protected]> wrote:
> From: Tom Rix <[email protected]>
>
> Clang's static analysis tool reports these double free memory errors.
>
> security/selinux/ss/services.c:2987:4: warning: Attempt to free released memory [unix.Malloc]
> kfree(bnames[i]);
> ^~~~~~~~~~~~~~~~
> security/selinux/ss/services.c:2990:2: warning: Attempt to free released memory [unix.Malloc]
> kfree(bvalues);
> ^~~~~~~~~~~~~~
>
> So improve the security_get_bools error handling by freeing these variables
> and setting their return pointers to NULL and the return len to 0
>
> Signed-off-by: Tom Rix <[email protected]>
> ---
> security/selinux/ss/services.c | 4 ++++
> 1 file changed, 4 insertions(+)

Thanks Tom for the patch and Stephen for the review.

I've marked this for stable and merged it into selinux/stable-5.8.
Considering we are just a few days away from the close of the merge
window and -rc1, I'll probably hold off until next to send this up to
Linus.

> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 313919bd42f8..ef0afd878bfc 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2888,8 +2888,12 @@ int security_get_bools(struct selinux_state *state,
> if (*names) {
> for (i = 0; i < *len; i++)
> kfree((*names)[i]);
> + kfree(*names);
> }
> kfree(*values);
> + *len = 0;
> + *names = NULL;
> + *values = NULL;
> goto out;
> }
>
> --
> 2.18.1

--
paul moore
http://www.paul-moore.com