dist/tools: add firmware metadata generator by kYc0o · Pull Request #6919 · RIOT-OS/RIOT

kYc0o · 2017-04-18T08:53:30Z

Since #6450 became a bit complicated to manage, I'll start to split the work in several PRs, with the aim to ease review and have a better picture of the approach.

This PR adds a firmware metadata generator, which will produce a binary file representing a structure of this kind:

typedef struct FW_metadata {
    uint8_t hash[SHA256_DIGEST_LENGTH]; /**< SHA256 Hash of firmware image */
    uint8_t shash[SIGN_LEN];            /**< Signed SHA256 */
    uint16_t version;                   /**< Integer representing firmware version */
    uint32_t size;                      /**< Size of firmware image */
    uint32_t appid;                     /**< Integer representing the application ID */
} FW_metadata_t;

as a simplified way to characterise the contents of a firmware, while adding some security features as the sha256 hash for integrity check, as well as a field for a signed hash, which will come in a following PR.

This approach will use the current RIOT implementation of sha256 to allow portability.

To test the firmware generator, you can run the following command for any RIOT example or application:

make firmware-metadata VERSION=0x1 APPID=0xabcd1234

using VERSION=0x1 and APPID=0xabcd1234 as examples.

The ultimate goal is to put this metadata at the beginning of any firmware, to use it e.g. for a firmware update. This feature will come in an upcoming PR, stay tuned.

aabadie

A few inline comments. Didn't test locally.

aabadie · 2017-04-18T15:08:11Z

