On Fri, 26 Jan 2024 at 21:58, Heinrich Schuchardt <xypron.g...@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.g...@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.koj...@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.
> >>
>
