[PATCH 1/2] cmd: mvebu/bubt: Check for A38x image data checksum

[PATCH 1/2] cmd: mvebu/bubt: Check for A38x image data checksum

[Pali Rohár]

Currently for A38x image is checked only header checksum.
So check also for image data checksum to prevent flashing broken image.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 cmd/mvebu/bubt.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
index ffa05bc20181..615234e41897 100644
--- a/cmd/mvebu/bubt.c
+++ b/cmd/mvebu/bubt.c
@@ -689,9 +689,24 @@ static uint8_t image_checksum8(const void *start, size_t len)
 	return csum;
 }
 
+static uint32_t image_checksum32(const void *start, size_t len)
+{
+	u32 csum = 0;
+	const u32 *p = start;
+
+	while (len) {
+		csum += *p;
+		++p;
+		len -= sizeof(u32);
+	}
+
+	return csum;
+}
+
 static int check_image_header(void)
 {
 	u8 checksum;
+	u32 checksum32, exp_checksum32;
 	const struct a38x_main_hdr_v1 *hdr =
 		(struct a38x_main_hdr_v1 *)get_load_addr();
 	const size_t image_size = a38x_header_size(hdr);
@@ -702,11 +717,39 @@ static int check_image_header(void)
 	checksum = image_checksum8(hdr, image_size);
 	checksum -= hdr->checksum;
 	if (checksum != hdr->checksum) {
-		printf("Error: Bad A38x image checksum. 0x%x != 0x%x\n",
+		printf("Error: Bad A38x image header checksum. 0x%x != 0x%x\n",
 		       checksum, hdr->checksum);
 		return -ENOEXEC;
 	}
 
+	offset = le32_to_cpu(hdr->srcaddr);
+	size = le32_to_cpu(hdr->blocksize);
+
+	if (hdr->blockid == 0x78) { /* SATA id */
+		if (offset < 1) {
+			printf("Error: Bad A38x image srcaddr.\n");
+			return -ENOEXEC;
+		}
+		offset -= 1;
+		offset *= 512;
+	}
+
+	if (hdr->blockid == 0xAE) /* SDIO id */
+		offset *= 512;
+
+	if (offset % 4 != 0 || size < 4 || size % 4 != 0) {
+		printf("Error: Bad A38x image blocksize.\n");
+		return -ENOEXEC;
+	}
+
+	checksum32 = image_checksum32((u8 *)hdr + offset, size - 4);
+	exp_checksum32 = *(u32 *)((u8 *)ptr + offset + size - 4);
+	if (checksum32 != exp_checksum32) {
+		printf("Error: Bad A38x image data checksum. 0x%08x != 0x%08x\n",
+		       checksum32, exp_checksum32);
+		return -ENOEXEC;
+	}
+
 	printf("Image checksum...OK!\n");
 	return 0;
 }
-- 
2.20.1

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

[Pali Rohár]

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.

Additionally Armada 3700 BootROM rejects signed trusted image when secure
boot is not enabled in OTP. So add also check for this case.

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@kernel.org>
---
 cmd/mvebu/bubt.c | 123 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 114 insertions(+), 9 deletions(-)

diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
index 615234e41897..ac98154fb759 100644
--- a/cmd/mvebu/bubt.c
+++ b/cmd/mvebu/bubt.c
@@ -14,6 +14,8 @@
 #include <vsprintf.h>
 #include <errno.h>
 #include <dm.h>
+#include <fuse.h>
+#include <mach/efuse.h>
 
 #include <spi_flash.h>
 #include <spi.h>
@@ -122,6 +124,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;
@@ -707,6 +720,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);
@@ -743,7 +757,7 @@ static int check_image_header(void)
 	}
 
 	checksum32 = image_checksum32((u8 *)hdr + offset, size - 4);
-	exp_checksum32 = *(u32 *)((u8 *)ptr + offset + size - 4);
+	exp_checksum32 = *(u32 *)((u8 *)hdr + offset + size - 4);
 	if (checksum32 != exp_checksum32) {
 		printf("Error: Bad A38x image data checksum. 0x%08x != 0x%08x\n",
 		       checksum32, exp_checksum32);
@@ -753,6 +767,36 @@ static int check_image_header(void)
 	printf("Image checksum...OK!\n");
 	return 0;
 }