+
+#define FW_METADATA_BIN "firmware-metadata.bin"
+
+typedef struct FW_metadata {


This doesn't seem to follow coding conventions, why not use the explicit name: firmware_metadata ? and firmware_metadata_t below ?

aabadie · 2017-04-18T15:09:39Z

 endif # BUILD_IN_DOCKER

+firmware-metadata: all generate-metadata
+	$(Q)$(FW_METADATA)/bin/./generate-metadata $(BINFILE) $(VERSION) $(APPID) $(METADATA_FILE)


do you really need the ./ in $(Q)$(FW_METADATA)/bin/./generate-metadata ?

aabadie · 2017-04-18T15:11:16Z

+all: bin bin/generate-metadata
+
+bin:
+	mkdir bin


maybe use -p to avoid messages like mkdir: cannot create directory ‘bin’: File exists

aabadie · 2017-04-18T15:11:49Z

+follows:
+
+```c
+typedef struct FW_metadata {


same comment as below about structure name

aabadie · 2017-04-18T15:12:25Z

@@ -0,0 +1,163 @@
+/*
+ * Copyright (c) 2016, Mark Solters <msolters@gmail.com>.
+ *               2016, Francisco Acosta <francisco.acosta@inria.fr>


I think you should simply put Inria in the copyright

aabadie · 2017-04-18T15:13:02Z

+    fclose(firmware_bin);
+
+    /*
+     * TODO Sign hash


Do you plan to address this ?

Yes, it will come in a following up PR.

So I guess it can be removed from this one.

aabadie · 2017-04-18T15:13:12Z

+    }
+
+    /* Generate FW image metadata */
+


this empty line is not needed

aabadie · 2017-04-18T15:19:16Z

+
+    (void)argc;
+
+    if (!argv[1]) {


Can you just check argc value and display the usage in case it's < 4 ?

Kijewski · 2017-04-18T20:01:10Z

+
+all: bin bin/generate-metadata
+
+bin:


add a slash bin/

Kijewski · 2017-04-18T20:01:19Z

+
+CFLAGS += -g -O3 -Wall -Wextra -pedantic -std=c99
+
+all: bin bin/generate-metadata


remove bin

Kijewski · 2017-04-18T20:02:12Z

+bin:
+	mkdir bin
+
+bin/generate-metadata: $(METADATA_HDR) $(METADATA_SRC)


append | bin/

kYc0o · 2017-04-19T11:31:01Z

@aabadie @Kijewski Comments addressed.

Kijewski · 2017-04-19T16:12:26Z

+bin/:
+	mkdir -p bin
+
+bin/generate-metadata: bin/ $(METADATA_HDR) $(METADATA_SRC)


bin/ should be a pre-requirement instead of a requirement. A pre-requirement only needs to exist, but a newer timestamp does not force a rebuild. Creating bin/generate-metadata will touch bin/, which will tell make to recreate bin/generate-metadata in its next invocation. Pre-requirements are placed after the requirements, separated by a pipe.

Oh! Ok I understand now. However, don't we actually want bin as a requirement? What's really the difference? Anyways,bin can only exists if a build was triggered, and if I understand correctly the contents cannot be changed by other means.

When a file is changed, then the modification timestamp of its parent folder gets updated. A newer timestamp of a requirement of $X tells Make to rebuild $X (in here $X's parent directory is its requirement). So updating $X will make $X look outdated to Make. Every time you run Make it will update $X, which wastes a tiny amount of wall clock time.

As a rule of thumb directories should always be prerequirements, not requirements, unless you are doing something really odd. :)

OK, thanks for the explanation, I'll modify this file accordingly.

aabadie · 2017-04-20T07:52:41Z

@@ -0,0 +1,19 @@
+RIOTBASE := ../../..


I think this line should be:

RIOTBASE ?= $(CURDIR)/../../..

I think since dist/tools/ is a path that will always exists in any clone of RIOT, we don't really need to define $(CURDIR), and so the relative path will always be the same.

Ok you are right.

kYc0o · 2017-04-24T20:08:32Z

@Kijewski addressed.

aabadie

Added a few other comments

aabadie · 2017-04-25T10:36:45Z

@@ -0,0 +1,35 @@
+# Metadata generator for firmware verification
+This program will generate a binary file containing a metadata structure as


Minor: use present instead of future.
This program generates a binary file...

aabadie · 2017-04-25T10:37:47Z

+} firmware_metadata_t;
+```
+
+This structure will be filled with the data obtained from the firmware.


Same here: This structure is filled with...

aabadie · 2017-04-25T10:40:10Z

+
+_\<output-path\>_\: The path for fimrware_metadata.bin
+
+The results will be printed if the operation is successful, and a binary


Same, with if the sense is conditional (something like might be). Also use present:
If the operation succeeds, the results is printed out and a binary called "firmware-metadata.bin" is created (written?)

Looking at generate-metadata.c this is not totally true: the user can set a custom output filename.

aabadie · 2017-04-25T10:43:46Z

+    fclose(firmware_bin);
+
+    /*
+     * TODO Sign hash


So I guess it can be removed from this one.

aabadie · 2017-05-11T17:40:46Z

@kYc0o, what's the status here ?

kYc0o · 2017-05-11T17:57:11Z

Just addressed your comments.

addressed

aabadie

Changes are good to me here. ACK

kYc0o · 2017-05-12T12:32:48Z

@Kijewski all ok for you?

aabadie · 2017-05-12T13:40:02Z

@kYc0o, Murdock is reporting errors, can you have a look ?

kYc0o · 2017-05-12T14:03:51Z

It's again related to the pic32 boards... @kaspar030 there is something wrong in the toolchain of Murdock2?

BTW it needs rebase.

kYc0o · 2017-05-12T14:08:43Z

BTW it needs rebase.

I meant squash.

aabadie · 2017-05-14T12:56:09Z

+
+_\<appid\>_\: ID for the application in 32-bit HEX
+
+_\<output-path\>_\: The path for fimrware_metadata.bin


fimrware_metadata.bin => firmware_metadata.bin

aabadie · 2017-05-14T12:57:29Z

+
+If the operation succeeds, the results are printed out and a binary called
+"firmware-metadata.bin" is written, if no _output-path_ option is specified,
+in which case the file is written to the given path with the given name.


in which case should be removed

Found problems

aabadie · 2017-05-14T13:03:05Z

Just tested it and had problems:

bin/generate-metadata ~/samr21-default.elf 1.0 1234 ~/.

generate a segfault. Here is what gdb says:

Starting program: /home/aabadie/softs/src/riot/RIOT/dist/tools/firmware_metadata/bin/generate-metadata /home/aabadie/samr21-default.elf 1.0 1234 /home/aabadie/.
Firmware bytes read: 1855452
Firmware Size: 1855452
Firmware Version: 0x1
Firmware APPID: 0x1234
Firmware HASH: ce 9c f5 56 84 44 2f 36 97 50 1f 6b 34 68 56 3f 4c 5d dc a9 a9 48 71 73 59 08 b8 1a ac 65 80 b3 
Firmware signed HASH: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
Metadata size: 76

Program received signal SIGSEGV, Segmentation fault.
__GI__IO_fwrite (buf=0x7fffffffd1d0, size=76, count=1, fp=0x0) at iofwrite.c:37
37	iofwrite.c: No such file or directory.

Leaving the output path empty works fine.

miri64 · 2017-05-14T13:17:26Z

+
+Where:
+
+_\<firmware.bin\>\:_ The firmware in binary format


Maybe use <firmware.bin> instead so it is better readable in raw text?

I don't get exactly what you mean...

I think @kaspar030 means "use ` ` around <firmware.bin>". +1 on my side

kYc0o · 2017-05-22T13:33:05Z

@aabadie addressed comments.

Your problem is that you didn't give a file name for the binary with the metadata. I fixed the README.md to be explicit about this.

aabadie

A few other comments mainly rephrasing in the README to make things crystal clear.

aabadie · 2017-05-22T14:58:47Z

+To use, you should call `generate-metadata` with the following arguments:
+
+```console
+./generate-metadata <firmware.bin> <version> <appid> <output-path>


<output-path> should be <output-filename> for clarity

I think <output-filename> would lead to confusion, since we need the full path plus the file name.

aabadie · 2017-05-22T14:59:21Z

+This structure is filled with the data obtained from the firmware.
+
+## Usage
+To use, you should call `generate-metadata` with the following arguments:


To use it, call generate-metadata with the following arguments:

aabadie · 2017-05-22T15:00:59Z

+_\<output-path/filename.bin\>_\: The path and name of the metadata binary file
+
+If the operation succeeds, the results are printed out and a binary called
+"firmware-metadata.bin" is written, if no _output-path_ option is specified.


This is confusing as first sight.

What's the confusion?

aabadie · 2017-05-22T15:03:07Z

@@ -0,0 +1,36 @@
+# Metadata generator for firmware verification
+This program generates a binary file containing a metadata structure as


Maybe add an empty line after title.

Quick question: does the file only contain the metadata or the metadata + the initial firmware ? If it's the second case, it should be more explicit. Otherwise, one can easily understand that the generated binary file only contain the struct.

Maybe add an empty line after title.

What this will change?

uick question: does the file only contain the metadata or the metadata + the initial firmware ? If it's the second case, it should be more explicit. Otherwise, one can easily understand that the generated binary file only contain the struct.

It is the first case.

aabadie · 2017-05-22T15:04:08Z

+
+Where:
+
+_\<firmware.bin\>\:_ The firmware in binary format


I think @kaspar030 means "use ` ` around <firmware.bin>". +1 on my side

aabadie · 2017-05-22T15:05:02Z

+    char firmware_metadata_path[128];
+
+    if (argc < 4) {
+        puts("Usage: generate-metadata <BINFILE> <VERSION> <APPID> [output path]");


s/output path/output filename/

I still think output path is ok...

aabadie · 2017-05-22T15:05:46Z

+
+    sha256_init(&firmware_sha256);
+
+    while((bytes_read = fread(firmware_buffer, 1, sizeof(firmware_buffer), firmware_bin))) {


space after while missing

aabadie · 2017-05-22T15:06:16Z

+/* Output metadata .bin file */
+FILE *metadata_bin;
+
+uint32_t firmware_size = 0;


could be declared static

Does it matter for a native C program? In that case all the variables should be declared static in this program.

kYc0o · 2017-05-22T16:32:36Z

Actually, I don't think that beautify a lot the README would be of any use. This tool is not really meant to be used by a final user, but called automatically by the build system when building firmware updates like in the subsequent PRs.

kYc0o · 2017-05-23T10:11:29Z

@aabadie everything good here for you?

kaspar030

Let's not merge this until we have a set of PR's that actually enable OTA.

kYc0o · 2017-05-23T11:25:17Z

Let's not merge this until we have a set of PR's that actually enable OTA.

What do you mean with that? I'm about to push the last PR which enables the "updates fetch" using CoAP (though the transport method is TFTP). That would be the last PR regarding this topic.

kYc0o · 2017-06-09T17:19:19Z

#7167 adds the firmware downloading for OTA updates. I think with this PR the firmware update functionality is complete.

kYc0o · 2017-08-07T18:07:32Z

Closing in favour of #7457.

kYc0o added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: tools Area: Supplementary tools labels Apr 18, 2017

kYc0o assigned haukepetersen, LudwigKnuepfer and kaspar030 Apr 18, 2017

kYc0o force-pushed the firmware_metadata branch from 745cc1b to 75b06f7 Compare April 18, 2017 13:09

This was referenced Apr 18, 2017

Add RIOT bootloader to manage firmware slots #6922

Closed

examples: add tftp OTA programming example #6924

Closed

aabadie requested changes Apr 18, 2017

View reviewed changes

Kijewski suggested changes Apr 18, 2017

View reviewed changes

kYc0o force-pushed the firmware_metadata branch 2 times, most recently from f527342 to 13d5d9b Compare April 19, 2017 11:30

Kijewski previously requested changes Apr 19, 2017

View reviewed changes

aabadie reviewed Apr 20, 2017

View reviewed changes

aabadie requested changes Apr 25, 2017

View reviewed changes

kYc0o force-pushed the firmware_metadata branch from 0048735 to a61a4d4 Compare May 11, 2017 17:56

aabadie previously approved these changes May 12, 2017

View reviewed changes

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 12, 2017

kYc0o force-pushed the firmware_metadata branch from a61a4d4 to f444d1b Compare May 12, 2017 14:12

aabadie reviewed May 14, 2017

View reviewed changes

miri64 requested changes May 14, 2017

View reviewed changes

kYc0o force-pushed the firmware_metadata branch from 004c495 to 9f385e4 Compare May 22, 2017 13:32

kYc0o added 3 commits May 22, 2017 15:32

dist/tools: Add firmware metadata generator

0cb0e60

Makefile.include: add firmware-metadata target

500d0d9

f dist/tools: Add firmware metadata generator

9f385e4

aabadie requested changes May 22, 2017

View reviewed changes

kYc0o added 3 commits May 22, 2017 18:39

f Makefile.include: use BINFILE only inside metadata rule

e908bd6

f dist/tools/firmware_metadata: add space after while

0221cc3

f dist/tools/firmware_metadata: rephrase

b057174

kaspar030 requested changes May 23, 2017

View reviewed changes

aabadie modified the milestone: Release 2017.07 Jun 26, 2017

kYc0o modified the milestones: Release 2017.10, Release 2017.07 Jun 30, 2017

kYc0o closed this Aug 7, 2017


		#define FW_METADATA_BIN "firmware-metadata.bin"

		typedef struct FW_metadata {


		CFLAGS += -g -O3 -Wall -Wextra -pedantic -std=c99

		all: bin bin/generate-metadata

		@@ -0,0 +1,35 @@
		# Metadata generator for firmware verification
		This program will generate a binary file containing a metadata structure as


		_\<output-path\>_\: The path for fimrware_metadata.bin

		The results will be printed if the operation is successful, and a binary


		_\<appid\>_\: ID for the application in 32-bit HEX

		_\<output-path\>_\: The path for fimrware_metadata.bin

		@@ -0,0 +1,36 @@
		# Metadata generator for firmware verification
		This program generates a binary file containing a metadata structure as


		sha256_init(&firmware_sha256);

		while((bytes_read = fread(firmware_buffer, 1, sizeof(firmware_buffer), firmware_bin))) {

Uh oh!

Conversation

kYc0o commented Apr 18, 2017

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Apr 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kijewski Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Apr 24, 2017

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented May 11, 2017

Uh oh!

kYc0o commented May 11, 2017

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

kYc0o commented May 12, 2017

Uh oh!

aabadie commented May 12, 2017

Uh oh!

kYc0o commented May 12, 2017

Uh oh!

kYc0o commented May 12, 2017

Kijewski Apr 20, 2017 •

edited

Loading

aabadie May 14, 2017 •

edited

Loading