[PATCH v2 2/2] cmd: mvebu/bubt: Check for A38x/A37xx OTP secure bits and secure boot

Pali Rohár pali at kernel.org
Tue Aug 23 14:44:55 CEST 2022


On Tuesday 23 August 2022 07:05:33 Stefan Roese wrote:
> Hi Pali,
> 
> On 09.08.22 21:42, Pali Rohár wrote:
> > For obvious reasons BootROMS rejects unsigned images when secure boot is
> > enabled in OTP secure bits. So check for OPT secure bits and do not allow
> > flashing unsigned images when secure boot is enabled. Access to OTP via
> > U-Boot fuse API is currently implemented only for A38x and A37xx SoCs.
> > 
> > Additionally Armada 3700 BootROM rejects signed trusted image when secure
> > boot is not enabled in OTP. So add also check for this case. On the other
> > hand Armada 38x BootROM acceps images with secure boot header when secure
> > boot is not enabled in OTP.
> > 
> > OTP secure bits may have burned also boot device source. Check it also and
> > reject flashing images to target storage which does not match OTP.
> > 
> > Signed-off-by: Pali Rohár <pali at kernel.org>
> > 
> > ---
> > Changes in v2:
> > * Add missing dependency on MVEBU_EFUSE
> > * Use only for A38x and A37xx
> 
> Running a world CI build leads to these errors:
> 
> $ make clearfog_gt_8k_defconfig
> ...
> $ make -sj
> cmd/mvebu/bubt.c:809:12: warning: 'fuse_read_u64' defined but not used
> [-Wunused-function]
>   809 | static u64 fuse_read_u64(u32 bank)
>       |            ^~~~~~~~~~~~~
> 
> Could you please please take a look and fix this issue?

There is missing guard to not compile that function for A7k/A8k. I will
fix it in v3.

