Re: [PATCH v2 2/2] efi_loader: support fmp versioning for multi bank update

11 Jan 2024


      On Wed, 10 Jan 2024 at 18:10, Ilias Apalodimas
ilias.apalodimas@linaro.org wrote:
...
On Wed, 10 Jan 2024 at 02:53, Masahisa Kojima
masahisa.kojima@linaro.org wrote:
...
On Tue, 9 Jan 2024 at 22:02, Ilias Apalodimas
ilias.apalodimas@linaro.org wrote:
...
On Tue, 9 Jan 2024 at 03:00, Masahisa Kojima masahisa.kojima@linaro.org wrote:
...
Hi Ilias,
On Thu, 28 Dec 2023 at 00:14, Ilias Apalodimas
ilias.apalodimas@linaro.org wrote:
...
Kojima-san
On Thu, Dec 21, 2023 at 06:52:58PM +0900, Masahisa Kojima wrote:
...
This commit stores the firmware version into the array
of fmp_state structure to support the fmp versioning
for multi bank update. The index of the array is identified
by the bank index.
Why do we all of them? Can't we just always store/use the version of the active
bank?
Sorry for the late reply.
When the capsule update is reverted, the active bank is back to the
previous active bank.
So I think versions for all banks should be stored.
But even in that case, the active bank should point to the correct
version when the ESRT tables are generated. Wouldn't it be simpler to
just set the FmpState variable accordingly when that happens?
The fw_version in FmpState variable comes from UEFI Capsule Header and
FmpState variable is set when the UEFI capsule update is performed.
If we only store the fw_version of the active bank, the fw_version of
the previous active bank is overwritten. When the capsule is reverted,
we can not get the fw_version of the previous active bank.
Thanks,
Masahisa Kojima
Ah yes, that makes sense.  In that case, the patch looks fine, apart
from the return value of the getvariable call which is never checked
...
...
...
...
...
  image_type_id = efi_firmware_get_image_type_id(image_index);
  if (!image_type_id)

@@ -372,19 +392,38 @@ efi_status_t efi_firmware_set_fmp_state_var(struct fmp_state *state, u8 image_in
      efi_create_indexed_name(varname, sizeof(varname), "FmpState",
                              image_index);

if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) {


        ret = fwu_plat_get_update_index(&update_bank);


        if (ret)


                return EFI_INVALID_PARAMETER;



        num_banks = CONFIG_FWU_NUM_BANKS;


}



size = num_banks * sizeof(*var_state);


var_state = calloc(1, size);


if (!var_state)


        return EFI_OUT_OF_RESOURCES;



ret = efi_get_variable_int(varname, image_type_id, NULL, &size,


                           var_state, NULL);




We should be checking the return value here.
I will fix it.
Thanks,
Masahisa Kojima
...
...
...
...
...
...
  /*
   * Only the fw_version is set here.
   * lowest_supported_version in FmpState variable is ignored since
   * it can be tampered if the file based EFI variable storage is used.
   */


var_state.fw_version = state->fw_version;




var_state[update_bank].fw_version = state->fw_version;



size = num_banks * sizeof(*var_state);
ret = efi_set_variable_int(varname, image_type_id,
                           EFI_VARIABLE_READ_ONLY |
                           EFI_VARIABLE_NON_VOLATILE |
                           EFI_VARIABLE_BOOTSERVICE_ACCESS |
                           EFI_VARIABLE_RUNTIME_ACCESS,




                           sizeof(var_state), &var_state, false);




                           size, var_state, false);



free(var_state);

return ret;



}
2.34.1
Thanks
/Ilias

    