Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Firmware swapping #6450

Closed
wants to merge 44 commits into from
Closed

Firmware swapping #6450

wants to merge 44 commits into from

Conversation

kYc0o
Copy link
Contributor

@kYc0o kYc0o commented Jan 22, 2017

This is a module which enables firmware swapping on stm32f103re based boards. Theoretically it can also be easily ported for any cortex-m3 by adding the necessary defines in cpu_conf.h.

The goal is to provide a secure way to boot different firmwares flashed in the internal ROM, since each image can be compiled to embed metadata that can be used to verify and validate the internal images.

Verification is done using SHA256 over the whole firmware, which is checked before swapping between firmwares.

The images can be validated by verifying their signature using tweetnacl, which will decrypt and verify the signature of the hash.

The process is quite simple:

  • The bootloader is compiled to fit into the first 16K of the ROM, it includes tweetnacl and sha256 modules.
  • Any firmware can be compiled using the FW_SLOTS flag, with the information provided in the Makefile in examples/firmware_swapping.
  • An image compiled with the FW_SLOTS flag will be linked to fit in the slot indicated by FW_SLOT, the metadata is generated automatically using the provided metadata generator in dist/tools/firmware_metadata, which will hash the firmware, encrypt and sign the hash using tweetnacl, then adding these metadata to the first 256 bytes.
  • The bootloader + the firmware with metadata can be merged into one hex file that can be flashed into the ROM of a stm32f103re cpu.

The key pairs used by tweetnacl can be generated by the provided tool on dist/tools/nacl_key_generator.

Other firmwares can be added by several means, either wireless or wired, using the module periph_flashpage to write on the internal ROM.

An interactive bootloader is provided to swap between firmwares, providing also some shell commands to get the metadata, verify and validate the internal images.

The example firmware_swapping provides a Makefile which will execute the whole process mentioned above, using the gcoap example as the first slot firmware and the default example as the second slot.

The proposed method needs SRecord tools (present on most Unix based systems), namely srec_cat.

This PR has been tested on iotlab-m3 boards with success.

Thanks to @msolters for his inspiring job!

@kYc0o kYc0o added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: tools Area: Supplementary tools labels Jan 22, 2017
@kYc0o kYc0o added this to the Release 2017.01 milestone Jan 22, 2017
@PeterKietzmann
Copy link
Member

@kYc0o great to see this feature and progress in that direction. However, (the already postponed) feature freeze will be tomorrow. I think this is not a candidate for 2017.01 and we should focus a well reviewed, stable and clean soultion for the next release.

@emmanuelsearch
Copy link
Member

@PeterKietzmann this is a new feature, so we could think about it differently (basically: if it works and the code is not ugly, why not have it in)?

@kYc0o
Copy link
Contributor Author

kYc0o commented Jan 23, 2017

Maybe @aabadie can take a quick look? The PR states more than 3K lines, but actually is because tweetnacl code is in, which represents this 3K lines. The real amount of code for review is around 400 lines or less.

@cgundogan
Copy link
Member

I didn't look much into the code yet, but is it really necessary to have all those extra tools? Isn't it also possible to create keys with "more standard" tools like openssl and then work from there?

@kYc0o
Copy link
Contributor Author

kYc0o commented Jan 23, 2017

Yes for the keys I needed something that works out of the box and included in RIOT, especially for the generation of the keys.c file which is needed by the bootloader to integrate the keys in the firmware.

I didn't test using keys generated by other tools but it could be a good interoperability test.

@kYc0o kYc0o force-pushed the firmware_swapping branch 2 times, most recently from b6235ff to 82d6e5f Compare January 23, 2017 12:56
@@ -0,0 +1,13 @@
# Key-pair generator using [NaCL](https://nacl.cr.yp.to/)
This program will generate two key-pairs and store them as binary files, using
tweetNaCl package in RIOT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you included the source of tweetnacl, so it's not a package in RIOT's sense anymore, is it? What was the idea behind going this route isntead of using USEPKG=tweetnacl?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the difficulty here is using a RIOT pkg as host tool dependency.

Copy link
Contributor Author

@kYc0o kYc0o Jan 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I though a lot about it and didn't find a "clean" solution. What can I do is to check it out using git or something like that, just to avoid the code being part of RIOT.