> Thanks,
> Stefan
> 
> > ---
> >   cmd/mvebu/Kconfig |   1 +
> >   cmd/mvebu/bubt.c  | 127 +++++++++++++++++++++++++++++++++++++++++++---
> >   2 files changed, 120 insertions(+), 8 deletions(-)
> > 
> > diff --git a/cmd/mvebu/Kconfig b/cmd/mvebu/Kconfig
> > index 120397d6d4d0..9ec3aa983a51 100644
> > --- a/cmd/mvebu/Kconfig
> > +++ b/cmd/mvebu/Kconfig
> > @@ -5,6 +5,7 @@ config CMD_MVEBU_BUBT
> >   	bool "bubt"
> >   	select SHA256 if ARMADA_3700
> >   	select SHA512 if ARMADA_3700
> > +	select MVEBU_EFUSE if ARMADA_38X || ARMADA_3700
> >   	help
> >   	  bubt - Burn a u-boot image to flash
> >   	  For details about bubt command please see the documentation
> > diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
> > index 3b6ffb7ffd1f..feb1e778a98c 100644
> > --- a/cmd/mvebu/bubt.c
> > +++ b/cmd/mvebu/bubt.c
> > @@ -13,6 +13,8 @@
> >   #include <vsprintf.h>
> >   #include <errno.h>
> >   #include <dm.h>
> > +#include <fuse.h>
> > +#include <mach/efuse.h>
> >   #include <spi_flash.h>
> >   #include <spi.h>
> > @@ -121,6 +123,17 @@ struct a38x_main_hdr_v1 {
> >   	u8  checksum;              /* 0x1F      */
> >   };
> > +/*
> > + * Header for the optional headers, version 1 (Armada 370/XP/375/38x/39x)
> > + */
> > +struct a38x_opt_hdr_v1 {
> > +	u8	headertype;
> > +	u8	headersz_msb;
> > +	u16	headersz_lsb;
> > +	u8	data[0];
> > +};
> > +#define A38X_OPT_HDR_V1_SECURE_TYPE	0x1
> > +
> >   struct a38x_boot_mode {
> >   	unsigned int id;
> >   	const char *name;
> > @@ -706,6 +719,7 @@ static int check_image_header(void)
> >   {
> >   	u8 checksum;
> >   	u32 checksum32, exp_checksum32;
> > +	u32 offset, size;
> >   	const struct a38x_main_hdr_v1 *hdr =
> >   		(struct a38x_main_hdr_v1 *)get_load_addr();
> >   	const size_t image_size = a38x_header_size(hdr);
> > @@ -752,6 +766,38 @@ static int check_image_header(void)
> >   	printf("Image checksum...OK!\n");
> >   	return 0;
> >   }
> > +
> > +#if defined(CONFIG_ARMADA_38X)
> > +static int a38x_image_is_secure(const struct a38x_main_hdr_v1 *hdr)
> > +{
> > +	u32 image_size = a38x_header_size(hdr);
> > +	struct a38x_opt_hdr_v1 *ohdr;
> > +	u32 ohdr_size;
> > +
> > +	if (hdr->version != 1)
> > +		return 0;
> > +
> > +	if (!hdr->ext)
> > +		return 0;
> > +
> > +	ohdr = (struct a38x_opt_hdr_v1 *)(hdr + 1);
> > +	do {
> > +		if (ohdr->headertype == A38X_OPT_HDR_V1_SECURE_TYPE)
> > +			return 1;
> > +
> > +		ohdr_size = (ohdr->headersz_msb << 16) | le16_to_cpu(ohdr->headersz_lsb);
> > +
> > +		if (!*((u8 *)ohdr + ohdr_size - 4))
> > +			break;
> > +
> > +		ohdr = (struct a38x_opt_hdr_v1 *)((u8 *)ohdr + ohdr_size);
> > +		if ((u8 *)ohdr >= (u8 *)hdr + image_size)
> > +			break;
> > +	} while (1);
> > +
> > +	return 0;
> > +}
> > +#endif
> >   #else /* Not ARMADA? */
> >   static int check_image_header(void)
> >   {
> > @@ -760,20 +806,58 @@ static int check_image_header(void)
> >   }
> >   #endif
> > +static u64 fuse_read_u64(u32 bank)
> > +{
> > +	u32 val[2];
> > +	int ret;
> > +
> > +	ret = fuse_read(bank, 0, &val[0]);
> > +	if (ret < 0)
> > +		return -1;
> > +
> > +	ret = fuse_read(bank, 1, &val[1]);
> > +	if (ret < 0)
> > +		return -1;
> > +
> > +	return ((u64)val[1] << 32) | val[0];
> > +}
> > +
> > +#if defined(CONFIG_ARMADA_3700)
> > +static inline u8 maj3(u8 val)
> > +{
> > +	/* return majority vote of 3 bits */
> > +	return ((val & 0x7) == 3 || (val & 0x7) > 4) ? 1 : 0;
> > +}
> > +#endif
> > +
> >   static int bubt_check_boot_mode(const struct bubt_dev *dst)
> >   {
> >   #if defined(CONFIG_ARMADA_3700) || defined(CONFIG_ARMADA_32BIT)
> > -	int mode;
> > +	int mode, secure_mode;
> >   #if defined(CONFIG_ARMADA_3700)
> >   	const struct tim_boot_flash_sign *boot_modes = tim_boot_flash_signs;
> >   	const struct common_tim_data *hdr =
> >   		(struct common_tim_data *)get_load_addr();
> >   	u32 id = hdr->boot_flash_sign;
> > +	int is_secure = hdr->trusted != 0;
> > +	u64 otp_secure_bits = fuse_read_u64(1);
> > +	int otp_secure_boot = ((maj3(otp_secure_bits >> 0) << 0) |
> > +			       (maj3(otp_secure_bits >> 4) << 1)) == 2;
> > +	unsigned int otp_boot_device = (maj3(otp_secure_bits >> 48) << 0) |
> > +				       (maj3(otp_secure_bits >> 52) << 1) |
> > +				       (maj3(otp_secure_bits >> 56) << 2) |
> > +				       (maj3(otp_secure_bits >> 60) << 3);
> >   #elif defined(CONFIG_ARMADA_32BIT)
> >   	const struct a38x_boot_mode *boot_modes = a38x_boot_modes;
> >   	const struct a38x_main_hdr_v1 *hdr =
> >   		(struct a38x_main_hdr_v1 *)get_load_addr();
> >   	u32 id = hdr->blockid;
> > +#if defined(CONFIG_ARMADA_38X)
> > +	int is_secure = a38x_image_is_secure(hdr);
> > +	u64 otp_secure_bits = fuse_read_u64(EFUSE_LINE_SECURE_BOOT);
> > +	int otp_secure_boot = otp_secure_bits & 0x1;
> > +	unsigned int otp_boot_device = (otp_secure_bits >> 8) & 0x7;
> > +#endif
> >   #endif
> >   	for (mode = 0; boot_modes[mode].name; mode++) {
> > @@ -786,15 +870,42 @@ static int bubt_check_boot_mode(const struct bubt_dev *dst)
> >   		return -ENOEXEC;
> >   	}
> > -	if (strcmp(boot_modes[mode].name, dst->name) == 0)
> > -		return 0;
> > +	if (strcmp(boot_modes[mode].name, dst->name) != 0) {
> > +		printf("Error: image meant to be booted from \"%s\", not \"%s\"!\n",
> > +		       boot_modes[mode].name, dst->name);
> > +		return -ENOEXEC;
> > +	}
> > -	printf("Error: image meant to be booted from \"%s\", not \"%s\"!\n",
> > -	       boot_modes[mode].name, dst->name);
> > -	return -ENOEXEC;
> > -#else
> > -	return 0;
> > +#if defined(CONFIG_ARMADA_38X) || defined(CONFIG_ARMADA_3700)
> > +	if (otp_secure_bits == (u64)-1) {
> > +		printf("Error: cannot read OTP secure bits\n");
> > +		return -ENOEXEC;
> > +	} else {
> > +		if (otp_secure_boot && !is_secure) {
> > +			printf("Error: secure boot is enabled in OTP but image does not have secure boot header!\n");
> > +			return -ENOEXEC;
> > +		} else if (!otp_secure_boot && is_secure) {
> > +#if defined(CONFIG_ARMADA_3700)
> > +			/*
> > +			 * Armada 3700 BootROM rejects trusted image when secure boot is not enabled.
> > +			 * Armada 385 BootROM accepts image with secure boot header also when secure boot is not enabled.
> > +			 */
> > +			printf("Error: secure boot is disabled in OTP but image has secure boot header!\n");
> > +			return -ENOEXEC;
> >   #endif
> > +		} else if (otp_boot_device && otp_boot_device != id) {
> > +			for (secure_mode = 0; boot_modes[secure_mode].name; secure_mode++) {
> > +				if (boot_modes[secure_mode].id == otp_boot_device)
> > +					break;
> > +			}
> > +			printf("Error: boot source is set to \"%s\" in OTP but image is for \"%s\"!\n",
> > +			       boot_modes[secure_mode].name ?: "unknown", dst->name);
> > +			return -ENOEXEC;
> > +		}
> > +	}
> > +#endif
> > +#endif
> > +	return 0;
> >   }
> >   static int bubt_verify(const struct bubt_dev *dst)
> 
> Viele Grüße,
> Stefan Roese
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de


More information about the U-Boot mailing list