diff mbox series

efi_loader: check efi_get_variable_int return value

Message ID 20240122072617.1104227-1-masahisa.kojima@linaro.org
State Superseded
Headers show
Series efi_loader: check efi_get_variable_int return value | expand

Commit Message

Masahisa Kojima Jan. 22, 2024, 7:26 a.m. UTC
efi_get_variable_int() may fail, the buffer should be
cleared before using it.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
Addresses-Coverity-ID: 478333 ("Error handling issues")
---
 lib/efi_loader/efi_firmware.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Heinrich Schuchardt Jan. 26, 2024, 7:20 a.m. UTC | #1
On 1/22/24 08:26, Masahisa Kojima wrote:
> efi_get_variable_int() may fail, the buffer should be
> cleared before using it.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> Addresses-Coverity-ID: 478333 ("Error handling issues")
> ---
>   lib/efi_loader/efi_firmware.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> index 9fd13297a6..fb83fa8113 100644
> --- a/lib/efi_loader/efi_firmware.c
> +++ b/lib/efi_loader/efi_firmware.c
> @@ -407,11 +407,11 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in
>   	/*
>   	 * GetVariable may fail, EFI_NOT_FOUND is returned if FmpState
>   	 * variable has not been set yet.
> -	 * Ignore the error here since the correct FmpState variable
> -	 * is set later.
>   	 */
> -	efi_get_variable_int(varname, image_type_id, NULL, &size, var_state,
> -			     NULL);
> +	ret = efi_get_variable_int(varname, image_type_id, NULL, &size,
> +				   var_state, NULL);
> +	if (ret != EFI_SUCCESS)
> +		memset(var_state, 0, num_banks * sizeof(*var_state));

Hello Masahisa,

We allocate var_state with calloc(). So it is cleared before we call
GetVariable(). Do we have an implementation of GetVariable() that would
fill it with data before returning an error code?

Best regards

Heinrich

>
>   	/*
>   	 * Only the fw_version is set here.
Masahisa Kojima Jan. 26, 2024, 11:36 a.m. UTC | #2
Hi Heinrich,

On Fri, 26 Jan 2024 at 16:20, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 1/22/24 08:26, Masahisa Kojima wrote:
> > efi_get_variable_int() may fail, the buffer should be
> > cleared before using it.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > Addresses-Coverity-ID: 478333 ("Error handling issues")
> > ---
> >   lib/efi_loader/efi_firmware.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> > index 9fd13297a6..fb83fa8113 100644
> > --- a/lib/efi_loader/efi_firmware.c
> > +++ b/lib/efi_loader/efi_firmware.c
> > @@ -407,11 +407,11 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in
> >       /*
> >        * GetVariable may fail, EFI_NOT_FOUND is returned if FmpState
> >        * variable has not been set yet.
> > -      * Ignore the error here since the correct FmpState variable
> > -      * is set later.
> >        */
> > -     efi_get_variable_int(varname, image_type_id, NULL, &size, var_state,
> > -                          NULL);
> > +     ret = efi_get_variable_int(varname, image_type_id, NULL, &size,
> > +                                var_state, NULL);
> > +     if (ret != EFI_SUCCESS)
> > +             memset(var_state, 0, num_banks * sizeof(*var_state));
>
> Hello Masahisa,
>
> We allocate var_state with calloc(). So it is cleared before we call
> GetVariable(). Do we have an implementation of GetVariable() that would
> fill it with data before returning an error code?

No, current GetVariable() implementations do not fill the buffer
when GetVariable() returns an error.
This is why my original implementation does not clear the buffer in
case of error.

Anyway, I agree with your previous comment.
I think it is better to clear the buffer in case of error for safety,
because UEFI spec does not say GetVariable() will not fill the
buffer in case of error.

Thanks,
Masahisa Kojima


