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

Sw version based downgrade prevention #1239

Conversation

mateo-ligne-netatmo
Copy link

Hello everyone,

This PR follow a previous issue where I express my need of having a software version comparison before loading a new firmware through Recovery uart in a Single application slot configuration.

The firmware update will be monitored by MCUmgr, and in order to stop the update from it, I defined a new error type that will stop update process. This MCUmgr feature is the following open PR

I also added a commit allowing to use 2 UART peripheral at the same time one for Logs and the other for MCUmgr communication.

It's my first MCUBoot upload so don't hesitate to make feedbacks or suggestions, I will take them into account !

Copy link
Member

@utzig utzig left a comment

Choose a reason for hiding this comment

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

Please sign-off your commits.

@@ -22,8 +22,8 @@
#include "bootutil/bootutil_log.h"
#include <usb/usb_device.h>

#if defined(CONFIG_BOOT_SERIAL_UART) && defined(CONFIG_UART_CONSOLE)
#error Zephyr UART console must been disabled if serial_adapter module is used.
#if DT_SAME_NODE(DT_CHOSEN(zephyr_uart_mcumgr), DT_CHOSEN(zephyr_console))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check is here for prevent an ademption to usage the same uart device concurrently by diffrent software entities.
There is nothing bad in having the same node for already disabled console for instance.

Choose a reason for hiding this comment

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

Yes, but what about the case where we want to have UART console and DFU over UART on two separate UART device ? As far as I understood this check disallow this possibility, whereas my changes enable it if console UART and MCUmgr UART are 2 differents devices.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@de-nordic Can you take a look on this and the next line?

Copy link
Member

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

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

Can you also write a bit more in the commit text. The summary lines are good, but the body (above the signed off by line) should have a bit of an explanation as to why a change is being made. (probably not needed for the typo fix).

@mateo-ligne-netatmo mateo-ligne-netatmo force-pushed the SW-version-Based-downgrade-prevention branch from d214fde to efcbd14 Compare December 27, 2021 10:36
Copy link
Member

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

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

For future reference, please format the text of the commit text to 75-ish columns. If there are no other changes, I can fix this up before this goes in, however.

@mateo-ligne-netatmo mateo-ligne-netatmo force-pushed the SW-version-Based-downgrade-prevention branch from efcbd14 to 558148e Compare January 12, 2022 08:28
@mateo-ligne-netatmo mateo-ligne-netatmo force-pushed the SW-version-Based-downgrade-prevention branch from 558148e to 0a246dc Compare February 8, 2022 13:47
@mateo-ligne-netatmo
Copy link
Author

Hello, I have rebased my PR with current MCUboot main especially following #1255. It turns out that on a failed firmware update (for too low SW version for instance), stored binary won't be overwritten but it will be decrypted a second time which will break the applicative. To counter that, I've just had a return code check before decryption.

This commit purpose is to allow the use of UART console and Serial
recovery UART in a same firmware as long as they are associated to two
separate UART devices.

To ensure this we compare following devices defined in dts file :
    zephyr_uart_mcumgr : UART associated to DFU process
    zephyr_console : UART used for console

Signed-off-by: Mateo Ligne <[email protected]>
In a serial recovery configuration, software based downgrade prevention
option cannot be used for now as it is designed to trigger a software
version comparison only before a swap in a 2 slots MCUboot configuration

In fact, serial recovery update will overwrite 1st slot on every update,
 so this check won't be done on the reception of a new firmware.

This commit tackles this issue as it implements the comparison of
software versions on the reception of the first update chunk (which
contains the image header).

So, if CONFIG_MCUBOOT_DOWNGRADE_PREVENTION option is selected, a
comparison will be done between stored and incoming firmware.

If the stored firmware version is strictly superior to incoming one,
flash writing will be bypassed and an error code will be sent to upgrade
sender. Otherwise, the update process is done and stored firmware
is overwritten.

Signed-off-by: Mateo Ligne <[email protected]>
@mateo-ligne-netatmo mateo-ligne-netatmo force-pushed the SW-version-Based-downgrade-prevention branch from 0a246dc to 9f81652 Compare February 8, 2022 14:16
@d3zd3z
Copy link
Member

d3zd3z commented Feb 9, 2022

I believe @utzig concerns have been addressed.

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the stale label Jan 5, 2023
@github-actions github-actions bot closed this Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants