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

[Lakshmi Ramasubramanian <nramas@linux.microsoft.com>]

On 4/19/21 10:00 PM, Dan Carpenter wrote:
> On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
>> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
>>> On 4/16/21 2:05 AM, Michael Ellerman wrote:
>>>
>>>> Daniel Axtens <dja@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;
> 
> This will leak.  It would need to be something like:
> 
> 	if (ret) {
> 		pr_err("Error setting up the purgatory.\n");
> 		goto out_free_fdt;
> 	}
Once "fdt" buffer is successfully passed to kexec_add_buffer() it cannot 
be freed here - it will be freed when the kexec cleanup function is called.

> 
> 	goto out;
> 
> But we should also fix the uninitialized variable of "elf_info" if
> kexec_build_elf_info() fails.

kexec_build_elf_info() frees elf_info and zeroes it in error paths, 
except when elf_read_ehdr() fails. So, I think it is better to 
initialize the local variable "elf_info" before calling 
kexec_build_elf_info().

	memset(&elf_info, 0, sizeof(elf_info));

thanks,
  -lakshmi

> 
>> +
>> +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;
>>   }
> 
> regards,
> dan carpenter
>