[PATCH v2 2/2] cmd: mvebu/bubt: Check for A38x/A37xx OTP secure bits and secure boot
Stefan Roese
sr at denx.de
Tue Aug 23 16:53:23 CEST 2022
On 23.08.22 14:44, Pali Rohár wrote:
> 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 for fixing this. I'll probably skip v3 in the next pull request,
as I've everything ready with the other patches now.
Thanks,
Stefan
>> 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
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