On Tue, Apr 14, 2020 at 04:23:51PM +0200, Ondrej Mosnacek wrote:
> In [see "Fixes:"] I missed the fact that str_read() may give back an
> allocated pointer even if it returns an error, causing a potential
> memory leak in filename_trans_read_one(). Fix this by making the
> function free the allocated string whenever it returns a non-zero value,
> which also makes its behavior more obvious and prevents repeating the
> same mistake in the future.
>
> Reported-by: coverity-bot <keescook+coverity-bot@xxxxxxxxxxxx>
> Addresses-Coverity-ID: 1461665 ("Resource leaks")
> Fixes: c3a276111ea2 ("selinux: optimize storage of filename transitions")
> Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
-Kees
> ---
> security/selinux/ss/policydb.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 70ecdc78efbd..c21b922e5ebe 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1035,14 +1035,14 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
> if (!str)
> return -ENOMEM;
>
> - /* it's expected the caller should free the str */
> - *strp = str;
> -
> rc = next_entry(str, fp, len);
> - if (rc)
> + if (rc) {
> + kfree(str);
> return rc;
> + }
>
> str[len] = '\0';
> + *strp = str;
> return 0;
> }
>
> --
> 2.25.2
>
--
Kees Cook
