[PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()

[Lakshmi Ramasubramanian]

On 4/19/21 4:30 PM, Michael Ellerman wrote:
> Lakshmi Ramasubramanian <nramas at linux.microsoft.com> writes:
>> On 4/16/21 2:05 AM, Michael Ellerman wrote:
>>
>>> Daniel Axtens <dja at axtens.net> writes:
>>>>> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
>>>>>
>>>>> Sorry - missed copying device-tree and powerpc mailing lists.
>>>>>
>>>>>> There are a few "goto out;" statements before the local variable "fdt"
>>>>>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
>>>>>> elf64_load(). This will result in an uninitialized "fdt" being passed
>>>>>> to kvfree() in this function if there is an error before the call to
>>>>>> of_kexec_alloc_and_setup_fdt().
>>>>>>
>>>>>> Initialize the local variable "fdt" to NULL.
>>>>>>
>>>> I'm a huge fan of initialising local variables! But I'm struggling to
>>>> find the code path that will lead to an uninit fdt being returned...
>>>>
>>>> The out label reads in part:
>>>>
>>>> 	/* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
>>>> 	return ret ? ERR_PTR(ret) : fdt;
>>>>
>>>> As far as I can tell, any time we get a non-zero ret, we're going to
>>>> return an error pointer rather than the uninitialised value...
>>
>> As Dan pointed out, the new code is in linux-next.
>>
>> I have copied the new one below - the function doesn't return fdt, but
>> instead sets it in the arch specific field (please see the link to the
>> updated elf_64.c below).
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
>>
>>>>
>>>> (btw, it does look like we might leak fdt if we have an error after we
>>>> successfully kmalloc it.)
>>>>
>>>> Am I missing something? Can you link to the report for the kernel test
>>>> robot or from Dan?
>>
>> /*
>>            * Once FDT buffer has been successfully passed to
>> kexec_add_buffer(),
>>            * the FDT buffer address is saved in image->arch.fdt. In that
>> case,
>>            * the memory cannot be freed here in case of any other error.
>>            */
>>           if (ret && !image->arch.fdt)
>>                   kvfree(fdt);
>>
>>           return ret ? ERR_PTR(ret) : NULL;
>>
>> In case of an error, the memory allocated for fdt is freed unless it has
>> already been passed to kexec_add_buffer().
> 
> It feels like the root of the problem is that the kvfree of fdt is in
> the wrong place. It's only allocated later in the function, so the error
> path should reflect that. Something like the patch below.
> 
> cheers
> 
> 
> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> index 5a569bb51349..02662e72c53d 100644
> --- a/arch/powerpc/kexec/elf_64.c
> +++ b/arch/powerpc/kexec/elf_64.c
> @@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>   	ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
>   				  initrd_len, cmdline);
>   	if (ret)
> -		goto out;
> +		goto out_free_fdt;
>   
>   	fdt_pack(fdt);
>   
> @@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>   	kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>   	ret = kexec_add_buffer(&kbuf);
>   	if (ret)
> -		goto out;
> +		goto out_free_fdt;
>   
>   	/* FDT will be freed in arch_kimage_file_post_load_cleanup */
>   	image->arch.fdt = fdt;
> @@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>   	if (ret)
>   		pr_err("Error setting up the purgatory.\n");
>   
> +	goto out;
> +
> +out_free_fdt:
> +	kvfree(fdt);
>   out:
>   	kfree(modified_cmdline);
>   	kexec_free_elf_info(&elf_info);
>   
> -	/*
> -	 * Once FDT buffer has been successfully passed to kexec_add_buffer(),
> -	 * the FDT buffer address is saved in image->arch.fdt. In that case,
> -	 * the memory cannot be freed here in case of any other error.
> -	 */
> -	if (ret && !image->arch.fdt)
> -		kvfree(fdt);
> -
>   	return ret ? ERR_PTR(ret) : NULL;
>   }
>   
> 

This looks good to me. Thanks Michael.

I'll post the updated patch shortly.

  -lakshmi