scripts: west: sign: Add support for image signing for Silabs SiWx91x #87669

Aasimshaik11 · 2025-03-26T09:45:22Z

Added changes to automate the signing of zephyr security using customtool during the west build process.

github-actions · 2025-03-26T09:45:54Z

Hello @Aasimshaik11, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

asmellby · 2025-04-04T09:51:46Z

CMakeLists.txt

Please revert these unrelated changes.

Resolved this comment

asmellby · 2025-04-04T09:52:04Z

boards/silabs/radio_boards/siwx917_rb4338a/Kconfig.siwx917_rb4338a

@@ -3,3 +3,25 @@

 config BOARD_SIWX917_RB4338A
 	select SOC_PART_NUMBER_SIWG917M111MGTBA
+
+# Kconfig.board for SiWx917 RB4338A


Unnecessary comment, please remove.

Resolved this comment

asmellby · 2025-04-04T09:53:34Z

boards/silabs/radio_boards/siwx917_rb4338a/Kconfig.siwx917_rb4338a

+
+# Kconfig.board for SiWx917 RB4338A
+
+config SIGNING_ENABLED


Signing support should be a property of the SiWx91x SoC, not of the board. All of this must move to soc/silabs/silabs_siwx91x/, and the Kconfig options must be renamed to show that they apply to 91x only.

Resolved this comment

asmellby · 2025-04-04T09:53:58Z

boards/silabs/radio_boards/siwx917_rb4338a/board.cmake

@@ -11,3 +11,71 @@ include(${ZEPHYR_BASE}/boards/common/silabs_commander.board.cmake)
 # Once started, it should be possible to reset the device with "monitor reset"
 board_runner_args(jlink "--device=Si917" "--speed=10000")
 include(${ZEPHYR_BASE}/boards/common/jlink.board.cmake)
+
+# Custom signing with Silicon Labs Commander


Move to SoC level

Resolved this comment

asmellby · 2025-04-04T09:54:36Z

boards/silabs/radio_boards/siwx917_rb4338a/board.cmake

+
+  # Define paths and keys from Kconfig
+  set(WORKSPACE_ROOT $ENV{PWD})
+  set(SL_COMMANDER_PATH $ENV{PWD}/SimplicityCommander-Linux/commander/commander)


You can't hardcode the path to Commander. You must assume that it exists on PATH.

Ok I resolved this

Resolved this comment

asmellby · 2025-04-04T09:55:48Z

scripts/west_commands/sign.py

@@ -79,6 +80,21 @@
 [rimage] sections in your west config file(s); this is especially useful
 when invoking west sign _indirectly_ through CMake/ninja. See how at
 https://docs.zephyrproject.org/latest/develop/west/sign.html
+
+sirps


This should be silabs_rps or silabs_commander_rps for consistency with the silabs_commander runner.

Resolved this comment

asmellby · 2025-04-04T09:57:01Z

scripts/west_commands/sign.py

+
+        # Primary source: Board Kconfig defaults
+        board_dir = cache.get('BOARD_DIR') # Fallback to PWD if BOARD_DIR missing
+        board_kconfig = pathlib.Path(board_dir) / 'Kconfig.siwx917_r4338a'


You can't hard-code the board Kconfig path, this must generically work for all SiWx91x based boards.

Resolved this comment

asmellby · 2025-04-04T09:57:30Z

boards/silabs/radio_boards/siwx917_rb4338a/board.cmake

+    message(STATUS "Private key found at '${M4_PRIVATE_KEY}'")
+  endif()
+
+  # Define the commander signing command


Why is this calling Commander directly, and not invoking west sign?

The direct call to Silicon Labs Commander in board.cmake is intentional to integrate signing into the west build process, ensuring that zephyr.bin.rps is generated automatically as part of the build output for SiWx917-based boards. This approach is used to meet hardware requirements where a signed binary is mandatory for flashing or execution. Invoking west sign would require a separate post-build step, which we avoid here to streamline the workflow and guarantee that the signed artifact is available immediately after west build, consistent with the board’s firmware deployment needs.

Why would a post-build step need to be separately invoked just because it's calling west instead of Commander? There is no difference if west sign or commander is called as a custom command.

Yeah, that's right. We can do that too, But there will be extra call of west sign that's it.

If west sign used:
Cmakefile -> west sign -> sign.py -> commander for signing

If not west sign:
Cmake -> Commander

IMO it would be better to call west sign, in order to avoid duplicating the implementation. The overhead of an extra call is well worth avoiding two implementations of the same logic.

