[PATCH v5 08/23] FWU: Add boot time checks as highlighted by the FWU specification
Sughosh Ganu
sughosh.ganu at linaro.org
Tue Jun 28 12:42:19 CEST 2022
hi Etienne,
On Thu, 23 Jun 2022 at 18:02, Etienne Carriere
<etienne.carriere at linaro.org> wrote:
>
> Hi Sughosh,
>
> On Thu, 23 Jun 2022 at 11:46, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >
> > hi Etienne,
> >
> > On Tue, 21 Jun 2022 at 16:26, Etienne Carriere
> > <etienne.carriere at linaro.org> wrote:
> > >
> > > Hi Sughosh,
> > >
> > > On Thu, 9 Jun 2022 at 14:31, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > >
> > > > The FWU Multi Bank Update specification requires the Update Agent to
> > > > carry out certain checks at the time of platform boot. The Update
> > > > Agent is the component which is responsible for updating the firmware
> > > > components and maintaining and keeping the metadata in sync.
> > > >
> > > > The spec requires that the Update Agent perform the following checks
> > > > at the time of boot
> > > > * Sanity check of both the metadata copies maintained by the platform.
> > > > * Get the boot index passed to U-Boot by the prior stage bootloader
> > > > and use this value for metadata bookkeeping.
> > > > * Check if the system is booting in Trial State. If the system boots
> > > > in the Trial State for more than a specified number of boot counts,
> > > > change the Active Bank to be booting the platform from.
> > > >
> > > > Add these checks in the board initialisation sequence, invoked after
> > > > relocation.
> > > >
> > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > ---
> > > > common/board_r.c | 5 ++
> > > > include/fwu.h | 3 +
> > > > lib/fwu_updates/fwu.c | 170 ++++++++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 178 insertions(+)
> > > > create mode 100644 lib/fwu_updates/fwu.c
> >
> > <snip>
> >
> > > > --- /dev/null
> > > > +++ b/lib/fwu_updates/fwu.c
> > > > @@ -0,0 +1,170 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * Copyright (c) 2022, Linaro Limited
> > > > + */
> > > > +
> > > > +#include <dm.h>
> > > > +#include <efi.h>
> > > > +#include <efi_loader.h>
> > > > +#include <efi_variable.h>
> > > > +#include <fwu.h>
> > > > +#include <fwu_mdata.h>
> > > > +#include <malloc.h>
> > > > +
> > > > +#include <linux/errno.h>
> > > > +#include <linux/types.h>
> > > > +
> > > > +static u8 trial_state;
> > > > +static u8 boottime_check;
> > > > +
> > > > +static int fwu_trial_state_check(void)
> > > > +{
> > > > + int ret, i;
> > > > + efi_status_t status;
> > > > + efi_uintn_t var_size;
> > > > + u16 trial_state_ctr;
> > > > + u32 nimages, active_bank, var_attributes, active_idx;
> > > > + struct fwu_mdata *mdata = NULL;
> > > > + struct fwu_image_entry *img_entry;
> > > > + struct fwu_image_bank_info *img_bank_info;
> > > > +
> > > > + ret = fwu_get_mdata(&mdata);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = 0;
> > > > + nimages = CONFIG_FWU_NUM_IMAGES_PER_BANK;
> > > > + active_bank = mdata->active_index;
> > > > + img_entry = &mdata->img_entry[0];
> > > > + for (i = 0; i < nimages; i++) {
> > > > + img_bank_info = &img_entry[i].img_bank_info[active_bank];
> > > > + if (!img_bank_info->accepted) {
> > > > + trial_state = 1;
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + if (trial_state) {
> > > > + var_size = (efi_uintn_t)sizeof(trial_state_ctr);
> > > > + log_info("System booting in Trial State\n");
> > > > + var_attributes = EFI_VARIABLE_NON_VOLATILE |
> > > > + EFI_VARIABLE_BOOTSERVICE_ACCESS;
> > > > + status = efi_get_variable_int(L"TrialStateCtr",
> > > > + &efi_global_variable_guid,
> > > > + &var_attributes,
> > > > + &var_size, &trial_state_ctr,
> > > > + NULL);
> > > > + if (status != EFI_SUCCESS) {
> > > > + log_err("Unable to read TrialStateCtr variable\n");
> > > > + ret = -1;
> > > > + goto out;
> > > > + }
> > > > +
> > > > + ++trial_state_ctr;
> > > > + if (trial_state_ctr > CONFIG_FWU_TRIAL_STATE_CNT) {
> > > > + log_info("Trial State count exceeded. Revert back to previous_active_index\n");
> > > > + active_idx = mdata->active_index;
> > > > + ret = fwu_revert_boot_index();
> > > > + if (ret) {
> > > > + log_err("Unable to revert active_index\n");
> > > > + goto out;
> > > > + }
> > > > +
> > > > + trial_state_ctr = 0;
> > > > + status = efi_set_variable_int(L"TrialStateCtr",
> > > > + &efi_global_variable_guid,
> > > > + var_attributes,
> > > > + 0,
> > >
> > > s/0/var_size/ ?
> > > Ditto 24 lines below.
> >
> > The variable size is 0 since the variable is being deleted here and
> > the other instance that you mention. Maybe I can put a comment in the
> > two places.
>
> The goal is to delete the variable or to reset it to 0?
The goal is to delete the variable.
> If really deleting, this function should rather pass NULL instead of
> &trial_state_ctr.
The efi_set_variable_int checks the values of the attributes and
data_size parameters to decide if the variable is to be deleted. The
value of the variable data is not considered. But I can see that this
can be confusing to someone reading the code. I will pass this as NULL
instead.
>
> Regarding adding an inline comment, i guess you don't need to if
> that's part of efi_set_variable_int() API.
> By the way, it would help if this behaviour was described in the
> function declaration, but that's another story.
>
> >
> > >
> > > > + &trial_state_ctr, false);
> > > > + if (status != EFI_SUCCESS) {
> > > > + log_err("Unable to clear TrialStateCtr variable\n");
> > > > + ret = -1;
> > > > + goto out;
> > > > + }
> > > > + } else {
> > > > + status = efi_set_variable_int(L"TrialStateCtr",
> > > > + &efi_global_variable_guid,
> > > > + var_attributes,
> > > > + var_size,
> > > > + &trial_state_ctr, false);
> > > > + if (status != EFI_SUCCESS) {
> > > > + log_err("Unable to increment TrialStateCtr variable\n");
> > > > + ret = -1;
> > > > + goto out;
> > > > + }
> > > > + }
> > > > + } else {
> > > > + trial_state_ctr = 0;
> > > > + status = efi_set_variable_int(L"TrialStateCtr",
> > > > + &efi_global_variable_guid,
> > > > + 0,
> > > > + 0, &trial_state_ctr,
> > > > + NULL);
> > >
> > > check status value.
> >
> > Okay, I can put a log mentioning the error. There is not much use of
> > the status apart from this.
>
> Failing to reset/delete TrialStateCtr is an error case and should be
> reported when this function returns, no?
I do see your point. I was thinking that the inability to delete the
variable for any reason should not result in the boottime_check
variable being set to 0, as in that case, an update initiated would
not proceed. This may or may not be an issue since the ESP and the
firmware images might be on different storage devices. Please let me
know what would be your preference. Thanks.
-sughosh
>
> >
> > -sughosh
> >
> > >
> > >
> > > > + }
> > > > +
> > > > +out:
> > > > + free(mdata);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +u8 fwu_update_checks_pass(void)
> > > > +{
> > > > + return !trial_state && boottime_check;
> > > > +}
> > > > +
> > > > +int fwu_boottime_checks(void)
> > > > +{
> > > > + int ret;
> > > > + struct udevice *dev;
> > > > + u32 boot_idx, active_idx;
> > > > +
> > > > + if (uclass_get_device(UCLASS_FWU_MDATA, 0, &dev) || !dev) {
> > > > + log_err("FWU Metadata device not found\n");
> > > > + return 0;
> > > > + }
> > > > +
> > > > + ret = fwu_mdata_check();
> > > > + if (ret) {
> > > > + return 0;
> > > > + }
> > > > +
> > > > + /*
> > > > + * Get the Boot Index, i.e. the bank from
> > > > + * which the platform has booted. This value
> > > > + * gets passed from the ealier stage bootloader
> > > > + * which booted u-boot, e.g. tf-a. If the
> > > > + * boot index is not the same as the
> > > > + * active_index read from the FWU metadata,
> > > > + * update the active_index.
> > > > + */
> > > > + fwu_plat_get_bootidx(&boot_idx);
> > > > + if (boot_idx >= CONFIG_FWU_NUM_BANKS) {
> > > > + log_err("Received incorrect value of boot_index\n");
> > > > + return 0;
> > > > + }
> > > > +
> > > > + ret = fwu_get_active_index(&active_idx);
> > > > + if (ret) {
> > > > + log_err("Unable to read active_index\n");
> > > > + return 0;
> > > > + }
> > > > +
> > > > + if (boot_idx != active_idx) {
> > > > + log_info("Boot idx %u is not matching active idx %u, changing active_idx\n",
> > > > + boot_idx, active_idx);
> > > > + ret = fwu_update_active_index(boot_idx);
> > > > + if (!ret)
> > > > + boottime_check = 1;
> > > > +
> > > > + return 0;
> > > > + }
> > > > +
> > > > + if (efi_init_obj_list() != EFI_SUCCESS)
> > > > + return 0;
> > > > +
> > > > + ret = fwu_trial_state_check();
> > > > + if (!ret)
> > > > + boottime_check = 1;
> > > > +
> > > > + return 0;
> > > > +}
> > > > --
> > > > 2.25.1
> > > >
More information about the U-Boot
mailing list