SAMx5x, NVM: stack usage

[ALTracer]

Summary

Current implementation of samx5x.c target support driver uses SAMX5X_PAGE_SIZE 512-byte buffers on stack of probe running Blackmagic firmware. This may be an issue (or even architectural bug) for RAM-constrained platforms, such as ones based on stm32f103cb (native, even more so stlink & swlink) and stm32f072rb ("F072-IF").

Details

Current code fragments

in samx5x_set_flashlock()

blackmagic/src/target/samx5x.c

Lines 637 to 646 in 578eaaa

	static int samx5x_set_flashlock(target_s *t, uint32_t value)
	{
	uint8_t buffer[SAMX5X_PAGE_SIZE];
	target_mem_read(t, buffer, SAMX5X_NVM_USER_PAGE, SAMX5X_PAGE_SIZE);
	uint32_t current_value;
	memcpy(&current_value, buffer + SAMX5X_USER_PAGE_OFFSET_LOCK, 4);
	if (value != current_value)
	return samx5x_update_user_word(t, SAMX5X_USER_PAGE_OFFSET_LOCK, value, NULL, false);

and samx5x_set_bootprot()

blackmagic/src/target/samx5x.c

Lines 682 to 694 in 578eaaa

	static int samx5x_set_bootprot(target_s *t, uint8_t value)
	{
	uint8_t buffer[SAMX5X_PAGE_SIZE];
	target_mem_read(t, buffer, SAMX5X_NVM_USER_PAGE, SAMX5X_PAGE_SIZE);
	uint32_t current_value;
	memcpy(&current_value, buffer + SAMX5X_USER_PAGE_OFFSET_BOOTPROT, 4);
	uint32_t new_value = current_value & ~SAMX5X_USER_PAGE_MASK_BOOTPROT;
	new_value |= (value << SAMX5X_USER_PAGE_SHIFT_BOOTPROT) & SAMX5X_USER_PAGE_MASK_BOOTPROT;
	if (new_value != current_value)
	return samx5x_update_user_word(t, SAMX5X_USER_PAGE_OFFSET_BOOTPROT, new_value, NULL, false);

but then allocates another buffer in the next/nested call to samx5x_update_user_word()

blackmagic/src/target/samx5x.c

Lines 607 to 624 in 578eaaa

	uint8_t buffer[SAMX5X_PAGE_SIZE];
	uint32_t current_word;
	target_mem_read(t, buffer, SAMX5X_NVM_USER_PAGE, SAMX5X_PAGE_SIZE);
	memcpy(&current_word, buffer + addr, 4);
	uint32_t factory_word = 0;
	for (size_t i = 0; !force && i < 4U && addr + i < 20U; ++i)
	factory_word |= (uint32_t)factory_bits[addr + i] << (i * 8U);
	const uint32_t new_word = (current_word & factory_word) | (value & ~factory_word);
	if (value_written != NULL)
	*value_written = new_word;
	if (new_word != current_word) {
	DEBUG_INFO("Writing user page word 0x%08" PRIx32 " at offset 0x%03" PRIx32 "\n", new_word, addr);
	memcpy(buffer + addr, &new_word, 4);
	return samx5x_write_user_page(t, buffer);

I think this may be mitigated by tactical block scoping -- the first buffer is not used in the update call, so it could be automatically deallocated from stack before that call. Alternatively, one could allocate a single buffer for that page via target_flash structs and/or priv_storage mechanisms. Or it could reuse the target_flash->buf (which disappears on flash_done()).

sizeof(target_flash_s) = 64 at the moment, I think.

samx5x_cmd_read_userpage() is fine as it only dumps freshly read userpage contents. These are the 4 concerning functions lighting up in Stack Analyzer, and are the scope of this issue.

Misc. analysis

I found this by loading the project into STM32CubeIDE with Stack Analyzer and compiling with OPT_FLAGS="-Os -fstack-usage". Sometime back in June 2023.
Puncover also supports stack usage analysis, but doesn't display stack data on any of my machines.
Firmware does not compile with -Wstack-usage=4096 because of 5 functions with unbounded stack usage (recursion?).

Next biggest obvious stack footprints are (by cranking down the -Wstack-usage):

• 1032 bytes in exec_q_memory_map() which stems from clever on-the-fly XML generation
• 736 bytes in cortexm_halt_poll(), thanks to inlining cortexm_fault_unwind() (264) and cortexm_hostio_request() (328), see with -fno-inline
• 576 bytes in lpc_iap_call(), due to uint32_t saved_regs[53] and other compicated stuff.

Sufficient reserved stack size is estimated to be around 4 KiB, but I never measured this directly by inquiring the probe at runtime (and it doesn't have such telemetry support or stack coloration).

Impact

Users of SAMx5x (SAMD51, SAME51) who use Blackmagic firmware (specifically) to modify user/option bytes via monitor commands (un)lock_{flash,bootprot}, like to wipe/replace Adafruit bootloader etc.
Disclaimer: I do not work with these targets and can't test support of these features (does it work, errors out, or blows up the probe and/or the target?) I'm only posting this in hope someone gets to it, if it's a real problem. This was contributed in #534 since v1.7.0.