Re: [PATCH] selinux: free str on error in str_read()

[Kees Cook <keescook@chromium.org>]

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@chromium.org>
> Addresses-Coverity-ID: 1461665 ("Resource leaks")
> Fixes: c3a276111ea2 ("selinux: optimize storage of filename transitions")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-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