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

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

On 4/20/21 7:42 AM, Lakshmi Ramasubramanian wrote:
> On 4/20/21 6:06 AM, Rob Herring wrote:
>> On Tue, Apr 20, 2021 at 12:20 AM Lakshmi Ramasubramanian
>> <nramas@linux.microsoft.com> wrote:
>>>
>>> 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.
>>
>> That may be the case currently, but really if a function returns an
>> error it should have undone anything it did like memory allocations. I
>> don't think you should do that to fix this issue, but it would be a
>> good clean-up.
>>
> 
> I agree - in case of an error the function should do a proper clean-up.
> Just to be clear - for now, I will leave this as is. Correct?
> 
> In my patch, I will do the following changes:
> 
>   => Free "fdt" when possible (as Michael had suggested in his patch)
>   => Zero out "elf_info" struct at the start of the function.
> 

Instead of zeroing out "elf_info", I think it would be better to return 
an error immediately, instead of the "goto out;", if 
kexec_build_elf_info() fails.

    ret = kexec_build_elf_info(kernel_buf, kernel_len, &ehdr, &elf_info);
    if (ret)
      return ERR_PTR(ret);

  -lakshmi