+
+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;
+}
 #else /* Not ARMADA? */
 static int check_image_header(void)
 {
@@ -761,20 +805,56 @@ 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;
+	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
 
 	for (mode = 0; boot_modes[mode].name; mode++) {
@@ -787,15 +867,40 @@ 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 (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
+	return 0;
 }
 
 static int bubt_verify(const struct bubt_dev *dst)
-- 
2.20.1

[PATCH v2 1/2] cmd: mvebu/bubt: Check for A38x image data checksum

[Pali Rohár]

Currently for A38x image is checked only header checksum.
So check also for image data checksum to prevent flashing broken image.

Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v2:
* Compile fix (move code chunk from patch 2/2 to 1/2)
---
 cmd/mvebu/bubt.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
index 2136af64163c..3b6ffb7ffd1f 100644
--- a/cmd/mvebu/bubt.c
+++ b/cmd/mvebu/bubt.c
@@ -688,9 +688,24 @@ static uint8_t image_checksum8(const void *start, size_t len)
 	return csum;
 }
 
+static uint32_t image_checksum32(const void *start, size_t len)
+{
+	u32 csum = 0;
+	const u32 *p = start;
+
+	while (len) {
+		csum += *p;
+		++p;
+		len -= sizeof(u32);
+	}
+
+	return csum;
+}
+
 static int check_image_header(void)
 {
 	u8 checksum;
+	u32 checksum32, exp_checksum32;
 	const struct a38x_main_hdr_v1 *hdr =
 		(struct a38x_main_hdr_v1 *)get_load_addr();
 	const size_t image_size = a38x_header_size(hdr);
@@ -701,11 +716,39 @@ static int check_image_header(void)
 	checksum = image_checksum8(hdr, image_size);
 	checksum -= hdr->checksum;
 	if (checksum != hdr->checksum) {
-		printf("Error: Bad A38x image checksum. 0x%x != 0x%x\n",
+		printf("Error: Bad A38x image header checksum. 0x%x != 0x%x\n",
 		       checksum, hdr->checksum);
 		return -ENOEXEC;
 	}
 
+	offset = le32_to_cpu(hdr->srcaddr);
+	size = le32_to_cpu(hdr->blocksize);
+
+	if (hdr->blockid == 0x78) { /* SATA id */
+		if (offset < 1) {
+			printf("Error: Bad A38x image srcaddr.\n");
+			return -ENOEXEC;
+		}
+		offset -= 1;
+		offset *= 512;
+	}
+
+	if (hdr->blockid == 0xAE) /* SDIO id */
+		offset *= 512;
+
+	if (offset % 4 != 0 || size < 4 || size % 4 != 0) {
+		printf("Error: Bad A38x image blocksize.\n");
+		return -ENOEXEC;
+	}
+
+	checksum32 = image_checksum32((u8 *)hdr + offset, size - 4);
+	exp_checksum32 = *(u32 *)((u8 *)hdr + offset + size - 4);
+	if (checksum32 != exp_checksum32) {
+		printf("Error: Bad A38x image data checksum. 0x%08x != 0x%08x\n",
+		       checksum32, exp_checksum32);
+		return -ENOEXEC;
+	}
+
 	printf("Image checksum...OK!\n");
 	return 0;
 }
-- 
2.20.1

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

[Pali Rohár]

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@kernel.org>

---
Changes in v2:
* Add missing dependency on MVEBU_EFUSE
* Use only for A38x and A37xx
---
 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)
-- 
2.20.1

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

[Stefan Roese]

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@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
> Changes in v2:
> * Add missing dependency on MVEBU_EFUSE
> * Use only for A38x and A37xx
> ---
>   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@denx.de

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

[Stefan Roese]

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@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?

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

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

[Pali Rohár]

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

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

[Stefan Roese]

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

Re: [PATCH v2 1/2] cmd: mvebu/bubt: Check for A38x image data checksum

[Stefan Roese]

On 09.08.22 21:42, Pali Rohár wrote:
> Currently for A38x image is checked only header checksum.
> So check also for image data checksum to prevent flashing broken image.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
> Changes in v2:
> * Compile fix (move code chunk from patch 2/2 to 1/2)
> ---
>   cmd/mvebu/bubt.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
> index 2136af64163c..3b6ffb7ffd1f 100644
> --- a/cmd/mvebu/bubt.c
> +++ b/cmd/mvebu/bubt.c
> @@ -688,9 +688,24 @@ static uint8_t image_checksum8(const void *start, size_t len)
>   	return csum;
>   }
>   
> +static uint32_t image_checksum32(const void *start, size_t len)
> +{
> +	u32 csum = 0;
> +	const u32 *p = start;
> +
> +	while (len) {
> +		csum += *p;
> +		++p;
> +		len -= sizeof(u32);
> +	}
> +
> +	return csum;
> +}
> +
>   static int check_image_header(void)
>   {
>   	u8 checksum;
> +	u32 checksum32, exp_checksum32;
>   	const struct a38x_main_hdr_v1 *hdr =
>   		(struct a38x_main_hdr_v1 *)get_load_addr();
>   	const size_t image_size = a38x_header_size(hdr);
> @@ -701,11 +716,39 @@ static int check_image_header(void)
>   	checksum = image_checksum8(hdr, image_size);
>   	checksum -= hdr->checksum;
>   	if (checksum != hdr->checksum) {
> -		printf("Error: Bad A38x image checksum. 0x%x != 0x%x\n",
> +		printf("Error: Bad A38x image header checksum. 0x%x != 0x%x\n",
>   		       checksum, hdr->checksum);
>   		return -ENOEXEC;
>   	}
>   
> +	offset = le32_to_cpu(hdr->srcaddr);
> +	size = le32_to_cpu(hdr->blocksize);
> +
> +	if (hdr->blockid == 0x78) { /* SATA id */
> +		if (offset < 1) {
> +			printf("Error: Bad A38x image srcaddr.\n");
> +			return -ENOEXEC;
> +		}
> +		offset -= 1;
> +		offset *= 512;
> +	}
> +
> +	if (hdr->blockid == 0xAE) /* SDIO id */
> +		offset *= 512;
> +
> +	if (offset % 4 != 0 || size < 4 || size % 4 != 0) {
> +		printf("Error: Bad A38x image blocksize.\n");
> +		return -ENOEXEC;
> +	}
> +
> +	checksum32 = image_checksum32((u8 *)hdr + offset, size - 4);
> +	exp_checksum32 = *(u32 *)((u8 *)hdr + offset + size - 4);
> +	if (checksum32 != exp_checksum32) {
> +		printf("Error: Bad A38x image data checksum. 0x%08x != 0x%08x\n",
> +		       checksum32, exp_checksum32);
> +		return -ENOEXEC;
> +	}
> +
>   	printf("Image checksum...OK!\n");
>   	return 0;
>   }

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

Re: [PATCH v2 1/2] cmd: mvebu/bubt: Check for A38x image data checksum

[Stefan Roese]

Hi Pali,

On 09.08.22 21:42, Pali Rohár wrote:
> Currently for A38x image is checked only header checksum.
> So check also for image data checksum to prevent flashing broken image.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> ---
> Changes in v2:
> * Compile fix (move code chunk from patch 2/2 to 1/2)
> ---
>   cmd/mvebu/bubt.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
> index 2136af64163c..3b6ffb7ffd1f 100644
> --- a/cmd/mvebu/bubt.c
> +++ b/cmd/mvebu/bubt.c
> @@ -688,9 +688,24 @@ static uint8_t image_checksum8(const void *start, size_t len)
>   	return csum;
>   }
>   
> +static uint32_t image_checksum32(const void *start, size_t len)
> +{
> +	u32 csum = 0;
> +	const u32 *p = start;
> +
> +	while (len) {
> +		csum += *p;
> +		++p;
> +		len -= sizeof(u32);
> +	}
> +
> +	return csum;
> +}
> +
>   static int check_image_header(void)
>   {
>   	u8 checksum;
> +	u32 checksum32, exp_checksum32;
>   	const struct a38x_main_hdr_v1 *hdr =
>   		(struct a38x_main_hdr_v1 *)get_load_addr();
>   	const size_t image_size = a38x_header_size(hdr);
> @@ -701,11 +716,39 @@ static int check_image_header(void)
>   	checksum = image_checksum8(hdr, image_size);
>   	checksum -= hdr->checksum;
>   	if (checksum != hdr->checksum) {
> -		printf("Error: Bad A38x image checksum. 0x%x != 0x%x\n",
> +		printf("Error: Bad A38x image header checksum. 0x%x != 0x%x\n",
>   		       checksum, hdr->checksum);
>   		return -ENOEXEC;
>   	}
>   
> +	offset = le32_to_cpu(hdr->srcaddr);
> +	size = le32_to_cpu(hdr->blocksize);

While running a world Azure CI build, I get these errors:

$ make clearfog_defconfig
$ make -sj
...
cmd/mvebu/bubt.c: In function 'check_image_header':
cmd/mvebu/bubt.c:724:9: error: 'offset' undeclared (first use in this 
function); did you mean 'off_t'?
   724 |         offset = le32_to_cpu(hdr->srcaddr);
       |         ^~~~~~
       |         off_t
cmd/mvebu/bubt.c:724:9: note: each undeclared identifier is reported 
only once for each function it appears in
cmd/mvebu/bubt.c:725:9: error: 'size' undeclared (first use in this 
function); did you mean 'size_t'?
   725 |         size = le32_to_cpu(hdr->blocksize);
       |         ^~~~
       |         size_t
make[2]: *** [scripts/Makefile.build:258: cmd/mvebu/bubt.o] Error 1
make[1]: *** [scripts/Makefile.build:398: cmd/mvebu] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1916: cmd] Error 2
make: *** Waiting for unfinished jobs....

Could you please take a look and fix these issues?

Thanks,
Stefan

> +
> +	if (hdr->blockid == 0x78) { /* SATA id */
> +		if (offset < 1) {
> +			printf("Error: Bad A38x image srcaddr.\n");
> +			return -ENOEXEC;
> +		}
> +		offset -= 1;
> +		offset *= 512;
> +	}
> +
> +	if (hdr->blockid == 0xAE) /* SDIO id */
> +		offset *= 512;
> +
> +	if (offset % 4 != 0 || size < 4 || size % 4 != 0) {
> +		printf("Error: Bad A38x image blocksize.\n");
> +		return -ENOEXEC;
> +	}
> +
> +	checksum32 = image_checksum32((u8 *)hdr + offset, size - 4);
> +	exp_checksum32 = *(u32 *)((u8 *)hdr + offset + size - 4);
> +	if (checksum32 != exp_checksum32) {
> +		printf("Error: Bad A38x image data checksum. 0x%08x != 0x%08x\n",
> +		       checksum32, exp_checksum32);
> +		return -ENOEXEC;
> +	}
> +
>   	printf("Image checksum...OK!\n");
>   	return 0;
>   }

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

