DRAFT: flash: espressif: erase flash region before writing when flash encryption is enabled

drivers/flash/flash_esp32.c

@@ -34,6 +34,32 @@
 
 #define FLASH_SEM_TIMEOUT (k_is_in_isr() ? K_NO_WAIT : K_FOREVER)
 
+#ifdef CONFIG_EFUSE_VIRTUAL_KEEP_IN_FLASH
+#define ENCRYPTION_IS_VIRTUAL (!efuse_hal_flash_encryption_enabled())
+#else
+#define ENCRYPTION_IS_VIRTUAL 0
+#endif
+
+#ifndef MIN
+#  define MIN(a, b)                 (((a) < (b)) ? (a) : (b))
+#endif
+
+#ifndef ALIGN_UP
+#  define ALIGN_UP(num, align)      (((num) + ((align) - 1)) & ~((align) - 1))
+#endif
+
+#ifndef ALIGN_DOWN
+#  define ALIGN_DOWN(num, align)    ((num) & ~((align) - 1))
+#endif
+
+#ifndef ALIGN_OFFSET
+#  define ALIGN_OFFSET(num, align)  ((num) & ((align) - 1))
+#endif
+
+#ifndef IS_ALIGNED
+#  define IS_ALIGNED(num, align)    (ALIGN_OFFSET((num), (align)) == 0)
+#endif
+
 struct flash_esp32_dev_config {
 	spi_dev_t *controller;
 };
@@ -76,6 +102,8 @@
 #include <stdint.h>
 #include <string.h>
 
+static bool aligned_flash_erase(const struct device *dev, size_t addr, size_t size);
+
 #ifdef CONFIG_MCUBOOT
 #define READ_BUFFER_SIZE 32
 static bool flash_esp32_is_aligned(off_t address, void *buffer, size_t length)
@@ -85,6 +113,46 @@
 }
 #endif
 
