On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
> Lakshmi Ramasubramanian <nra...@linux.microsoft.com> writes:
> > On 4/16/21 2:05 AM, Michael Ellerman wrote:
> >
> >> Daniel Axtens <d...@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;
}
goto out;
But we should also fix the uninitialized variable of "elf_info" if
kexec_build_elf_info() fails.
> +
> +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
