diff mbox series

[RFC,v3,6/9] FWU: Add boot time checks as highlighted by the FWU specification

Message ID 20220119185548.16730-7-sughosh.ganu@linaro.org
State Superseded
Headers show
Series FWU: Add support for FWU Multi Bank Update feature | expand

Commit Message

Sughosh Ganu Jan. 19, 2022, 6:55 p.m. UTC
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 V2:
* Add logic to delete the TrialStateCtr variable if system is not in
  Trial State

 common/board_r.c      |   6 ++
 include/fwu.h         |   3 +
 lib/fwu_updates/fwu.c | 171 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 180 insertions(+)
 create mode 100644 lib/fwu_updates/fwu.c

Comments

Ilias Apalodimas Jan. 21, 2022, 1:15 p.m. UTC | #1
Hi Sughosh

[...]

> +#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 = 0;
> +static u8 boottime_check = 0;
> +
> +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 < 0)
> +		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) {

That's personal preference and feel free to ignore it,  but I generally
find the code easier to read like 
if (!trial_state)
	do stuff;
	goto out;

<rest of the code follows here>

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

Do we really need it with runtime 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 < 0) {
> +				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,
> +						      &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 {
> +				ret = 0;
> +			}
> +		}
> +	} else {
> +		trial_state_ctr = 0;
> +		ret = 0;
> +		status = efi_set_variable_int(L"TrialStateCtr",
> +					      &efi_global_variable_guid,
> +					      0,
> +					      0, &trial_state_ctr,
> +					      NULL);

We can't completely ignore the failed setvariable here, as it will affect
the entire code path from that point onwards.  Any checks after an update
will fail since the variable wont be there.


> +	}
> +
> +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 < 0) {
> +		boottime_check = 0;
> +		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");
> +		boottime_check = 0;
> +		return 0;
> +	}
> +
> +	ret = fwu_get_active_index(&active_idx);
> +	if (ret < 0) {
> +		log_err("Unable to read active_index\n");
> +		boottime_check = 0;
> +		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 < 0)
> +			boottime_check = 0;
> +		else
> +			boottime_check = 1;
> +
> +		return 0;
> +	}
> +
> +	ret = fwu_trial_state_check();
> +	if (ret < 0)
> +		boottime_check = 0;
> +	else
> +		boottime_check = 1;
> +
> +	return 0;
> +}
> -- 

cheers
/Ilias
> 2.17.1
>
diff mbox series

Patch

diff --git a/common/board_r.c b/common/board_r.c
index 31a59c585a..81678870b9 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -78,6 +78,9 @@ 
 #ifdef CONFIG_EFI_SETUP_EARLY
 #include <efi_loader.h>
 #endif
+#ifdef CONFIG_FWU_MULTI_BANK_UPDATE
+#include <fwu.h>
+#endif
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -805,6 +808,9 @@  static init_fnc_t init_sequence_r[] = {
 #endif
 #ifdef CONFIG_EFI_SETUP_EARLY
 	(init_fnc_t)efi_init_obj_list,
+#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 6393a1dbb5..950a816dbd 100644
--- a/include/fwu.h
+++ b/include/fwu.h
@@ -47,6 +47,9 @@  struct fwu_mdata_ops *get_plat_fwu_mdata_ops(void);
 	EFI_GUID(0x8a7a84a0, 0x8387, 0x40f6, 0xab, 0x41, \
 		 0xa8, 0xb9, 0xa5, 0xa6, 0x0d, 0x23)
 
+int fwu_boottime_checks(void);
+u8 fwu_update_checks_pass(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..906b5e622a
--- /dev/null
+++ b/lib/fwu_updates/fwu.c
@@ -0,0 +1,171 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2021, Linaro Limited
+ */
+
+#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 = 0;
+static u8 boottime_check = 0;
+
+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 < 0)
+		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 |
+			EFI_VARIABLE_RUNTIME_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 < 0) {
+				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,
+						      &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 {
+				ret = 0;
+			}
+		}
+	} else {
+		trial_state_ctr = 0;
+		ret = 0;
+		status = efi_set_variable_int(L"TrialStateCtr",
+					      &efi_global_variable_guid,
+					      0,
+					      0, &trial_state_ctr,
+					      NULL);
+	}
+
+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 < 0) {
+		boottime_check = 0;
+		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");
+		boottime_check = 0;
+		return 0;
+	}
+
+	ret = fwu_get_active_index(&active_idx);
+	if (ret < 0) {
+		log_err("Unable to read active_index\n");
+		boottime_check = 0;
+		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 < 0)
+			boottime_check = 0;
+		else
+			boottime_check = 1;
+
+		return 0;
+	}
+
+	ret = fwu_trial_state_check();
+	if (ret < 0)
+		boottime_check = 0;
+	else
+		boottime_check = 1;
+
+	return 0;
+}