Copy link
Member

@cgundogan cgundogan Jan 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually would prefer then a shell script that checks out the source from [1] instead of essentially duplicating the source.

[1] https://github.com/RIOT-OS/tweetnacl

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I can do that and modify the README.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Maybe the README doesn't need any modification then?

FWDEFINES = -DFW_IMAGE_OFFSET=$(FW_IMAGE_OFFSET) -DFW_IMAGE_LENGTH=$(FW_IMAGE_LENGTH) \
-DFW_METADATA_SPACE=$(FW_METADATA_SPACE)
CFLAGS += $(FWDEFINES)
GENERATE_LDSCRIPT = $(Q)$(LINK) -P -E $(FWDEFINES) $(RIOTCPU)/$(CPU)/ldscripts/$(CPU_MODEL)-slots.c \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use linker to generate an ld out of a c file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this way we can dynamically generate linker files for different slot sizes and addresses.

@@ -95,19 +95,25 @@
#endif
#endif

#ifndef FW_SLOTS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there's no clock configuration when using OTA?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can consider it as an issue. If the clock is initialised again it doesn't boot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this... AFAIK the clock can be initialized several times but there might be registers which might need to be set to the "default" value.

EDIT: maybe a different initialization cpu is cleaner. Maybe for the bootloader only the internal oscillator and not all peripherals are needed.

#define FW_SLOT_PAGES (120)
#define FW_SLOT_1 (0x08004000)
#define FW_SLOT_1_END (0x0803FFFF)
#define FW_SLOT_1_PAGE (8)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can some of these be calculated from others? Feels like there's redundant information, especially in combination with FLASHPAGE_SIZE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I can try, I just did a straightforward define.

@@ -0,0 +1 @@
/stm32f103re-slots.ld
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is there a gitignore here? are you generating the .ld in this directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I don't want to include this .ld file into the RIOT source, since it can change if the slot sizes change.

@@ -0,0 +1,31 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not a .c file, please change extension

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a .c file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I got it, will change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c2017? ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c2017? ;)

I like the syntax of that, sign me up!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the extension led to:

arm-none-eabi-gcc: warning: /Users/facosta/git/RIOT-OS/RIOT/cpu/stm32f1/ldscripts/stm32f103re-slots-template.ld: linker input file unused because linking not done

