sys/riotboot: riotboot header v2

[fabian18]

Contribution description

Introducing riotboot_hdr_v2.

We want to test boot an image and have an automatic revert to a previous image if a new image fails to boot.
For example due to an unexpected endless loop or compromised firmware or kernel panic.

This adds a flags field to the header to encode an image state and a number of boot attempts.
The watchdog is started in the bootloader. An image has to confirm that it booted successfully to not be labeled as DISMISSED by the bootloader.

Testing procedure

Issues/PRs references

[fabian18]

I first wanted that the flags are also covered by the checksum. But this required rewriting of the checksum and this required a page erase of the current firmware and this made the program stall.

[crasbe]

Just some preliminary high-level styling related comments. The static test also has some warnings left.

[crasbe]

Suggested change

	#include "riotboot/wdt.h"
	# include "riotboot/wdt.h"

[crasbe]

Suggested change

	#if defined(MODULE_RIOTBOOT_HDR_V2)
	#define RIOTBOOT_MAGIC RIOTBOOT_MAGIC_V2
	#else
	#define RIOTBOOT_MAGIC RIOTBOOT_MAGIC_V1
	#endif
	#if defined(MODULE_RIOTBOOT_HDR_V2)
	# define RIOTBOOT_MAGIC RIOTBOOT_MAGIC_V2
	#else
	# define RIOTBOOT_MAGIC RIOTBOOT_MAGIC_V1
	#endif

[crasbe]

Suggested change

	#ifndef CONFIG_RIOTBOOT_MAX_ATTEMPTS
	#define CONFIG_RIOTBOOT_MAX_ATTEMPTS 3u
	#endif
	#ifndef CONFIG_RIOTBOOT_MAX_ATTEMPTS
	# define CONFIG_RIOTBOOT_MAX_ATTEMPTS 3u
	#endif

[crasbe]

Suggested change

	#define CONFIG_RIOTBOOT_WDT_TIMEOUT_MSEC 4000
	# define CONFIG_RIOTBOOT_WDT_TIMEOUT_MSEC 4000

[crasbe]

Suggested change

	switch (riotboot_hdr->v1.magic_number) {
	case RIOTBOOT_MAGIC_V1:
	return &riotboot_hdr->v1.version;
	case RIOTBOOT_MAGIC_V2:
	return &riotboot_hdr->v2.version;
	default:
	return NULL;
	switch (riotboot_hdr->v1.magic_number) {
	case RIOTBOOT_MAGIC_V1:
	return &riotboot_hdr->v1.version;
	case RIOTBOOT_MAGIC_V2:
	return &riotboot_hdr->v2.version;
	default:
	return NULL;

As per our Coding Convention: https://github.com/RIOT-OS/RIOT/blob/master/CODING_CONVENTIONS.md#indentation-and-braces

[crasbe]

Suggested change

	case RIOTBOOT_MAGIC_V1:
	return &riotboot_hdr->v1.start_addr;
	case RIOTBOOT_MAGIC_V2:
	return &riotboot_hdr->v2.start_addr;
	default:
	return NULL;
	case RIOTBOOT_MAGIC_V1:
	return &riotboot_hdr->v1.start_addr;
	case RIOTBOOT_MAGIC_V2:
	return &riotboot_hdr->v2.start_addr;
	default:
	return NULL;

[crasbe]

Suggested change

	case RIOTBOOT_MAGIC_V1:
	return &riotboot_hdr->v1.chksum;
	case RIOTBOOT_MAGIC_V2:
	return &riotboot_hdr->v2.chksum;
	default:
	return NULL;
	case RIOTBOOT_MAGIC_V1:
	return &riotboot_hdr->v1.chksum;
	case RIOTBOOT_MAGIC_V2:
	return &riotboot_hdr->v2.chksum;
	default:
	return NULL;

[crasbe]

Suggested change

	* @brief Calculate header checksum
	* @brief Getter for the header checksum

If I understand the code correctly, it does not calculate the checksum when calling this function, right?

[mguetschow]

Are the riotboot images somehow compatible with the SUIT ecosystem? If not, wouldn't it make sense to converge towards that instead of introducing yet another custom format?

[fabian18]

What contradiction do you see in SUIT? From a SUIT perspective there is only binary payload.
You could say that the header does not have to be shipped with an image because it could be generated by a currently running image. This would not change the fact that there is a header and that I would like to encode an image state.

[mguetschow]

Disclaimer: I haven't looked much into SUIT. But from my understanding, your proposed riotboot header includes metadata information about the image, that I would have expected in a SUIT manifest such as https://datatracker.ietf.org/doc/draft-ietf-suit-manifest/.

But it can well be that that metadata is disjoint from what would be carried in a SUIT manifest, in that case please just disregard my comment.