Re: [PATCH v2 1/2] cmd: mvebu/bubt: Check for A38x image data checksum

[Pali Rohár]

On Tuesday 23 August 2022 12:28:48 Stefan Roese wrote:
> Hi Pali,
> 
> On 09.08.22 21:42, Pali Rohár wrote:
> > Currently for A38x image is checked only header checksum.
> > So check also for image data checksum to prevent flashing broken image.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > 
> > ---
> > Changes in v2:
> > * Compile fix (move code chunk from patch 2/2 to 1/2)
> > ---
> >   cmd/mvebu/bubt.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 44 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
> > index 2136af64163c..3b6ffb7ffd1f 100644
> > --- a/cmd/mvebu/bubt.c
> > +++ b/cmd/mvebu/bubt.c
> > @@ -688,9 +688,24 @@ static uint8_t image_checksum8(const void *start, size_t len)
> >   	return csum;
> >   }
> > +static uint32_t image_checksum32(const void *start, size_t len)
> > +{
> > +	u32 csum = 0;
> > +	const u32 *p = start;
> > +
> > +	while (len) {
> > +		csum += *p;
> > +		++p;
> > +		len -= sizeof(u32);
> > +	}
> > +
> > +	return csum;
> > +}
> > +
> >   static int check_image_header(void)
> >   {
> >   	u8 checksum;
> > +	u32 checksum32, exp_checksum32;
> >   	const struct a38x_main_hdr_v1 *hdr =
> >   		(struct a38x_main_hdr_v1 *)get_load_addr();
> >   	const size_t image_size = a38x_header_size(hdr);
> > @@ -701,11 +716,39 @@ static int check_image_header(void)
> >   	checksum = image_checksum8(hdr, image_size);
> >   	checksum -= hdr->checksum;
> >   	if (checksum != hdr->checksum) {
> > -		printf("Error: Bad A38x image checksum. 0x%x != 0x%x\n",
> > +		printf("Error: Bad A38x image header checksum. 0x%x != 0x%x\n",
> >   		       checksum, hdr->checksum);
> >   		return -ENOEXEC;
> >   	}
> > +	offset = le32_to_cpu(hdr->srcaddr);
> > +	size = le32_to_cpu(hdr->blocksize);
> 
> While running a world Azure CI build, I get these errors:
> 
> $ make clearfog_defconfig
> $ make -sj
> ...
> cmd/mvebu/bubt.c: In function 'check_image_header':
> cmd/mvebu/bubt.c:724:9: error: 'offset' undeclared (first use in this
> function); did you mean 'off_t'?
>   724 |         offset = le32_to_cpu(hdr->srcaddr);
>       |         ^~~~~~
>       |         off_t
> cmd/mvebu/bubt.c:724:9: note: each undeclared identifier is reported only
> once for each function it appears in
> cmd/mvebu/bubt.c:725:9: error: 'size' undeclared (first use in this
> function); did you mean 'size_t'?
>   725 |         size = le32_to_cpu(hdr->blocksize);
>       |         ^~~~
>       |         size_t
> make[2]: *** [scripts/Makefile.build:258: cmd/mvebu/bubt.o] Error 1
> make[1]: *** [scripts/Makefile.build:398: cmd/mvebu] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1916: cmd] Error 2
> make: *** Waiting for unfinished jobs....
> 
> Could you please take a look and fix these issues?

It took me some time to find where is the issue, because I was not able
to reproduce it. But finally I was able to trigger it. The issue is that
changes are incorrectly split into patch 1/2 and 2/2. So if you apply
both patches, there is no issue.

I will try to fix it and send a v3.

> Thanks,
> Stefan
> 
> > +
> > +	if (hdr->blockid == 0x78) { /* SATA id */
> > +		if (offset < 1) {
> > +			printf("Error: Bad A38x image srcaddr.\n");
> > +			return -ENOEXEC;
> > +		}
> > +		offset -= 1;
> > +		offset *= 512;
> > +	}
> > +
> > +	if (hdr->blockid == 0xAE) /* SDIO id */
> > +		offset *= 512;
> > +
> > +	if (offset % 4 != 0 || size < 4 || size % 4 != 0) {
> > +		printf("Error: Bad A38x image blocksize.\n");
> > +		return -ENOEXEC;
> > +	}
> > +
> > +	checksum32 = image_checksum32((u8 *)hdr + offset, size - 4);
> > +	exp_checksum32 = *(u32 *)((u8 *)hdr + offset + size - 4);
> > +	if (checksum32 != exp_checksum32) {
> > +		printf("Error: Bad A38x image data checksum. 0x%08x != 0x%08x\n",
> > +		       checksum32, exp_checksum32);
> > +		return -ENOEXEC;
> > +	}
> > +
> >   	printf("Image checksum...OK!\n");
> >   	return 0;
> >   }
> 
> 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@denx.de

