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

[Rob Herring <robh@kernel.org>]

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.

Rob