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

[Rob Herring <robh@kernel.org>]

On Tue, Apr 20, 2021 at 10:04 AM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> 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?

Yes.

> > 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);

I thought kexec_build_elf_info() can return an error and allocated
memory, so that would leak memory.

Rob