[PATCH v3 1/2] cmd: mvebu/bubt: Check for A38x image data checksum

[Pali Rohár]

Currently for A38x image is checked only header checksum.
So check also for image data checksum to prevent flashing broken image.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Stefan Roese <sr@denx.de>

---
Changes in v3:
* Compile fix (move another code chunk from patch 2/2 to 1/2)

Changes in v2:
* Compile fix (move code chunk from patch 2/2 to 1/2)
---
 cmd/mvebu/bubt.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
index 2136af64163c..a97e5ce38a5e 100644
--- a/cmd/mvebu/bubt.c
+++ b/cmd/mvebu/bubt.c
@@ -688,9 +688,25 @@ static uint8_t image_checksum8(const void *start, size_t len)
 	return csum;
 }
 
+static uint32_t image_checksum32(const void *start, size_t len)
+{
+	u32 csum = 0;
+	const u32 *p = start;
+
+	while (len) {
+		csum += *p;
+		++p;
+		len -= sizeof(u32);
+	}
+
+	return csum;
+}
+
 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);
@@ -701,11 +717,39 @@ static int check_image_header(void)
 	checksum = image_checksum8(hdr, image_size);
 	checksum -= hdr->checksum;
 	if (checksum != hdr->checksum) {
-		printf("Error: Bad A38x image checksum. 0x%x != 0x%x\n",
+		printf("Error: Bad A38x image header checksum. 0x%x != 0x%x\n",
 		       checksum, hdr->checksum);
 		return -ENOEXEC;
 	}
 
+	offset = le32_to_cpu(hdr->srcaddr);
+	size = le32_to_cpu(hdr->blocksize);
+
+	if (hdr->blockid == 0x78) { /* SATA id */
+		if (offset < 1) {
+			printf("Error: Bad A38x image srcaddr.\n");
+			return -ENOEXEC;
+		}
+		offset -= 1;
+		offset *= 512;
+	}
+
+	if (hdr->blockid == 0xAE) /* SDIO id */
+		offset *= 512;
+
+	if (offset % 4 != 0 || size < 4 || size % 4 != 0) {
+		printf("Error: Bad A38x image blocksize.\n");
+		return -ENOEXEC;
+	}
+
+	checksum32 = image_checksum32((u8 *)hdr + offset, size - 4);
+	exp_checksum32 = *(u32 *)((u8 *)hdr + offset + size - 4);
+	if (checksum32 != exp_checksum32) {
+		printf("Error: Bad A38x image data checksum. 0x%08x != 0x%08x\n",
+		       checksum32, exp_checksum32);
+		return -ENOEXEC;
+	}
+
 	printf("Image checksum...OK!\n");
 	return 0;
 }