>
> Best regards
>
> Heinrich
>
> >
> >       /*
> >        * Only the fw_version is set here.
>
Heinrich Schuchardt Jan. 26, 2024, 12:58 p.m. UTC | #3
On 26.01.24 12:36, Masahisa Kojima wrote:
> Hi Heinrich,
>
> On Fri, 26 Jan 2024 at 16:20, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 1/22/24 08:26, Masahisa Kojima wrote:
>>> efi_get_variable_int() may fail, the buffer should be
>>> cleared before using it.
>>>
>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>> Addresses-Coverity-ID: 478333 ("Error handling issues")
>>> ---
>>>    lib/efi_loader/efi_firmware.c | 8 ++++----
>>>    1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
>>> index 9fd13297a6..fb83fa8113 100644
>>> --- a/lib/efi_loader/efi_firmware.c
>>> +++ b/lib/efi_loader/efi_firmware.c
>>> @@ -407,11 +407,11 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in
>>>        /*
>>>         * GetVariable may fail, EFI_NOT_FOUND is returned if FmpState
>>>         * variable has not been set yet.
>>> -      * Ignore the error here since the correct FmpState variable
>>> -      * is set later.
>>>         */
>>> -     efi_get_variable_int(varname, image_type_id, NULL, &size, var_state,
>>> -                          NULL);
>>> +     ret = efi_get_variable_int(varname, image_type_id, NULL, &size,
>>> +                                var_state, NULL);
>>> +     if (ret != EFI_SUCCESS)
>>> +             memset(var_state, 0, num_banks * sizeof(*var_state));
>>
>> Hello Masahisa,
>>
>> We allocate var_state with calloc(). So it is cleared before we call
>> GetVariable(). Do we have an implementation of GetVariable() that would
>> fill it with data before returning an error code?
>
> No, current GetVariable() implementations do not fill the buffer
> when GetVariable() returns an error.
> This is why my original implementation does not clear the buffer in
> case of error.
>
> Anyway, I agree with your previous comment.
> I think it is better to clear the buffer in case of error for safety,
> because UEFI spec does not say GetVariable() will not fill the
> buffer in case of error.

In this case I guess malloc() is good enough. No need to use calloc().

Best regards

Heinrich

>
> Thanks,
> Masahisa Kojima
>
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>        /*
>>>         * Only the fw_version is set here.
>>
Masahisa Kojima Jan. 29, 2024, 2:22 a.m. UTC | #4
On Fri, 26 Jan 2024 at 21:58, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 26.01.24 12:36, Masahisa Kojima wrote:
> > Hi Heinrich,
> >
> > On Fri, 26 Jan 2024 at 16:20, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 1/22/24 08:26, Masahisa Kojima wrote:
> >>> efi_get_variable_int() may fail, the buffer should be
> >>> cleared before using it.
> >>>
> >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>> Addresses-Coverity-ID: 478333 ("Error handling issues")
> >>> ---
> >>>    lib/efi_loader/efi_firmware.c | 8 ++++----
> >>>    1 file changed, 4 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
> >>> index 9fd13297a6..fb83fa8113 100644
> >>> --- a/lib/efi_loader/efi_firmware.c
> >>> +++ b/lib/efi_loader/efi_firmware.c
> >>> @@ -407,11 +407,11 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in
> >>>        /*
> >>>         * GetVariable may fail, EFI_NOT_FOUND is returned if FmpState
> >>>         * variable has not been set yet.
> >>> -      * Ignore the error here since the correct FmpState variable
> >>> -      * is set later.
> >>>         */
> >>> -     efi_get_variable_int(varname, image_type_id, NULL, &size, var_state,
> >>> -                          NULL);
> >>> +     ret = efi_get_variable_int(varname, image_type_id, NULL, &size,
> >>> +                                var_state, NULL);
> >>> +     if (ret != EFI_SUCCESS)
> >>> +             memset(var_state, 0, num_banks * sizeof(*var_state));
> >>
> >> Hello Masahisa,
> >>
> >> We allocate var_state with calloc(). So it is cleared before we call
> >> GetVariable(). Do we have an implementation of GetVariable() that would
> >> fill it with data before returning an error code?
> >
> > No, current GetVariable() implementations do not fill the buffer
> > when GetVariable() returns an error.
> > This is why my original implementation does not clear the buffer in
> > case of error.
> >
> > Anyway, I agree with your previous comment.
> > I think it is better to clear the buffer in case of error for safety,
> > because UEFI spec does not say GetVariable() will not fill the
> > buffer in case of error.
>
> In this case I guess malloc() is good enough. No need to use calloc().

OK, I will fix it.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> >
> > Thanks,
> > Masahisa Kojima
> >
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>>        /*
> >>>         * Only the fw_version is set here.
> >>
>
diff mbox series

Patch

diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c
index 9fd13297a6..fb83fa8113 100644
--- a/lib/efi_loader/efi_firmware.c
+++ b/lib/efi_loader/efi_firmware.c
@@ -407,11 +407,11 @@  efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in
 	/*
 	 * GetVariable may fail, EFI_NOT_FOUND is returned if FmpState
 	 * variable has not been set yet.
-	 * Ignore the error here since the correct FmpState variable
-	 * is set later.
 	 */
-	efi_get_variable_int(varname, image_type_id, NULL, &size, var_state,
-			     NULL);
+	ret = efi_get_variable_int(varname, image_type_id, NULL, &size,
+				   var_state, NULL);
+	if (ret != EFI_SUCCESS)
+		memset(var_state, 0, num_banks * sizeof(*var_state));
 
 	/*
 	 * Only the fw_version is set here.