Re: [PATCH 1/2] powerpc: Free fdt on error in elf64_load()

[Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx>]

On 4/21/21 12:18 AM, Michael Ellerman wrote:
> Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx> writes:
> > 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().
> >
> > If there is any error after fdt is allocated, but before it is
> > saved in the arch specific kimage struct, free the fdt.
> >
> > Signed-off-by: Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx>
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > Suggested-by: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
>
> I basically sent you the diff, so this should probably be:
>
>    Reported-by: kernel test robot <lkp@xxxxxxxxx>
>    Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>    Signed-off-by: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
>    Signed-off-by: Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx>
>
> Otherwise looks good to me, thanks for turning it into a proper patch
> and submitting it.

I will submit the patch again with the above change.
Thanks for reviewing the patch.

Could you please review [PATCH 2/2] as well?

thanks,
 -lakshmi

> 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;
> > +
> > +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;
> >   }
> >   
> > --
> > 2.31.0