Resolved this comments

scripts/west_commands/sign.py

soc/silabs/silabs_siwx91x/Kconfig.soc

soc/silabs/silabs_siwx91x/CMakeLists.txt

scripts/west_commands/sign.py

pdgendt · 2025-04-09T09:47:38Z

Please, do not resolve comments as per the contribution guidelines.

soc/silabs/silabs_siwx91x/Kconfig.soc

scripts/west_commands/sign.py

jerome-pouiller · 2025-04-09T18:52:46Z

scripts/west_commands/sign.py

+                        elif current_config == 'M4_PRIVATE_KEY':
+                            m4_private_key = value
+                        elif current_config == 'COMMANDER_PATH':
+                            commander_path = value


6 level of indentation is a lot, especially in a function of 125 lines. Probably you could introduce a subfunction.

Resolved this comment

scripts/west_commands/sign.py

soc/silabs/silabs_siwx91x/CMakeLists.txt

scripts/west_commands/sign.py

soc/silabs/silabs_siwx91x/CMakeLists.txt

marc-hb · 2025-05-16T17:09:43Z

boards/silabs/radio_boards/siwx917_rb4338a/board.cmake

 board_runner_args(silabs_commander "--device=SiWG917M111GTBA" "--file-type=bin"
-    "--file=${PROJECT_BINARY_DIR}/${KERNEL_BIN_NAME}.rps")
+    "--file=${FLASH_FILE}")


In my experience, a build system that produces outputs with changing filenames tends to be really painful to maintain and to use, I don't recommend it. Among others, this variability may require extra logic in a CI building and consuming this output. Is it really important to have a different filename? Instead you could have KERNEL_NAME_pre.rps and KERNEL_NAME.rps. When you don't sign, the latter is either a mere copy of the former, or signed with a development, pseudo-private key.

So FLASH_FILE has a _signed suffix even when CONFIG_SIWX91X_SILABS_SIGN is false? That does not seem to make sense.

Just drop _signed and keep the filenames constant and CONFIG-independent, see #87669 (comment)

In fact, I believe the others tools change property bin_file of the target runners_yaml_props_target (see cmake/mcuboot.cmake:16 or soc/st/stm32/stm32n6x/CMakeLists.txt:35). Probably we should adopt the same design.

Then, we could probably remove the --file=${FLASH_FILE} from our command line.

@marc-hb any objection?

I don't have any blocking objection here, never had. But I've always thought and still think that the simplest and cleanest is to have something like:

KERNEL_BIN_NAME_unsigned.bin or KERNEL_BIN_NAME_pre.bin as west sign input.

Default KERNEL_BIN_NAME.bin as west sign output

When there is no signing, just copy the file. Print some warning that this is a mere copy.

This keeps everything "immutable": files and filenames. It's the best design from a ninja perspective and build troubleshooting perspective.

I believe the others tools change property bin_file ...

This area has always been a mess and I don't think there is a need to be "consistent" with other products there. Are they even consistent with each other in the first place? Consistency helps because it reduces cognitive load. But how many people sign different products on a regular basis? Very few if any.

BTW, for another example of a similar problem please search _prebuilt in zephyr/CMakeLists.txt

scripts/west_commands/sign.py

soc/silabs/silabs_siwx91x/CMakeLists.txt

scripts/west_commands/sign.py

jerome-pouiller · 2025-05-16T07:40:39Z

scripts/west_commands/sign.py

+
+        # Setting the input and output file directories
+        input_rps = b / 'zephyr' / "zephyr.bin.rps"
+        output_rps = args.sbin or (b / 'zephyr' / "zephyr_signed.bin.rps")


I belive you need to honor CONFIG_KERNEL_BIN_NAME:

kernel_name = build_conf.get('CONFIG_KERNEL_BIN_NAME') input_rps = str(b / 'zephyr' / f'{kernel_name}.bin.rps') output_rps = str(b / 'zephyr' / f'{kernel_name}_signed.bin.rps')

jerome-pouiller · 2025-05-22T14:19:13Z

scripts/west_commands/sign.py

