[PATCH] fwu: fix read buffer size

[PATCH] fwu: fix read buffer size

[Vincent Stehlé]

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.

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.

Re: [PATCH] fwu: fix read buffer size

[Kory Maincent]

On 9/15/26 16:29, 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.

Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>

Thank you!
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH] fwu: fix read buffer size

[Ilias Apalodimas]

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.


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.

Re: [PATCH] fwu: fix read buffer size

[Vincent Stehlé]

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.
> 

Re: [PATCH] fwu: fix read buffer size

[Ilias Apalodimas]

On Fri, 18 Sept 2026 at 16:45, Vincent Stehlé <vincent.stehle@arm.com> 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 <ilias.apalodimas@linaro.org>

>
> 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.
> >