Re: [PATCH 01/16] efi/libstub: Fix error unwind freeing fdt in allocate_new_fdt_and_exit_boot()

From: Jason Gunthorpe <jgg@nvidia.com>
To: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
Cc: Alexandre Ghiti <alex@ghiti.fr>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Ard Biesheuvel <ardb@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Jonathan Corbet <corbet@lwn.net>, David Sterba <dsterba@suse.com>,
	Ilias Apalodimas <ilias.apalodimas@linaro.org>,
	linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-doc@vger.kernel.org, linux-efi@vger.kernel.org,
	linux-riscv@lists.infradead.org,
	Mark Rutland <mark.rutland@arm.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Paul Walmsley <pjw@kernel.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	Simon Glass <sjg@chromium.org>,
	Shuah Khan <skhan@linuxfoundation.org>,
	Nick Terrell <terrelln@fb.com>, Will Deacon <will@kernel.org>,
	Alexandre Ghiti <alexghiti@rivosinc.com>,
	Conor Dooley <conor.dooley@microchip.com>,
	linux-integrity@vger.kernel.org,
	Palmer Dabbelt <palmer@rivosinc.com>,
	patches@lists.linux.dev,
	Ross Philipson <ross.philipson@gmail.com>,
	Sami Tolvanen <samitolvanen@google.com>,
	Song Shuai <songshuaishuai@tinylab.org>
Subject: Re: [PATCH 01/16] efi/libstub: Fix error unwind freeing fdt in allocate_new_fdt_and_exit_boot()
Date: Thu, 24 Sep 2026 20:44:57 -0300	[thread overview]
Message-ID: <20260924234457.GD9354@nvidia.com> (raw)
In-Reply-To: <20260924154227.00001cbe@oss.qualcomm.com>

On Thu, Sep 24, 2026 at 03:42:27PM -0700, Jonathan Cameron wrote:
> On Thu, 24 Sep 2026 10:53:04 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > Sashiko says fdt_addr can point to either an allocated fdt or the fdt from
> > get_fdt() which is memory owned by FW.
> > 
> > Only the allocated fdt should be freed on the error unwind path. Use a
> > dedicated variable for the allocation's size so that the free does not get
> > confused.
> > 
> > Fixes: 4fc8e738ff3e ("efi: libstub: remove DT dependency from generic stub")
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >  drivers/firmware/efi/libstub/fdt.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> > index 23b3543d3041b0..5b2dd709d7b151 100644
> > --- a/drivers/firmware/efi/libstub/fdt.c
> > +++ b/drivers/firmware/efi/libstub/fdt.c
> > @@ -229,6 +229,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
> >  	u32 desc_ver;
> >  	efi_status_t status;
> >  	struct exit_boot_struct priv;
> > +	unsigned long fdt_size_allocated = 0;
> >  	unsigned long fdt_addr = 0;
> >  	unsigned long fdt_size = 0;
> >  
> > @@ -257,6 +258,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
> >  			efi_err("Failed to load device tree!\n");
> >  			goto fail;
> >  		}
> > +		fdt_size_allocated = fdt_size;
> Hmm. I argued with myself for a while on this.  Which one of fdt_size and fdt_size_allocate
> is the appropriate one to pass to the call?  In the end I didn't get a good answer so
> oh I guess this is as good as the other way around.

Huh. Functionally it does not matter, but looking at it again I will
change it to pass fdt_size_allocated since that chases down to the
actual allocation. Makes more sense to me like that.

@@ -253,13 +253,13 @@ efi_status_t allocate_new_fdt_and_exit_boot(void *handle,
                if (strstr(cmdline_ptr, "dtb="))
                        efi_err("Ignoring DTB from command line.\n");
        } else {
-               status = efi_load_dtb(image, &fdt_addr, &fdt_size);
+               status = efi_load_dtb(image, &fdt_addr, &fdt_size_allocated);
 
                if (status != EFI_SUCCESS && status != EFI_NOT_READY) {
                        efi_err("Failed to load device tree!\n");
                        goto fail;
                }
-               fdt_size_allocated = fdt_size;
+               fdt_size = fdt_size_allocated;
        }

Thanks,
Jason