-        group.add_argument('-t', '--tool', choices=['imgtool', 'rimage'],
-                           help='''image signing tool name; imgtool and rimage
+        group.add_argument('-t', '--tool', choices=['imgtool', 'rimage', 'silabs_commander'],
+                           help='''image signing tool name; imgtool ,rimage and silabs_commander


space should after the colon.

jerome-pouiller · 2025-05-22T14:33:12Z

soc/silabs/silabs_siwx91x/Kconfig

+config SIWX91X_PRIVATE_KEY
+	string "Private Key Path for Signing."
+	help
+	  Path to private key file for signing with Silicon Labs Commander.


Don't you think we should mention "UG574: SiWx917 SoC Manufacturing Utility User Guide" somewhere?

jerome-pouiller · 2025-05-22T14:38:15Z

soc/silabs/silabs_siwx91x/Kconfig

+config SIWX91X_OTA_KEY
+	string "OTA Key for Encryption."
+	help
+	  Specifies OTA key (hex) for signing/encryption with Silicon Labs Commander.


Does this key only used for OTA or it also works for encrypted XiP ?

BTW, commander uses mic, encrypt and sign. I feel, it would make sense to name these option SIWX91X_ENCRYPT_KEY and SIWX91X_SIGN_KEY.

(BTW, the help message use "signing/encryption" while this key used for integrity check and encryption)

(BTW, the help message use "signing/encryption" while this key used for integrity check and encryption)

Indeed, it's not very clear what does what.

I agree that you should stick to the commander tool terminology. Ideally:
--mic -> SIWX91X_MIC_KEY
--encrypt -> SIWX91X_ENCRYPT_KEY
--sign -> SIWX91X_SIGN_KEY

If that's not possible then stay as close as possible.

If the commander tool terminology is confusing in the first place, then add plenty comments and help text to explain it but do not try to rename anything for a "better" interface. That adds even more confusion.

jerome-pouiller · 2025-05-22T14:40:07Z

scripts/west_commands/sign.py

+        if not siwx91x_ota_key:
+            command.die("SIWX91X_OTA_KEY not provided. Use --siwx91x-ota-key=your_key or add CONFIG_SIWX91X_OTA_KEY in prj.conf.")
+        if not siwx91x_private_key:
+            command.die("SIWX91X_PRIVATE_KEY not provided. Use --siwx91x-private-key=/path/to/key or add CONFIG_SIWX91X_PRIVATE_KEY in prj.conf.")


This behavior does not match with diagram "Secure Firmware Update Flowchart" in
"AN1431: SiWx917 SoC Firmware Update Application Note". Commander also allows to use every option separately.

marc-hb · 2025-05-22T16:28:50Z

soc/silabs/silabs_siwx91x/Kconfig

+config SIWX91X_OTA_KEY
+	string "OTA Key for Encryption."
+	help
+	  Specifies OTA key (hex) for signing/encryption with Silicon Labs Commander.


Suggested change

Specifies OTA key (hex) for signing/encryption with Silicon Labs Commander.

Specifies Over-The-Air key (hex) for signing/encryption with Silicon Labs Commander.

marc-hb · 2025-05-22T16:33:14Z

scripts/west_commands/sign.py

+        if not siwx91x_ota_key:
+            command.die("SIWX91X_OTA_KEY not provided. Use --siwx91x-ota-key=your_key or add CONFIG_SIWX91X_OTA_KEY in prj.conf.")
+        if not siwx91x_private_key:
+            command.die("SIWX91X_PRIVATE_KEY not provided. Use --siwx91x-private-key=/path/to/key or add CONFIG_SIWX91X_PRIVATE_KEY in prj.conf.")


You could drop CONFIG_SIWX91X_SILABS_COMMANDER_SIGN, make each key argument optional and use them as a toggle.

If none of them is found, then have west sign print a warning and just copy the file without signing or encrypting (e.g., copy zephyr_pre.rps -> zephyr.rps). This also avoids outputs with variable filenames: no matter what you do, the build always produces the same files. Also, west sign always runs. Fewer conditionals; simpler flow.

Commander also allows to use every option separately.

So, imagine the user wants to encrypt without signing. Is the final file going to be have some _signed suffix in its name anyway? Confusing. If you never use any _signed suffix in the first place then you don't have that problem. The final filename is always the final file no matter how the build is configured.

I speak from direct and painful experience dealing with builds that produce variable filenames: don't do it.

Added changes to automate the signing of zephyr security using Simplicity Commander during the west build process. Upstream-status: pr <zephyrproject-rtos#87669> Signed-off-by: Aasim Shaik <[email protected]>

marc-hb

The picture is getting a bit clearer but the user interface is still confusing.

As of 0351532, it looks like you're forcing the users to define _KEY strings even when the corresponding boolean is false. You seem to be forcing this in two ways:

CONFIG_SIWX91X_OTA_ENCRYPT_KEY exists even when CONFIG_SIWX91X_SILABS_ENCRYPT disables it.
The following checks are run even when the corresponding boolean is false and the key not used:

        if not siwx91x_ota_sign_key:
            command.die("SIWX91X_OTA_SIGN_KEY not provided. Use --siwx91x-ota-sign-key=/path/to/key or add CONFIG_SIWX91X_OTA_SIGN_KEY in prj.conf."

I think the key problem and the main reason why we seem to be going around in circles a bit is: you don't know yet what the user interface must be. Prototyping is useful and the code in this PR is currently even beyond a great prototype. But at some point one must take a step back from the code and define precisely what features and possibilities this code is meant to provide users. Documentation usually defines that and interestingly enough this PR does not provide any documentation.

So, I think what this PR misses the most is a new section in doc/develop/west/sign.rst. Take a look at 1df0781 for inspiration.

EDIT: new sign.rst section now added, thanks!

marc-hb · 2025-05-29T16:28:26Z

boards/silabs/radio_boards/siwx917_rb4338a/board.cmake

 board_runner_args(silabs_commander "--device=SiWG917M111GTBA" "--file-type=bin"
-    "--file=${PROJECT_BINARY_DIR}/${KERNEL_BIN_NAME}.rps")
+    "--file=${FLASH_FILE}")


So FLASH_FILE has a _signed suffix even when CONFIG_SIWX91X_SILABS_SIGN is false? That does not seem to make sense.

Just drop _signed and keep the filenames constant and CONFIG-independent, see #87669 (comment)

marc-hb · 2025-05-29T16:36:23Z

scripts/west_commands/sign.py

+            sign_base.extend(["--encrypt", str(siwx91x_ota_encrypt_key)])
+
+        if siwx91x_sign_enable:
+            sign_base.extend(["--sign", str(siwx91x_ota_sign_key)])


Why is "OTA" everywhere in the key names but no "OTA" in the name of the booleans? This inconsistency gives the impression that there is some subtle difference between the keys and the booleans. But this code looks like there is none. Confusing.

Added changes to automate the signing of zephyr security using customtool during the west build process. Signed-off-by: Aasim Shaik <[email protected]>

sonarqubecloud · 2025-05-30T11:38:42Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

marc-hb

Thanks @Aasimshaik11 for the new sign.rst section! Makes a big difference IMHO.

Now we need someone very familiar with https://www.silabs.com/documents/public/user-guides/ug574-siwx917-soc-manufacturing-utility-user-guide.pdf to review that sign.rst section and user interface. @jerome-pouiller is that you?

I have resolved all the comments from @asmellby , @jerome-pouiller , @jhedberg , and @pdgendt . Could you mark these comments as resolved and provide any further suggestions or approval for my PR?

I second that: this PR has got old and long and a lot of comments are either resolved or now irrelevant. Yet they are still open. I just reviewed and closed mine. @asmellby and @jerome-pouiller please do the same so this PR becomes less noisy and somewhat readable again.

@Aasimshaik11 , please be cautious but if you applied someone 's suggestion EXACTLY, then you can close it yourself:
https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#workflow-suggestions-that-help-reviewers

Especially if that suggestion is very old: reviewers had their chance.

Also, if someone made a minor typo or codestyle suggestion on some code that does not even exist anymore, then you can close those too. Just make sure you don't close any higher level, more "philosophical" design discussion. These don't need to be closed before merge.

Another option is to abandon this PR and open a fresh new one (that links to the older one). We're probably not there yet but getting close?

marc-hb · 2025-06-25T14:59:01Z

Another option is to abandon this PR and open a fresh new one (that links to the older one). We're probably not there yet but getting close?

Happened in #92181 by @jerome-pouiller

Aasimshaik11 force-pushed the zephyr-boot-security branch 2 times, most recently from 7541fb9 to 7052317 Compare March 26, 2025 11:28

Aasimshaik11 force-pushed the zephyr-boot-security branch from 7052317 to da52b68 Compare April 4, 2025 06:46

jerome-pouiller self-requested a review April 4, 2025 09:55

asmellby requested changes Apr 4, 2025

View reviewed changes

Aasimshaik11 force-pushed the zephyr-boot-security branch 4 times, most recently from 726af6b to b9ddd52 Compare April 8, 2025 10:31

Aasimshaik11 marked this pull request as ready for review April 9, 2025 04:34

github-actions bot added platform: Silabs Silicon Labs area: West West utility labels Apr 9, 2025

github-actions bot requested review from carlescufi, jhedberg, Martinhoff-maker, pdgendt and swinslow April 9, 2025 04:35

github-actions bot assigned pdgendt Apr 9, 2025

pdgendt previously requested changes Apr 9, 2025

View reviewed changes

Aasimshaik11 force-pushed the zephyr-boot-security branch 2 times, most recently from 6015f25 to d329322 Compare April 9, 2025 09:35

Aasimshaik11 force-pushed the zephyr-boot-security branch 2 times, most recently from 82ac254 to 2a2c05b Compare April 9, 2025 11:29

jhedberg reviewed Apr 9, 2025

View reviewed changes

soc/silabs/silabs_siwx91x/Kconfig.soc Outdated Show resolved Hide resolved

soc/silabs/silabs_siwx91x/Kconfig.soc Outdated Show resolved Hide resolved

jerome-pouiller reviewed Apr 9, 2025

View reviewed changes

soc/silabs/silabs_siwx91x/Kconfig.soc Outdated Show resolved Hide resolved

jerome-pouiller reviewed Apr 9, 2025

View reviewed changes

scripts/west_commands/sign.py Outdated Show resolved Hide resolved

jerome-pouiller reviewed Apr 9, 2025

View reviewed changes

jerome-pouiller requested changes Apr 9, 2025

View reviewed changes

Aasimshaik11 force-pushed the zephyr-boot-security branch from 437fe54 to fd7c629 Compare May 16, 2025 05:12

jhedberg requested a review from marc-hb May 16, 2025 16:01

marc-hb reviewed May 16, 2025

View reviewed changes

Aasimshaik11 force-pushed the zephyr-boot-security branch from fd7c629 to c654ef2 Compare May 19, 2025 04:11

Aasimshaik11 requested review from jhedberg, jerome-pouiller, pdgendt, marc-hb and asmellby May 19, 2025 06:25

marc-hb reviewed May 19, 2025

View reviewed changes

scripts/west_commands/sign.py Outdated Show resolved Hide resolved

Aasimshaik11 force-pushed the zephyr-boot-security branch from c654ef2 to ddd06a8 Compare May 20, 2025 04:52

Aasimshaik11 requested a review from marc-hb May 20, 2025 04:52

Aasimshaik11 force-pushed the zephyr-boot-security branch from ddd06a8 to fbdbfca Compare May 22, 2025 09:39

jerome-pouiller reviewed May 22, 2025

View reviewed changes

jerome-pouiller self-requested a review May 22, 2025 15:17

marc-hb reviewed May 22, 2025

View reviewed changes

Aasimshaik11 force-pushed the zephyr-boot-security branch 2 times, most recently from ca2a918 to f91537b Compare May 23, 2025 16:05

Aasimshaik11 force-pushed the zephyr-boot-security branch from f91537b to 0351532 Compare May 29, 2025 10:36

Aasimshaik11 requested a review from marc-hb May 29, 2025 10:37

marc-hb reviewed May 29, 2025

View reviewed changes

Aasimshaik11 force-pushed the zephyr-boot-security branch from 0351532 to 5df1b45 Compare May 30, 2025 09:17

github-actions bot added the area: Documentation label May 30, 2025

Scripts: Security enablement during west build

eef65bb

Added changes to automate the signing of zephyr security using customtool during the west build process. Signed-off-by: Aasim Shaik <[email protected]>

Aasimshaik11 force-pushed the zephyr-boot-security branch from 5df1b45 to eef65bb Compare May 30, 2025 10:43

marc-hb reviewed Jun 6, 2025

View reviewed changes

jerome-pouiller mentioned this pull request Jun 25, 2025

Add support for west sign for Silabs SoCs #92181

Open

	Specifies OTA key (hex) for signing/encryption with Silicon Labs Commander.
	Specifies Over-The-Air key (hex) for signing/encryption with Silicon Labs Commander.

scripts: west: sign: Add support for image signing for Silabs SiWx91x #87669

Are you sure you want to change the base?

scripts: west: sign: Add support for image signing for Silabs SiWx91x #87669

Conversation

Aasimshaik11 commented Mar 26, 2025

Uh oh!

github-actions bot commented Mar 26, 2025

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!

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!

Aasimshaik11 Apr 7, 2025 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pdgendt commented Apr 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Aasimshaik11 Apr 7, 2025 •

edited

Loading

marc-hb May 16, 2025 •

edited

Loading