Re: [PATCH 12/14] powerpc/rtas: Close theoretical memory leak

[Laurent Dufour <ldufour@linux.ibm.com>]

On 08/03/2022, 14:50:45, Nicholas Piggin wrote:
> If buff_copy allocation failed then there was an error and the errbuf
> allocation succeeded, it will not be logged or freed.

Good catch!

Since you're dealing with the error log buffer allocation/free, I think it
would be better to not make this allocation in __fetch_rtas_last_error()
and to rely on the caller to allocate it before taking the rtas lock.

This way, the allocation is done without holding the rtas lock, as done in
rtas().

This would simplify __fetch_rtas_last_error() and the caller logic to free
the buffer too.

Laurent.

> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/kernel/rtas.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index e047793cbb80..1fc22138e3ab 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -1239,9 +1239,10 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
>  
>  	unlock_rtas(flags);
>  
> -	if (buff_copy) {
> -		if (errbuf)
> -			log_error(errbuf, ERR_TYPE_RTAS_LOG, 0);
> +	if (errbuf) {
> +		log_error(errbuf, ERR_TYPE_RTAS_LOG, 0);
> +		kfree(errbuf);
> +	} else if (buff_copy) {
>  		kfree(buff_copy);
>  	}
>