Re: [PATCH 1/5] efi_selftest: fix buffer overflow

["Vincent Stehlé" <vincent.stehle@arm.com>]

On Tue, Feb 24, 2026 at 04:45:48PM +0100, Heinrich Schuchardt wrote:

Hi Heinrich,

Thanks for your review and sorry for the late reply.
My answers below.

Best regards,
Vincent.

> On 2/19/26 19:43, Vincent Stehlé wrote:
> > The test of the UEFI LocateHandleBuffer() function clears a returned buffer
> > at some point to reuse it, but there is an error in the size computation,
> > which leads to a buffer overflow; fix it.
> > 
> > Fixes: 927ca890b09f ("efi_selftest: test protocol management")
> > Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Cc: Tom Rini <trini@konsulko.com>
> > ---
> >   lib/efi_selftest/efi_selftest_manageprotocols.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/efi_selftest/efi_selftest_manageprotocols.c b/lib/efi_selftest/efi_selftest_manageprotocols.c
> > index 097b2ae3545..ccffa59095d 100644
> > --- a/lib/efi_selftest/efi_selftest_manageprotocols.c
> > +++ b/lib/efi_selftest/efi_selftest_manageprotocols.c
> > @@ -241,7 +241,7 @@ static int execute(void)
> >   		return EFI_ST_FAILURE;
> >   	}
> >   	/* Clear the buffer, we are reusing it it the next step. */
> > -	boottime->set_mem(buffer, sizeof(efi_handle_t) * buffer_size, 0);
> > +	boottime->set_mem(buffer, sizeof(efi_handle_t) * count, 0);
> >   	/*
> >   	 * Test LocateHandle with ByProtocol
> 
> Hello Vincent,
> 
> Thank you for reviewing the code and pointing to an issue.
> 
> The fix looks incomplete to me:

You are right: I broke down all the fixes into multiple steps, which explains
why the first patch ("fix buffer overflow") is not the full story.

I thought this would ease reviewing but now I wonder if that was maybe a bad
idea. Let me know if I should send a v2 with the series as a single patch if
that helps.

> 
> In line 167 we allocate a buffer with LocateHandleBuffer(). Assigning
> buffer_size in line 173 does not make any sense, as we free the buffer in
> line 185.

This is removed in patch 4 ("fix buffer size and count computations").

> 
> In line 223 we allocate another buffer with LocateHandleBuffer().
> Assigning the value of buffer_size value to count before the invocation
> doesn't make much sense.

This is removed in patch 3 ("remove unnecessary initializations").

> 
> You fix in line 244 looks correct.
> 
> Line 249 sets count to an arbitrary value that is not related to the size of
> the buffer.

This part is reworked in patch 4 to use buffer_size instead.

> 
> Line 260 sets variable buffer_size and buffer_size is again used in line 306
> to set count to an unused value.

This is removed in patch 3.

> 
> We should completely remove variable buffer_size from the function.

The patch series takes a different approach: keep both count and buffer_size
variables, and make sure to use those variables to contain quantities
corresponding to their respective names.

> 
> Best regards
> 
> Heinrich