[PATCH] efi_loader: check efi_get_variable_int return value

[Heinrich Schuchardt]

On 26.01.24 12:36, Masahisa Kojima wrote:
> Hi Heinrich,
>
> On Fri, 26 Jan 2024 at 16:20, Heinrich Schuchardt <xypron.glpk at 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 at 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.
>>