Bug report: 2nd-stage bootloader uses incorrect int32 / uint32 conversion

[code4sex]

BUG REPORT
IDF: 5.0.1 (possibly later releases also affected)

ABSTRACT: 2nd-stage bootloader (BL hereinafter) incorrectly treats the offset of the partition table (PT hereinafter) as uint32_t whereas it should be int32_t.

Description:
ESP-IDF says the flash region 0x0000...0x1000 is used by the Secure Boot stuff. If Secure Boot is not used then this space remains vacant. IDF docs DO NOT IMPLICITLY discourage users from using this space. Given that, a user can relocate a PT to 0x0000 and feel happy. However, under certain conditions severe problems are observed:

ISSUE: Flashing a BL (compiled with "Flash Encryption" enabled) on an unencrypted system instead of encryption results in the system failure.

Sample partition layout:
0x0000 - 0x1000 = PT
0x1000 - 0x8000 = BL
0x20000 - 0x50000 = ota0
0x50000 - 0x80000 = ota1 (not flashed, left as is)
0x80000 - 0x83000 = otadata_initial

Here is the boot log:

ets Jul 29 2019 12:21:46

rst:0x3 (SW_RESET),boot:0x13 (SPI_FAST_FLASH_BOOT)
configsip: 0, SPIWP:0xee
clk_drv:0x00,q_drv:0x00,d_drv:0x00,cs0_drv:0x00,hd_drv:0x00,wp_drv:0x00
mode:DIO, clock div:1
load:0x3fff00b8,len:11928
load:0x40078000,len:21792
load:0x40080400,len:4208
entry 0x4008066c
I (53) boot: ESP-IDF v5.0.1-dirty 2nd stage bootloader
I (53) boot: compile time 01:00:06
I (53) boot: chip revision: v3.1
I (57) qio_mode: Enabling QIO for flash chip WinBond
I (63) boot.esp32: SPI Speed : 80MHz
I (67) boot.esp32: SPI Mode : QIO
I (72) boot.esp32: SPI Flash Size : 4MB
I (76) boot: Enabling RNG early entropy source...
I (82) boot: Partition Table:
I (85) boot: ## Label Usage Type ST Offset Length
I (93) boot: 0 ota0 OTA app 00 10 00020000 00030000
I (100) boot: 1 ota1 OTA app 00 11 00050000 00030000
I (108) boot: 2 otadata OTA data 01 00 00080000 00002000
I (115) boot: End of partition table
I (120) esp_image: segment 0: paddr=00020020 vaddr=3f400020 size=07a30h ( 31280) map
I (132) esp_image: segment 1: paddr=00027a58 vaddr=3ffb0000 size=01d3ch ( 7484) load
I (138) esp_image: segment 2: paddr=0002979c vaddr=40080000 size=0687ch ( 26748) load
I (150) esp_image: segment 3: paddr=00030020 vaddr=400d0020 size=145cch ( 83404) map
I (163) esp_image: segment 4: paddr=000445f4 vaddr=4008687c size=03e3ch ( 15932) load
I (172) boot: Loaded app from partition at offset 0x20000
I (172) boot: Checking flash encryption...
I (173) efuse: Batch mode of writing fields is enabled
I (179) flash_encrypt: Using pre-loaded flash encryption key in efuse
I (186) flash_encrypt: Setting CRYPT_CONFIG efuse to 0xF
W (192) flash_encrypt: Not disabling UART bootloader encryption
I (198) flash_encrypt: Disable UART bootloader decryption...
I (205) flash_encrypt: Disable UART bootloader MMU cache...
I (211) flash_encrypt: Disable JTAG...
I (215) flash_encrypt: Disable ROM BASIC interpreter fallback...
I (222) efuse: Batch mode. Prepared fields are committed
E (228) esp_image: partition size 0xfffff000 invalid, larger than 16MB
W (235) flash_encrypt: no valid bootloader was found
E (241) boot: Encryption flash contents failed (261)
E (246) boot: OTA app partition slot 0 is not bootable
E (252) esp_image: image at 0x50000 has invalid magic byte (nothing flashed here?)
E (260) boot: OTA app partition slot 1 is not bootable
E (266) boot: No bootable app partitions in the partition table


