On Fri, 18 Sept 2026 at 16:45, Vincent Stehlé <[email protected]> wrote: > > 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.
Yep, I misread the patch the first time. But indeed efi_get_variable_int() only copies the actual data length Reviewed-by: Ilias Apalodimas <[email protected]> > > Best regards, > Vincent. > > > > > > > Cheers > > /Ilias > > > > > > Fixes: 7e9814cc6c1d ("FWU: Add boot time checks as highlighted by the FWU > > > specification") > > > Signed-off-by: Vincent Stehlé <[email protected]> > > > Cc: Sughosh Ganu <[email protected]> > > > Cc: Kory Maincent <[email protected]> > > > Cc: Tom Rini <[email protected]> > > > --- > > > 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. > >
