Re: [PATCH] fwu: fix read buffer size

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

On Thu, Sep 17, 2026 at 06:26:37PM +0300, Ilias Apalodimas wrote:
> Hi Vincent,
> 
> On Tue Sep 15, 2026 at 5:29 PM EEST, Vincent Stehlé wrote:
> > When the trial_counter_read() function reads its counter from the
> > TrialStateCtr UEFI Variable, it erroneously computes the return buffer
> > size to be the size of a pointer instead of a u16.
> >
> > As the UEFI Variable is always created by trial_counter_update() with
> > the right size before any read attempt, overflow cannot happen in
> > practice, but fix the size computation for correctness.
> 
> The fix is correct, but the commit message if a bit off. The creation
> is not the reason the read works. It works because the current sizeof is
> always > of the new updated sizeof so the GetVariable calls has enough size
> to fix the output.

Hi Ilias,

Thank you for reviewing.

Is that not the other way around?
In the patch the call to GetVariable() was truncated but here are the
details:

  status = efi_get_variable_int(
               u"TrialStateCtr",
               &efi_global_variable_guid,
               NULL,
               &var_size,                  // data_size
               trial_state_ctr,            // data
               NULL);

We are telling GetVariable() to read as much as 4 to 8 bytes
(sizeof(trial_state_ctr), the size of a pointer, in var_size), into a 2
bytes buffer (the u16 pointed to by trial_state_ctr).
I think what saves us here is that we are sure the variable contains
only 2 bytes, which will never overflow.

Best regards,
Vincent.

> 
> 
> Cheers
> /Ilias
> >
> > Fixes: 7e9814cc6c1d ("FWU: Add boot time checks as highlighted by the FWU specification")
> > Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> > Cc: Sughosh Ganu <sughosh.ganu@arm.com>
> > Cc: Kory Maincent <kory.maincent@bootlin.com>
> > Cc: Tom Rini <trini@konsulko.com>
> > ---
> > Hi,
> >
> > This has been tested on the sandbox, with CONFIG_FWU_MULTI_BANK_UPDATE=y
> > & CONFIG_FWU_MDATA_V2=y, and with a modified fwu_boottime_checks()
> > function doing essentially the following sequence:
> >
> >   efi_init_obj_list();
> >   trial_counter_update();
> >   trial_counter_read();
> > ---
> >  lib/fwu_updates/fwu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> > index e9bb1b4fc65..24a8f1ce5b2 100644
> > --- a/lib/fwu_updates/fwu.c
> > +++ b/lib/fwu_updates/fwu.c
> > @@ -82,7 +82,7 @@ static int trial_counter_read(u16 *trial_state_ctr)
> >  	efi_status_t status;
> >  	efi_uintn_t var_size;
> >
> > -	var_size = (efi_uintn_t)sizeof(trial_state_ctr);
> > +	var_size = (efi_uintn_t)sizeof(*trial_state_ctr);
> >  	status = efi_get_variable_int(u"TrialStateCtr",
> >  				      &efi_global_variable_guid,
> >  				      NULL,
> >
> > ---
> > base-commit: a44f46af0aa17e48572e0d46c0d082886ee353c3
> > change-id: 20260914-fwu-24265a691f9a
> >
> > Best regards,
> > --
> > Vincent.
>