The BL represents the physical address of PT as the sum: (0x1000 + OFFSET). The value of OFFSET is hard-coded in the header structure @ 0x0090 (=144) offset.
Since the PT is successfully relocated to 0x0000 the OFFSET becomes a negative value of -4096. This is a SIGNED 32-bit value = 0xFFFFF000.

However, BL internals mistakenly take this SIGNED value as UNSIGNED and come up with a wrong computation of partition size (esp_partition_pos_t::size) and pass this wrong value to a bunch of functions in "ESP-IDF\components\bootloader_support\src\esp_image_format.c".

Finally, we get a call to "image_load(mode, *part, *data)" that ends up with an error:

"E (228) esp_image: partition size 0xfffff000 invalid, larger than 16MB"

Code: Select all

if (part->size > SIXTEEN_MB) {
	err = ESP_ERR_INVALID_ARG;
	FAIL_LOAD("partition size 0x%x invalid, larger than 16MB", part->size);
	}

PROPOSED SOLUTIONS:

(a) The ASSUMPTION that PT resides AFTER BL might not always be true. Therefore, there is no place for assumptions in the code! Explicit checks required.
(b) Correct data type convertion should be used: int32_t instead of uint32_t.

* * *


PS: In order to generate a relocated to 0x0000 PT app a user needs to "hack" a little into 2 script files: "check_sizes.py" and "gen_esp32part.py" and skip some awkward overlapping checks. After that one gets all the BIN files generated as usual without any errors.
Alternatively, an app can generate and flash the PT at any desired address while running. In this case a BL needs to be updated. A simple way is to generate it with the conventional .py script tools and flash it.
The hard way is to edit the BL in place. This also requires updating the check sum so that the ROM bootloader does feel happy.

PS2 (for geeks). Also, if a PT is to be not only relocated but modified in place as well then "esp_partition_" functions can not be used. Some of them (if not all) use some sha256 checks and will fail to operate with a new PT even if it is a valid PT with a proper MD5 digest. Looks like BL has additional hardcoded sha256 data elsewhere. I did not go deep into this, instead I came up with my custom partition functions.

[MicroController]

Doesn't sound like a "bug" but rather a feature request.

Btw., the problem isn't "incorrect int32/unit32 conversion"; due to two's-complement representation, addition and subtraction are unaffected by the signedness of the involved integer types ((uint32_t)x + ((uint32_t)-4096) == (uint32_t)x + ((int32_t)-4096)).

[code4sex]

/* This is my reply to your initial post that you have edited while I was replying to

Nope, what you are saying is an attempt to represent the issue the other way round and dillute the negative effect...

Once again:

"TERMS":
- bootloader flow MUST be consistent with the IDF docs
- IDF docs _DO_NOT_ impose any limitations as per PT relocation
- IDF docs _DO_NOT_ impose any limitations as per moving PT before BL

It follows that a user is free to choose where to locate his PT without breaking any IDF "rules".

Once a user runs into errors under such conditions then it means that either:
(a) there are no bugs, the TERMS are just not consistent or
(b) the TERMS are okay, the system has a BUG.

In either case something has to be fixed/changed.

Espressif could've placed a wide banner saying "by no means a user should move PT before BL blah-blah-blah".
In this case I would've not reported the bug.

Otherwise, this is an ISSUE and it has to be addressed.

Do you agree this way?

If so, then "E (228) esp_image: partition size 0xfffff000 invalid, larger than 16MB" is a result of an incorrect offset/size calculation implemented in BL and this has to be fixed.

*/

[code4sex]

UPD. It's highy likely some intermediate procedure in .OBJ code fails to check the bounds and passes the value over.
I've scrolled the bootloader .MAP file and found dozzens of potential callers...
The developer team needs only to open the source code and add a few lines to have this fixed. Not a big deal.

[code4sex]

Doesn't sound like a "bug" but rather a feature request.

So-so... A feature request might have sound like this:
The system might become more flexible if BL2 is not bound to 0x1000 address. Instead, the flash at 0x0000 might have contained a 4-bytes offset of the bootloader. Dunno how this can influence the overall security but the flexibility will benefit 100%.

// I've got strict limits on 1-byte data usage via MMU and that's why I'm trying to place the required objects in the lower 4MB address. Hence the attempts to get extra space.