-- 
2.20.1

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

[Pali Rohár]

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@kernel.org>
Reviewed-by: Stefan Roese <sr@denx.de>

---
Changes in v3:
* Guard fuse_read_u64() function

Changes in v2:
* Add missing dependency on MVEBU_EFUSE
* Use only for A38x and A37xx
---
 cmd/mvebu/Kconfig |   1 +
 cmd/mvebu/bubt.c  | 128 +++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 121 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 a97e5ce38a5e..7e6e47f40d6e 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;
@@ -753,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)
 {
@@ -761,20 +806,60 @@ static int check_image_header(void)
 }
 #endif
 
+#if defined(CONFIG_ARMADA_3700) || defined(CONFIG_ARMADA_32BIT)
+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];
+}
+#endif
+
+#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++) {
@@ -787,15 +872,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)
-- 
2.20.1

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

[Stefan Roese]

On 23.08.22 14:52, 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@kernel.org>
> Reviewed-by: Stefan Roese <sr@denx.de>

Applied to u-boot-marvell/master

Thanks,
Stefan


> ---
> Changes in v3:
> * Guard fuse_read_u64() function
> 
> Changes in v2:
> * Add missing dependency on MVEBU_EFUSE
> * Use only for A38x and A37xx
> ---
>   cmd/mvebu/Kconfig |   1 +
>   cmd/mvebu/bubt.c  | 128 +++++++++++++++++++++++++++++++++++++++++++---
>   2 files changed, 121 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 a97e5ce38a5e..7e6e47f40d6e 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;
> @@ -753,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)
>   {
> @@ -761,20 +806,60 @@ static int check_image_header(void)
>   }
>   #endif
>   
> +#if defined(CONFIG_ARMADA_3700) || defined(CONFIG_ARMADA_32BIT)
> +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];
> +}
> +#endif
> +
> +#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++) {
> @@ -787,15 +872,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@denx.de

