[PATCH] efi_loader: check efi_get_variable_int return value

[Masahisa Kojima]

On Fri, 26 Jan 2024 at 21:58, Heinrich Schuchardt <xypron.glpk at 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 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().

OK, I will fix it.

Thanks,
Masahisa Kojima

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