[v7,08/13] FWU: Add boot time checks as highlighted by the FWU specification

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@linaro.org>
---
Changes since V6: None

 common/board_r.c      |   5 ++
 include/fwu.h         |   3 +
 lib/fwu_updates/fwu.c | 165 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 173 insertions(+)
 create mode 100644 lib/fwu_updates/fwu.c

Hi Sughosh,

On Thu, 14 Jul 2022 at 21:40, Sughosh Ganu <sughosh.ganu@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@linaro.org>
> ---
> Changes since V6: None
>
>  common/board_r.c      |   5 ++
>  include/fwu.h         |   3 +
>  lib/fwu_updates/fwu.c | 165 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 173 insertions(+)
>  create mode 100644 lib/fwu_updates/fwu.c
>
> diff --git a/common/board_r.c b/common/board_r.c
> index ed29069d2d..5210ed6f32 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -15,6 +15,7 @@
>  #include <cpu_func.h>
>  #include <exports.h>
>  #include <flash.h>
> +#include <fwu.h>
>  #include <hang.h>
>  #include <image.h>
>  #include <irq_func.h>
> @@ -785,6 +786,10 @@ static init_fnc_t init_sequence_r[] = {
>  #if defined(CONFIG_PRAM)
>         initr_mem,
>  #endif
> +
> +#ifdef CONFIG_FWU_MULTI_BANK_UPDATE
> +       fwu_boottime_checks,
> +#endif
>         run_main_loop,
>  };
>
> diff --git a/include/fwu.h b/include/fwu.h
> index edb28c9659..b374fd1179 100644
> --- a/include/fwu.h
> +++ b/include/fwu.h
> @@ -37,6 +37,9 @@ struct fwu_mdata_ops {
>         EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \
>                  0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23)
>
> +u8 fwu_update_checks_pass(void);
> +int fwu_boottime_checks(void);
> +
>  int fwu_get_mdata(struct fwu_mdata **mdata);
>  int fwu_update_mdata(struct fwu_mdata *mdata);
>  int fwu_get_active_index(u32 *active_idx);
> diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c
> new file mode 100644
> index 0000000000..10a0522333
> --- /dev/null
> +++ b/lib/fwu_updates/fwu.c
> @@ -0,0 +1,165 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * 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;

Is ret = 0 needed here? Isn't it 0 already ?

> +       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;
> +               }
> +       }

Is this used elsewhere in the patchset?  The function is starting to
be big, so perhaps moving this in a static bool "in_trial_state()" or
similar would make it more readable.

> +
> +       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(u"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;
> +                       }
> +
> +                       /* Delete the TrialStateCtr variable */
> +                       status = efi_set_variable_int(u"TrialStateCtr",
> +                                                     &efi_global_variable_guid,
> +                                                     var_attributes,
> +                                                     0, NULL, false);
> +                       if (status != EFI_SUCCESS) {
> +                               log_err("Unable to delete TrialStateCtr variable\n");
> +                               ret = -1;
> +                               goto out;
> +                       }
> +               } else {
> +                       status = efi_set_variable_int(u"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 {
> +               /* Delete the variable */
> +               status = efi_set_variable_int(u"TrialStateCtr",
> +                                             &efi_global_variable_guid,
> +                                             0, 0, NULL, NULL);

the last argument should be false

> +               if (status != EFI_SUCCESS && status != EFI_NOT_FOUND) {

Is there a chance the code will try to delete the variable when it's not there?

> +                       ret = -1;
> +                       log_err("Unable to delete TrialStateCtr variable\n");
> +               }
> +       }

Can we please move get/set variable into functions?  Something like
- trial_counter_inc
- trial_counter_reset

Regards
/Ilias



> +
> +out:
> +       free(mdata);
> +       return ret;
> +}
> +
> +u8 fwu_update_checks_pass(void)
> +{
> +       return !trial_state && boottime_check;
> +}
> +
> +int fwu_boottime_checks(void)
> +{
> +       int ret;
> +       u32 boot_idx, active_idx;
> +
> +       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.34.1
>

Hi Sughosh,

>
> > +       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;
> > +               }
> > +       }
>
> Is this used elsewhere in the patchset?  The function is starting to
> be big, so perhaps moving this in a static bool "in_trial_state()" or
> similar would make it more readable.
>

There was a discussion about this on the synquacer thread for A/B
updates.  Once you split those in a function, it's better to extend
the bootcount API with an EFI backed storage.  The reasoning that a
user might disable editing env variables for security reasons and that
device might not be able to preserve RAM or store the counter in CPU
registers across reboots.

If we extend the bootcount API with this code we can plug in the
functionality seamlessly based on the hardware capabilities.

[...]

Regards

/Ilias

hi Ilias,

On Wed, 20 Jul 2022 at 13:06, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Sughosh,
>
> >
> > > +       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;
> > > +               }
> > > +       }
> >
> > Is this used elsewhere in the patchset?  The function is starting to
> > be big, so perhaps moving this in a static bool "in_trial_state()" or
> > similar would make it more readable.
> >
>
> There was a discussion about this on the synquacer thread for A/B
> updates.  Once you split those in a function, it's better to extend
> the bootcount API with an EFI backed storage.  The reasoning that a
> user might disable editing env variables for security reasons and that
> device might not be able to preserve RAM or store the counter in CPU
> registers across reboots.
>
> If we extend the bootcount API with this code we can plug in the
> functionality seamlessly based on the hardware capabilities.

I have incorporated the rest of your comments on this patch. I moved
the access to the TrialStateCtr EFI variable to the bootcount API like
you suggested. However, I face an issue, in that on every boot, the
bootdelay_process function gets called which increments the bootcount
variable. This does not map with the requirement of the FWU feature
since the TrialStateCtr variable needs to be updated only when the
platform transitions to the Trial State, after a successful update.
Once the platform transitions from the Trial State to the Regular
State, the variable gets deleted. Hence I think it would be better to
use the current logic of the handling of the TrialStateCtr variable.
Thanks.

-sughosh

>
> [...]
>
> Regards
>
> /Ilias