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 <p...@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: s...@denx.de