Re: [PATCH v3 1/2] cmd: mvebu/bubt: Check for A38x image data checksum

[Pali Rohár]

On Tuesday 23 August 2022 14:52:23 Pali Rohár wrote:
> Currently for A38x image is checked only header checksum.
> So check also for image data checksum to prevent flashing broken image.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Stefan Roese <sr@denx.de>
> 
> ---
> Changes in v3:
> * Compile fix (move another code chunk from patch 2/2 to 1/2)
> 
> Changes in v2:
> * Compile fix (move code chunk from patch 2/2 to 1/2)
> ---

Hello Stefan! Is V3 version OK now?

>  cmd/mvebu/bubt.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
> index 2136af64163c..a97e5ce38a5e 100644
> --- a/cmd/mvebu/bubt.c
> +++ b/cmd/mvebu/bubt.c
> @@ -688,9 +688,25 @@ static uint8_t image_checksum8(const void *start, size_t len)
>  	return csum;
>  }
>  
> +static uint32_t image_checksum32(const void *start, size_t len)
> +{
> +	u32 csum = 0;
> +	const u32 *p = start;
> +
> +	while (len) {
> +		csum += *p;
> +		++p;
> +		len -= sizeof(u32);
> +	}
> +
> +	return csum;
> +}
> +
>  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);
> @@ -701,11 +717,39 @@ static int check_image_header(void)
>  	checksum = image_checksum8(hdr, image_size);
>  	checksum -= hdr->checksum;
>  	if (checksum != hdr->checksum) {
> -		printf("Error: Bad A38x image checksum. 0x%x != 0x%x\n",
> +		printf("Error: Bad A38x image header checksum. 0x%x != 0x%x\n",
>  		       checksum, hdr->checksum);
>  		return -ENOEXEC;
>  	}
>  
> +	offset = le32_to_cpu(hdr->srcaddr);
> +	size = le32_to_cpu(hdr->blocksize);
> +
> +	if (hdr->blockid == 0x78) { /* SATA id */
> +		if (offset < 1) {
> +			printf("Error: Bad A38x image srcaddr.\n");
> +			return -ENOEXEC;
> +		}
> +		offset -= 1;
> +		offset *= 512;
> +	}
> +
> +	if (hdr->blockid == 0xAE) /* SDIO id */
> +		offset *= 512;
> +
> +	if (offset % 4 != 0 || size < 4 || size % 4 != 0) {
> +		printf("Error: Bad A38x image blocksize.\n");
> +		return -ENOEXEC;
> +	}
> +
> +	checksum32 = image_checksum32((u8 *)hdr + offset, size - 4);
> +	exp_checksum32 = *(u32 *)((u8 *)hdr + offset + size - 4);
> +	if (checksum32 != exp_checksum32) {
> +		printf("Error: Bad A38x image data checksum. 0x%08x != 0x%08x\n",
> +		       checksum32, exp_checksum32);
> +		return -ENOEXEC;
> +	}
> +
>  	printf("Image checksum...OK!\n");
>  	return 0;
>  }
> -- 
> 2.20.1
> 