+bool aligned_flash_read_(off_t address, void *buffer, size_t length, bool read_encrypted)
+{
+	int ret = 0;
+
+	if (read_encrypted) {
+		LOG_INF("Flash read ENCRYPTED - address 0x%lx size 0x%x", address, length);
+		ret = esp_flash_read_encrypted(NULL, address, buffer, length);
+	} else {
+		LOG_INF("Flash read RAW - address 0x%lx size 0x%x", address, length);
+		ret = esp_flash_read(NULL, buffer, address, length);
+	}
+
+	if (ret != 0) {
+		LOG_ERR("Flash read error: %d", ret);
+		return false;
+	}
+
+	return true;
+}
+
+static int flash_esp32_read_check_enc(off_t address, void *buffer, size_t length)
+{
+	int ret = 0;
+
+	if (esp_flash_encryption_enabled()) {
+		LOG_DBG("Flash read ENCRYPTED - address 0x%lx size 0x%x", address, length);
+		ret = esp_flash_read_encrypted(NULL, address, buffer, length);
+	} else {
+		LOG_DBG("Flash read RAW - address 0x%lx size 0x%x", address, length);
+		ret = esp_flash_read(NULL, buffer, address, length);
+	}
+
+	if (ret != 0) {
+		LOG_ERR("Flash read error: %d", ret);
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static int flash_esp32_read(const struct device *dev, off_t address, void *buffer, size_t length)
 {
 	int ret = 0;
@@ -139,11 +207,7 @@
 #else
 	flash_esp32_sem_take(dev);
 
-	if (esp_flash_encryption_enabled()) {
-		ret = esp_flash_read_encrypted(NULL, address, buffer, length);
-	} else {
-		ret = esp_flash_read(NULL, buffer, address, length);
-	}
+	ret = flash_esp32_read_check_enc(address, buffer, length);
 
 	flash_esp32_sem_give(dev);
 #endif
@@ -156,6 +220,26 @@
 	return 0;
 }
 
+static int flash_esp32_write_check_enc(off_t address, const void *buffer, size_t length)
+{
+	int ret = 0;
+
+	if (esp_flash_encryption_enabled() && !ENCRYPTION_IS_VIRTUAL) {
+		LOG_DBG("Flash write ENCRYPTED - address 0x%lx size 0x%x", address, length);
+		ret = esp_flash_write_encrypted(NULL, address, buffer, length);
+	} else {
+		LOG_DBG("Flash write RAW - address 0x%lx size 0x%x", address, length);
+		ret = esp_flash_write(NULL, buffer, address, length);
+	}
+
+	if (ret != 0) {
+		LOG_ERR("Flash write error: %d", ret);
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static int flash_esp32_write(const struct device *dev,
 			     off_t address,
 			     const void *buffer,
@@ -176,9 +260,17 @@
 	flash_esp32_sem_take(dev);
 
 	if (esp_flash_encryption_enabled()) {
-		ret = esp_flash_write_encrypted(NULL, address, buffer, length);
-	} else {
-		ret = esp_flash_write(NULL, buffer, address, length);
+		/* Ensuring flash region has been erased before writing in order to
+		 * avoid inconsistences when hardware flash encryption is enabled.
+		 */
+		if (!aligned_flash_erase(dev, address, length)) {
+			LOG_ERR("%s: Flash erase before write failed", __func__);
+			ret = -1;
+		}
+	}
+
+	if (ret == 0) {
+		ret = flash_esp32_write_check_enc(address, buffer, length);
 	}
 
 	flash_esp32_sem_give(dev);
@@ -192,6 +284,73 @@
 	return 0;
 }
 
+/* Auxiliar buffer to store the sector that will be partially erased */
+static uint8_t __aligned(4) write_data[FLASH_SECTOR_SIZE] = {0};
+
+static bool aligned_flash_erase(const struct device *dev, size_t addr, size_t size)
+{
+	if (IS_ALIGNED(addr, FLASH_SECTOR_SIZE) && IS_ALIGNED(size, FLASH_SECTOR_SIZE)) {
+		/* A single erase operation is enough when all parameters are aligned */
+
+		return esp_flash_erase_region(NULL, addr, size) == ESP_OK;
+	}
+
+	const uint32_t aligned_addr = ALIGN_DOWN(addr, FLASH_SECTOR_SIZE);
+	const uint32_t addr_offset = ALIGN_OFFSET(addr, FLASH_SECTOR_SIZE);
+	uint32_t bytes_remaining = size;
+
+	/* Perform a read operation considering an offset not aligned to 4-byte boundary */
+
+	uint32_t bytes = MIN(bytes_remaining + addr_offset, sizeof(write_data));
+	if (flash_esp32_read_check_enc(aligned_addr, write_data, ALIGN_UP(bytes, FLASH_SECTOR_SIZE)) != ESP_OK) {
+		return false;
+	}
+
+	if (esp_flash_erase_region(NULL, aligned_addr, ALIGN_UP(bytes, FLASH_SECTOR_SIZE)) != ESP_OK) {
+		LOG_ERR("%s: Flash erase failed", __func__);
+		return -1;
+	}
+
+	uint32_t bytes_written = bytes - addr_offset;
+
+	/* Write first part of non-erased data */
+	if (addr_offset > 0) {
+		flash_esp32_write_check_enc(aligned_addr, write_data, addr_offset);
+	}
+
+	if (bytes < sizeof(write_data)) {
+		flash_esp32_write_check_enc(aligned_addr + bytes, write_data + bytes, sizeof(write_data) - bytes);
+	}
+
+	bytes_remaining -= bytes_written;
+
+	/* Write remaining data to Flash if any */
+
+	uint32_t offset = bytes;
+
+	while (bytes_remaining != 0) {
+		bytes = MIN(bytes_remaining, sizeof(write_data));
+		if (flash_esp32_read_check_enc(aligned_addr + offset, write_data, ALIGN_UP(bytes, FLASH_SECTOR_SIZE)) != ESP_OK) {
+			return false;
+		}
+
+		if (esp_flash_erase_region(NULL, aligned_addr + offset, ALIGN_UP(bytes, FLASH_SECTOR_SIZE)) != ESP_OK) {
+			LOG_ERR("%s: Flash erase failed", __func__);
+			return -1;
+		}
+
+		if(bytes < sizeof(write_data)) {
+			flash_esp32_write_check_enc(aligned_addr + offset + bytes, write_data + bytes, sizeof(write_data) - bytes);
+		}
+
+		offset += bytes;
+		bytes_written += bytes;
+		bytes_remaining -= bytes;
+	}
+
+	return true;
+}
+
 static int flash_esp32_erase(const struct device *dev, off_t start, size_t len)
 {
 	int ret = 0;
@@ -200,7 +359,20 @@
 	ret = esp_rom_flash_erase_range(start, len);
 #else
 	flash_esp32_sem_take(dev);
-	ret = esp_flash_erase_region(NULL, start, len);
+
+	if ((len % FLASH_SECTOR_SIZE) != 0 || (start % FLASH_SECTOR_SIZE) != 0) {
+		LOG_DBG("%s: Not aligned on sector Offset: 0x%x Length: 0x%x",
+					 __func__, (int)start, (int)len);
+
+		if(!aligned_flash_erase(dev, start, len)) {
+			ret = -EIO;
+		}
+	} else {
+		LOG_DBG("%s: Aligned Addr: 0x%08x Length: %d", __func__, (int)start, (int)len);
+
+		ret = esp_flash_erase_region(NULL, start, len);
+	}
+
 	flash_esp32_sem_give(dev);
 #endif
 	if (ret != 0) {

samples/hello_world/Kconfig

@@ -0,0 +1,5 @@
+config IMAGE_NAME
+	string "Image name"
+	default "Default"
+
+source "Kconfig.zephyr"

samples/hello_world/boards/esp32c3_devkitc.overlay

@@ -0,0 +1,21 @@
+
+/ {
+	chosen {
+		zephyr,console = &uart0;
+		zephyr,shell-uart = &uart0;
+		zephyr,code-partition = &slot0_partition;
+		zephyr,uart-mcumgr = &usb_serial;
+	};
+};
+
+&uart0 {
+	current-speed = <115200>;
+};
+
+&usb_serial {
+	status = "okay";
+};
+
+&flash0 {
+       write-block-size = <32>;
+};

samples/hello_world/boards/esp32s3_devkitc_procpu.overlay

@@ -0,0 +1,21 @@
+
+/ {
+	chosen {
+		zephyr,console = &uart0;
+		zephyr,shell-uart = &uart0;
+		zephyr,code-partition = &slot0_partition;
+		zephyr,uart-mcumgr = &usb_serial;
+	};
+};
+
+&uart0 {
+	current-speed = <115200>;
+};
+
+&usb_serial {
+	status = "okay";
+};
+
+&flash0 {
+       write-block-size = <32>;
+};

samples/hello_world/prj.conf

@@ -1 +1,52 @@
-# nothing here
+# Ensure an MCUboot-compatible binary is generated.
+CONFIG_BOOTLOADER_MCUBOOT=y
+
+# Enable MCUmgr and dependencies.
+CONFIG_NET_BUF=y
+CONFIG_ZCBOR=y
+CONFIG_CRC=y
+CONFIG_MCUMGR=y
+CONFIG_STREAM_FLASH=y
+CONFIG_FLASH_MAP=y
+
+# Enable flash operations.
+CONFIG_FLASH=y
+
+# Enable most core commands.
+CONFIG_IMG_MANAGER=y
+CONFIG_MCUMGR_GRP_IMG=y
+CONFIG_MCUMGR_GRP_OS=y
+CONFIG_MCUMGR_GRP_STAT=y
+CONFIG_STATS=y
+
+# Enable the serial MCUmgr transport.
+CONFIG_MCUMGR_TRANSPORT_UART=y
+CONFIG_CONSOLE=y
+
+# Enable the shell MCUmgr transport.
+CONFIG_BASE64=y
+CONFIG_CRC=y
+CONFIG_SHELL=y
+CONFIG_SHELL_BACKEND_SERIAL=y
+CONFIG_MCUMGR_TRANSPORT_SHELL=y
+
+# mcumgr-cli application doesn't accepts log in the channel it uses
+CONFIG_SHELL_LOG_BACKEND=n
+
+CONFIG_LOG=y
+CONFIG_LOG_MODE_MINIMAL=y
+# CONFIG_LOG_MODE_IMMEDIATE=y
+# CONFIG_LOG_MODE_DEFERRED=y
+# CONFIG_LOG_BUFFER_SIZE=4096
+CONFIG_MCUBOOT_UTIL_LOG_LEVEL_INF=y
+# CONFIG_MCUBOOT_UTIL_LOG_LEVEL_DBG=y
+CONFIG_MCUMGR_LOG_LEVEL_INF=y
+# CONFIG_FLASH_LOG_LEVEL_DBG=y
+CONFIG_FLASH_LOG_LEVEL_INF=y
+CONFIG_LOG_DEFAULT_LEVEL=3
+
+# Disable debug logging
+# CONFIG_EFUSE_VIRTUAL=y
+# CONFIG_EFUSE_VIRTUAL_KEEP_IN_FLASH=y
+
+CONFIG_MCUBOOT_EXTRA_IMGTOOL_ARGS="--align 32 --max-align 32 --pad --pad-sig"

samples/hello_world/src/main.c

@@ -9,6 +9,7 @@
 int main(void)
 {
 	printf("Hello World! %s\n", CONFIG_BOARD_TARGET);
++	printf("Image: %s\n", CONFIG_IMAGE_NAME);
 
 	return 0;
 }

samples/subsys/mgmt/mcumgr/smp_svr/boards/esp32s3_devkitm_procpu.overlay

@@ -0,0 +1,4 @@
+&flash0 {
+       write-block-size = <32>;
+};
+

samples/subsys/mgmt/mcumgr/smp_svr/overlay-bt.conf

@@ -14,7 +14,7 @@ CONFIG_MCUMGR_TRANSPORT_BT_CONN_PARAM_CONTROL=y
 # Enable the Shell mcumgr transport.
 CONFIG_BASE64=y
 CONFIG_CRC=y
-CONFIG_SHELL=y
+# CONFIG_SHELL=y
 CONFIG_SHELL_BACKEND_SERIAL=y
 CONFIG_MCUMGR_TRANSPORT_SHELL=y
 

samples/subsys/mgmt/mcumgr/smp_svr/prj.conf

@@ -8,7 +8,7 @@ CONFIG_FLASH_MAP=y
 
 # Some command handlers require a large stack.
 CONFIG_SYSTEM_WORKQUEUE_STACK_SIZE=2304
-CONFIG_MAIN_STACK_SIZE=2048
+CONFIG_MAIN_STACK_SIZE=8192
 
 # Ensure an MCUboot-compatible binary is generated.
 CONFIG_BOOTLOADER_MCUBOOT=y
@@ -35,7 +35,15 @@ CONFIG_MCUMGR_GRP_STAT=y
 
 # Enable logging
 CONFIG_LOG=y
-CONFIG_MCUBOOT_UTIL_LOG_LEVEL_WRN=y
+CONFIG_MCUBOOT_UTIL_LOG_LEVEL_INF=y
+CONFIG_LOG_MODE_IMMEDIATE=y
+# CONFIG_LOG_BUFFER_SIZE=4096
 
 # Disable debug logging
 CONFIG_LOG_MAX_LEVEL=3
+
+CONFIG_REBOOT=y
+
+# Ensure asset is aligned and padded correctly for flash encryption.
+CONFIG_MCUBOOT_EXTRA_IMGTOOL_ARGS="--align 32 --max-align 32 --pad --pad-sig"
+CONFIG_MCUBOOT_GENERATE_CONFIRMED_IMAGE=y