@@ -0,0 +1,148 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer a more generic toolset. One to create a keypair. use xxd to convert that to a .h file usable by code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for generic toolset here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So instead of creating the keys using tweetnacl you prefer to execute an script which runs local tools (if they're installed)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which runs local tools

local, but tested and proven solid tools. Such tools might also be more portable than a self-made one (is there a /dev/random for OSX/Windows)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if I added such a way it's because it's the way we currently do in RIOT. Tools like OpenSSL and so are not in RIOT for now, so the point here is to use what we have, and if a security bug is found on the tweetnacl library I think anyways "security" using this module will be broken anyways.

dev/random exists in Linux and OS X, but obviously not in Windows (AFAIK we don't support Windows, even unofficially).

Copy link
Member

@cgundogan cgundogan Jan 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not saying that tweetnacl might be unsecure, but how we interface to it and the way we use it might be (:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think writing our own tools is fine, but make them as generic as possible.
For example, your code create "firmware keys", "server keys", and "keys.c".
Why don't you write one tool to create a keypair, and use that for both creating a "firmware key" and a "server key"?
Why not use an existing tool for binary -> .c conversion (e.g., xxd), or write a super small tool that does only that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think writing our own tools is fine, but make them as generic as possible.

If there are tools around that do the same and have proven stable, why not use them?
I am just saying that we might introduce unwanted issues which have an impact on the security (e.g. see https://github.com/RIOT-OS/RIOT/pull/6450/files#r97352140). Other established tools might also adapt more quickly to changes in the random internals of the host OS, while we would actively need to maintain the (in this PR) proposed toolset.

seed_rng();

while (n--) {
*target++ = rand();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't there something like this above? anyhow, forget about rand(), or start using rot13 for encryption.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you suggest another way to generate random numbers? This approach worked for me to generate real random keys, but if you propose another approach we can discuss it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is using the C-Libraries "rand()" function, which is probably the most well understood not random pseudo random number generator. For generating keys, you need more "randomness".
probably using /dev/urandom is fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you cut the returned value of rand() to uint8_t, but rand() returns at least RAND_MAX, i.e. "guaranteed to be at least 32767 on any standard library implementation" [1].
And I guess this cut will negatively affect the overall entropy of the stored bytes. I have no refs at hand, but a feeling in my guts.

[1] http://www.cplusplus.com/reference/cstdlib/RAND_MAX/

@@ -0,0 +1 @@
/keys.c
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't generate code in the source directories.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From where can we securely store/fetch cryptographic keys?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try $(BINDIR). If you create stuff in the application's directory, it breaks when building for two boards in parallel. (CI does that).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok the issue was to automatically add the C file as a source to be compiled with the main.c source.

USEMODULE += fw_slots

# Use hashes module for sha256
USEMODULE += hashes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be a dependency of fw_slots?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think I can remove this line safely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it doesn't work to add USEMODULE += hashes on the fw_slots module Makefile...

* @param[in] metadata Metadata struct to fill with firmware metadata
*
*/
void print_metadata(FW_metadata_t *metadata);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all public functions should be prefixed by module name.

@kaspar030
Copy link
Contributor

Could you summarize the NaCL functions used for encrypting and signing? It seems you're using crypto box to "send" the encrypted firmware image to the node?

@kYc0o
Copy link
Contributor Author

kYc0o commented Jan 23, 2017

According to the documentation of NaCl:

C NaCl also provides a crypto_box function callable as follows:

#include "crypto_box.h"
     
const unsigned char pk[crypto_box_PUBLICKEYBYTES];
const unsigned char sk[crypto_box_SECRETKEYBYTES];
const unsigned char n[crypto_box_NONCEBYTES];
const unsigned char m[...]; unsigned long long mlen;
unsigned char c[...];

crypto_box(c,m,mlen,n,pk,sk);

The crypto_box function encrypts and authenticates a message m[0], ..., m[mlen-1] using the sender's secret key sk[0], sk[1], ..., sk[crypto_box_SECRETKEYBYTES-1], the receiver's public key pk[0], pk[1], ..., pk[crypto_box_PUBLICKEYBYTES-1], and a nonce n[0], n[1], ..., n[crypto_box_NONCEBYTES-1]. The crypto_box function puts the ciphertext into c[0], c[1], ..., c[mlen-1]. It then returns 0.

@kaspar030
Copy link
Contributor

kaspar030 commented Jan 23, 2017 via email

@kaspar030
Copy link
Contributor

IMHO this PR needs more work than can be done for the release, so I suggest postponing.

Why don't you concentrate on the firmware swapping and leave out the crypto and necessary tools for now? Even without, this is gonna be hard to integrate in a sane (not-too-hacky) way.

@kYc0o
Copy link
Contributor Author

kYc0o commented Jan 23, 2017

As you can see the firmware swapping is done in a kind of straightforward way, and the only issue is about the clock which cannot be initialised twice, which is maybe the most hacky thing here.

That's IMHO the thing we can think about and would need a more deep analysis, but at least it works for now.

The linking is also modified, but only for flexibility purposes, if we decide to format the ROM in a static way a single ld file would be enough.

I agree for postponing if besides the crypto stuff the firmware swapping doesn't work as expected, or if there are specific things that this PR is adding/modifying which cannot match with the current RIOT code.

Anyways, I know it's a bit late to integrate this feature for the release, but if there's any chance I'll take it. So, feel free to modify the milestone if you consider it convenient.

@kYc0o kYc0o force-pushed the firmware_swapping branch 2 times, most recently from b22189f to 2d8ed3a Compare January 24, 2017 01:04
@kYc0o
Copy link
Contributor Author

kYc0o commented Jan 24, 2017

I updated this PR with only the firmware swapping feature and verification through sha256. The validation could be added when we'll find a secure way to sign the firmware/hash.

@kYc0o
Copy link
Contributor Author

kYc0o commented Jan 25, 2017

Fix Jenkins spotted errors.

@kYc0o kYc0o force-pushed the firmware_swapping branch 3 times, most recently from 8ee1026 to 6c46200 Compare January 26, 2017 14:22
@PeterKietzmann PeterKietzmann modified the milestones: Release 2017.04, Release 2017.01 Jan 26, 2017
@kYc0o
Copy link
Contributor Author

kYc0o commented Mar 17, 2017

Rebased.

# Use ethos (ethernet over serial) for network communication and stdio over
# UART
GNRC_NETIF_NUMOF := 2
USEMODULE += ethos gnrc_netdev2

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bootloader does not build, because netdev2 was renamed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix.

}

/* Is this the newest non-Golden Image image we've found thus far? */
if ( fw_slot_metadata.version > newest_firmware_version ) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work, if a firmware with version number 0 is the latest version, e.g. if only one image is installed, because slot number 0 is an invalid slot. At least for the code used in the main.c file of the bootloader.
The easiest solution would be to define 0 as an invalid firmware version number in the documentation. But currently, you're using version number 0 in the Makefile of the firmware_swapping example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, on the example version 0 is used. I'll change it to match with this code. In practice, I didn't expect a firmware version 0 to be deployed, but it's true that some doc regarding this is necessary. Will add.

@kYc0o
Copy link
Contributor Author

kYc0o commented Mar 20, 2017

@kaspar030 can I squash? The commit list is getting long and I have problems with upcoming PR based on this one...

@kaspar030
Copy link
Contributor

@kaspar030 can I squash? The commit list is getting long and I have problems with upcoming PR based on this one...

Feel free to squash if the commit list gets too long!

Copy link
Member

@lebrush lebrush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of having a uniform way of doing it in RIOT and this looks in good shape but I think it needs some changes. I implemented something similar some years ago so I know some of the pitfalls of such a system :-)

  • You want the bootloader to be as small as possible and 16KB sounds like a lot. One of the first things I would make configurable is the sha256. In my experience a simple crc for which most mcus have a peripheral does the job. Making it configurable allows to set it to more complex hashing if necessary.

  • I read that one of the goals is that the bootloader is interactive. I didn't see it in the code but I think this should be optional. For development it's great that you can tell the bootloader boot 0 or boot 1 but for production might not be desired.

  • One use case which IMO should be considered: flash in MCU is expensive, flash as a external unit not. I can imagine the case where the bootloader fetches the flash image from an external flash unit rather than different sections of the internal flash.

  • Metadata can't be written in a single flash page! To write it you have to erase the flash page first, imagine that something happens then: your metadata is corrupt and you won't boot anything. A better idea is to store it in two flash pages with a counter and a crc :-)

@@ -26,6 +27,7 @@ override RIOTBOARD := $(abspath $(RIOTBOARD))
override RIOTPKG := $(abspath $(RIOTPKG))
override RIOTPROJECT := $(abspath $(RIOTPROJECT))
override GITCACHE := $(abspath $(GITCACHE))
override FW_METADATA := $(abspath $(FW_METADATA))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: missing space

# reflecting the FW_IMAGE_OFFSET and FW_IMAGE_LENGTH values defined in the
# target project's Makefile.
ifeq ($(FW_SLOTS),1)
ifeq ($(FW_SLOT),1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about LINKER_SCRIPT = $(CPU_MODEL)_slot$(FW_SLOT).ld. Then you don't limit it to 2 (it might be a future use case).

Copy link
Contributor Author

@kYc0o kYc0o Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea actually is to limit to 2. The next step is to try to get always slot 1 as the bootable slot, download images to slot 2 and then swap both images. The complexity of having several slots is that you need to know before compiling where are you going to put them (e.g. to know where they are the current images to don't overwrite them).

@@ -1 +1,2 @@
FEATURES_PROVIDED += periph_pm
FEATURES_PROVIDED += periph_slots
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new line at EOF :-)

@@ -95,19 +95,25 @@
#endif
#endif

#ifndef FW_SLOTS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this... AFAIK the clock can be initialized several times but there might be registers which might need to be set to the "default" value.

EDIT: maybe a different initialization cpu is cleaner. Maybe for the bootloader only the internal oscillator and not all peripherals are needed.

* @{
*/

#ifndef FW_METADATA_SPACE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all of this should be project specific and not coded in cpu_conf.h

CFLAGS+=-DFW_METADATA_SPACE=$(FW_METADATA_SPACE) make clean all; \
cp bin/$(BOARD)/bootloader.hex ../firmware_swapping

gnrc_networking-slot1:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe is more intuitive to to make gnrc_networking SLOT=1 than to call make gnrc_networking-slot1.

flash:
OPENOCD_CONFIG=../../boards/$(BOARD)/dist/openocd.cfg \
HEXFILE=firmware-slots.hex \
../../dist/tools/openocd/openocd.sh flash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line

static void int_flash_read(uint8_t *data_buffer, uint32_t address, uint32_t count)
{
uint8_t *read_addres = (uint8_t*)address;
while (count--) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about memcpy it's probably compiled in anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll try to use the flashpage_read functions to do this.

uint32_t page;

DEBUG("[fw_slots] Getting internal FW slot %d metadata\n", fw_slot);
if (fw_slot > MAX_FW_SLOTS || fw_slot == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why fw_slot == 0 is not valid? actually it would simplify the code quite a lot if the slots start at 0. So for 1 and 2 the variable should be 0 and 1. Or just rename so it's consistent with 0 and 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the idea was to assume that slot 0 is the bootloader, then slot 1 the bootable firmware and slot 2 the "caching" for the downloaded new firmware.

*/
extern uint32_t _estack;

void fw_slots_jump_to_image(uint32_t destination_address)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be implemented per architecture! Maybe add in cpu/stm32f1/cpu.c something called cpu_branch_address.

Copy link
Member

@lebrush lebrush Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(or in cortexm-common)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right! I'll change that.

@kYc0o
Copy link
Contributor Author

kYc0o commented Mar 21, 2017

@lebrush Thanks for your valuable comments! I'll address them asap.

You want the bootloader to be as small as possible and 16KB sounds like a lot. One of the first things I would make configurable is the sha256. In my experience a simple crc for which most mcus have a peripheral does the job. Making it configurable allows to set it to more complex hashing if necessary.

The goal of using hashing is that we can sign the hash for security purposes. A crc can be a weak option to do this. Otherwise, we might need to encrypt and sign the whole firmware, which is far more expensive in time thus energy.

I read that one of the goals is that the bootloader is interactive. I didn't see it in the code but I think this should be optional. For development it's great that you can tell the bootloader boot 0 or boot 1 but for production might not be desired.

Yeah this was the previous proposition. I updated this code to make it automatic but I can add an #ifdef to add this option if necessary.

One use case which IMO should be considered: flash in MCU is expensive, flash as a external unit not. I can imagine the case where the bootloader fetches the flash image from an external flash unit rather than different sections of the internal flash.

Yes. This case is kind of the "ideal", but we cannot assume that an external flash is always available. Besides, I think writing to external flash is quite expensive in terms of energy, and maybe a bit slower than internal flash (to be tested).
Some functions are part of this PR to take care of external flash, but they're not implemented yet. With #5624 merged, this should be easy to implement.

Metadata can't be written in a single flash page! To write it you have to erase the flash page first, imagine that something happens then: your metadata is corrupt and you won't boot anything. A better idea is to store it in two flash pages with a counter and a crc :-)

Thanks for this suggestion! I'll do it.

cladmi and others added 2 commits April 4, 2017 17:50
Allow compiling any firmware by using:

    make FIRMWARE=firmware_name

Main thing to make it work was adding 'romslot1' and 'romslot2' in the
bootloader linker script with the section names '.slot.1.*'
The bootloader is compiled with .o of the firmware slots.

To create the firmware slots .o files, I use objcopy to load the binaries at
sections called '.slot.1.*'.
firmware_swapping: compile to an elf file
@kYc0o
Copy link
Contributor Author

kYc0o commented Aug 7, 2017

Closing in favor of #7457 .

@kYc0o kYc0o closed this Aug 7, 2017
@kYc0o kYc0o added this to Closed/abandoned/superseeded in Software Updates Sep 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: tools Area: Supplementary tools Platform: ARM Platform: This PR/issue effects ARM-based platforms State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
Software Updates
  
PRs - closed/abandoned/superceded
Development

Successfully merging this pull request may close these issues.

None yet