Re: [PATCH v3 1/2] cmd: mvebu/bubt: Check for A38x image data checksum

[Stefan Roese]

On 11.09.22 13:26, Pali Rohár wrote:
> On Tuesday 23 August 2022 14:52:23 Pali Rohár wrote:
>> Currently for A38x image is checked only header checksum.
>> So check also for image data checksum to prevent flashing broken image.
>>
>> Signed-off-by: Pali Rohár <pali@kernel.org>
>> Reviewed-by: Stefan Roese <sr@denx.de>
>>
>> ---
>> Changes in v3:
>> * Compile fix (move another code chunk from patch 2/2 to 1/2)
>>
>> Changes in v2:
>> * Compile fix (move code chunk from patch 2/2 to 1/2)
>> ---
> 
> Hello Stefan! Is V3 version OK now?

Looks good.

Thanks,
Stefan

>>   cmd/mvebu/bubt.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 45 insertions(+), 1 deletion(-)
>>
>> diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
>> index 2136af64163c..a97e5ce38a5e 100644
>> --- a/cmd/mvebu/bubt.c
>> +++ b/cmd/mvebu/bubt.c
>> @@ -688,9 +688,25 @@ static uint8_t image_checksum8(const void *start, size_t len)
>>   	return csum;
>>   }
>>   
>> +static uint32_t image_checksum32(const void *start, size_t len)
>> +{
>> +	u32 csum = 0;
>> +	const u32 *p = start;
>> +
>> +	while (len) {
>> +		csum += *p;
>> +		++p;
>> +		len -= sizeof(u32);
>> +	}
>> +
>> +	return csum;
>> +}
>> +
>>   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);
>> @@ -701,11 +717,39 @@ static int check_image_header(void)
>>   	checksum = image_checksum8(hdr, image_size);
>>   	checksum -= hdr->checksum;
>>   	if (checksum != hdr->checksum) {
>> -		printf("Error: Bad A38x image checksum. 0x%x != 0x%x\n",
>> +		printf("Error: Bad A38x image header checksum. 0x%x != 0x%x\n",
>>   		       checksum, hdr->checksum);
>>   		return -ENOEXEC;
>>   	}
>>   
>> +	offset = le32_to_cpu(hdr->srcaddr);
>> +	size = le32_to_cpu(hdr->blocksize);
>> +
>> +	if (hdr->blockid == 0x78) { /* SATA id */
>> +		if (offset < 1) {
>> +			printf("Error: Bad A38x image srcaddr.\n");
>> +			return -ENOEXEC;
>> +		}
>> +		offset -= 1;
>> +		offset *= 512;
>> +	}
>> +
>> +	if (hdr->blockid == 0xAE) /* SDIO id */
>> +		offset *= 512;
>> +
>> +	if (offset % 4 != 0 || size < 4 || size % 4 != 0) {
>> +		printf("Error: Bad A38x image blocksize.\n");
>> +		return -ENOEXEC;
>> +	}
>> +
>> +	checksum32 = image_checksum32((u8 *)hdr + offset, size - 4);
>> +	exp_checksum32 = *(u32 *)((u8 *)hdr + offset + size - 4);
>> +	if (checksum32 != exp_checksum32) {
>> +		printf("Error: Bad A38x image data checksum. 0x%08x != 0x%08x\n",
>> +		       checksum32, exp_checksum32);
>> +		return -ENOEXEC;
>> +	}
>> +
>>   	printf("Image checksum...OK!\n");
>>   	return 0;
>>   }
>> -- 
>> 2.20.1
>>

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

Re: [PATCH v3 1/2] cmd: mvebu/bubt: Check for A38x image data checksum

[Stefan Roese]

On 23.08.22 14:52, Pali Rohár wrote:
> Currently for A38x image is checked only header checksum.
> So check also for image data checksum to prevent flashing broken image.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Stefan Roese <sr@denx.de>

Applied to u-boot-marvell/master

Thanks,
Stefan


> ---
> Changes in v3:
> * Compile fix (move another code chunk from patch 2/2 to 1/2)
> 
> Changes in v2:
> * Compile fix (move code chunk from patch 2/2 to 1/2)
> ---
>   cmd/mvebu/bubt.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
> index 2136af64163c..a97e5ce38a5e 100644
> --- a/cmd/mvebu/bubt.c
> +++ b/cmd/mvebu/bubt.c
> @@ -688,9 +688,25 @@ static uint8_t image_checksum8(const void *start, size_t len)
>   	return csum;
>   }
>   
> +static uint32_t image_checksum32(const void *start, size_t len)
> +{
> +	u32 csum = 0;
> +	const u32 *p = start;
> +
> +	while (len) {
> +		csum += *p;
> +		++p;
> +		len -= sizeof(u32);
> +	}
> +
> +	return csum;
> +}
> +
>   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);
> @@ -701,11 +717,39 @@ static int check_image_header(void)
>   	checksum = image_checksum8(hdr, image_size);
>   	checksum -= hdr->checksum;
>   	if (checksum != hdr->checksum) {
> -		printf("Error: Bad A38x image checksum. 0x%x != 0x%x\n",
> +		printf("Error: Bad A38x image header checksum. 0x%x != 0x%x\n",
>   		       checksum, hdr->checksum);
>   		return -ENOEXEC;
>   	}
>   
> +	offset = le32_to_cpu(hdr->srcaddr);
> +	size = le32_to_cpu(hdr->blocksize);
> +
> +	if (hdr->blockid == 0x78) { /* SATA id */
> +		if (offset < 1) {
> +			printf("Error: Bad A38x image srcaddr.\n");
> +			return -ENOEXEC;
> +		}
> +		offset -= 1;
> +		offset *= 512;
> +	}
> +
> +	if (hdr->blockid == 0xAE) /* SDIO id */
> +		offset *= 512;
> +
> +	if (offset % 4 != 0 || size < 4 || size % 4 != 0) {
> +		printf("Error: Bad A38x image blocksize.\n");
> +		return -ENOEXEC;
> +	}
> +
> +	checksum32 = image_checksum32((u8 *)hdr + offset, size - 4);
> +	exp_checksum32 = *(u32 *)((u8 *)hdr + offset + size - 4);
> +	if (checksum32 != exp_checksum32) {
> +		printf("Error: Bad A38x image data checksum. 0x%08x != 0x%08x\n",
> +		       checksum32, exp_checksum32);
> +		return -ENOEXEC;
> +	}
> +
>   	printf("Image checksum...OK!\n");
>   	return 0;
>   }

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