-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Increase support for IronSide SE UICR features #97337
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
Conversation
20f9f6c
to
a5e24d6
Compare
a5e24d6
to
469c331
Compare
469c331
to
2bf681c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a mechanism stopping users from enabling UICR.LOCK and UICR.ERASEPROTECT at the same time by accident.
This configuration does not give much value as one would typically complete the device lock down in run time in the final stages of production.
scripts/ci/check_compliance.py
Outdated
"GEN_UICR_SECURESTORAGE", | ||
"GEN_UICR_WDTSTART", | ||
"GEN_UICR_WDTSTART_CRV", | ||
"GEN_UICR_WDTSTART_INSTANCE_CODE", # Used in specialized build tool, not part of main Kconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is the purpose here to only have the comment on the last entry? If so the top entry still has the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You resolved the comment, but there are still two entries with a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The descriptive comments seem to be established standard in this list, is it really kosher to just drop them?
First and last entry for the GEN_UICR_*
entries seemed like a good compromise to me? Maybe "# Used by designated gen_uicr.py build tool, not part of main Kconfig" would even be a bit better than "not part of main Kconfig"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with having at the beginning and end is that alphabetical sorting is required so the comment would have to move from time to time, and this pollutes commits with unrelated changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, aight works as a reason for me
When enabled, the UICR generator will configure the | ||
watchdog timer to start automatically before the | ||
application core is booted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention that its the local watchdog timer instance as opposed to the global one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When enabled, the UICR generator will configure the
application domain's watchdog timer to start automaticall
before the application core is booted.
Like this maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is specific to the app local domain, I figure we don't need to use the generic local domain term.
When enabled, the UICR generator will lock the entire UICR | ||
configuration in NVR0. Once locked, the UICR can only be modified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it lock all NVR0? Should we specify that its the NVR0 in MRAM10? Which includes BICR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
in production devices to prevent unauthorized modification of the | ||
UICR configuration. | ||
|
||
config GEN_UICR_ERASEPROTECT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should prevent users from enabling this by accident as it will practically brick their device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
2bf681c
to
4111c92
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NACK since IMO we need further hardening against creating "bricking" UICRs.
scripts/ci/check_compliance.py
Outdated
"GEN_UICR_SECURESTORAGE", | ||
"GEN_UICR_WDTSTART", | ||
"GEN_UICR_WDTSTART_CRV", | ||
"GEN_UICR_WDTSTART_INSTANCE_CODE", # Used in specialized build tool, not part of main Kconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You resolved the comment, but there are still two entries with a comment.
soc/nordic/common/uicr/gen_uicr.py
Outdated
# Handle LOCK configuration | ||
if args.lock: | ||
uicr.LOCK = ENABLED_VALUE | ||
# Handle ERASEPROTECT configuration | ||
if args.eraseprotect: | ||
uicr.ERASEPROTECT = ENABLED_VALUE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since bricking devices is very costly, should we have a check in place here as well?
My experience is that these types of script is often pulled out from the context they are made for and brutally exploited to solve some users dire needs. We should help even those users not bricking their boards IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about a
--permanently-transition-device-to-deployed
option to allow this script to be used by:
Users that want to send their device into LCS Deployed, i.e. for production testing purposes.
Users that work on a simulated device and are therefore able to transition back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
4111c92
to
79c88b0
Compare
79c88b0
to
9c37709
Compare
9c37709
to
94de9fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only cosmetic feedback
scripts/ci/check_compliance.py
Outdated
"GEN_UICR_SECURESTORAGE", | ||
"GEN_UICR_WDTSTART", | ||
"GEN_UICR_WDTSTART_CRV", | ||
"GEN_UICR_WDTSTART_INSTANCE_CODE", # Used in specialized build tool, not part of main Kconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The descriptive comments seem to be established standard in this list, is it really kosher to just drop them?
First and last entry for the GEN_UICR_*
entries seemed like a good compromise to me? Maybe "# Used by designated gen_uicr.py build tool, not part of main Kconfig" would even be a bit better than "not part of main Kconfig"
24c4363
24c4363
to
531914a
Compare
Add support for UICR.WDTSTART. UICR.WDTSTART configures the automatic start of a local watchdog timer before the application core is booted. This provides early system protection ensuring that the system can recover from early boot failures. Signed-off-by: Sebastian Bøe <[email protected]>
Add support for UICR.SECONDARY.TRIGGER configuration, which enables automatic booting of secondary firmware based on specific reset reasons. This introduces Kconfig options for configuring: - UICR.SECONDARY.TRIGGER.ENABLE - Enable/disable automatic triggers - UICR.SECONDARY.TRIGGER.RESETREAS - Bitmask of reset reasons that trigger secondary firmware boot Individual Kconfig options are provided for each reset reason: - APPLICATIONWDT0/1 - Application core watchdog timeouts - APPLICATIONLOCKUP - Application core CPU lockup - RADIOCOREWDT0/1 - Radio core watchdog timeouts - RADIOCORELOCKUP - Radio core CPU lockup Signed-off-by: Sebastian Bøe <[email protected]>
Add support for UICR.SECONDARY.PROTECTEDMEM configuration, which enables configuration of the protected memory region for secondary firmware. This introduces Kconfig options for configuring: - GEN_UICR_SECONDARY_PROTECTEDMEM - Enable/disable protected memory for secondary firmware - GEN_UICR_SECONDARY_PROTECTEDMEM_SIZE_BYTES - Size of the protected memory region in bytes The implementation validates that the configured size is divisible by 4096 bytes (4 KiB) as required by the hardware, and converts it to 4 KiB units when writing to UICR.SECONDARY.PROTECTEDMEM.SIZE4KB. Signed-off-by: Sebastian Bøe <[email protected]>
Add support for UICR.LOCK configuration, which locks the entire UICR configuration in NVR0 to prevent unauthorized modifications. This introduces a Kconfig option GEN_UICR_LOCK that enables locking of the UICR. Once locked, the UICR can only be modified by performing an ERASEALL operation. This is a critical security feature for production devices, typically enabled alongside UICR.APPROTECT, UICR.PROTECTEDMEM, and UICR.ERASEPROTECT to establish a complete device protection scheme. When enabled, the gen_uicr.py script sets UICR.LOCK to 0xFFFFFFFF, which configures the NVR0 page as read-only and enforces integrity checks on the UICR content. Signed-off-by: Sebastian Bøe <[email protected]>
Add support for UICR.ERASEPROTECT configuration, which blocks ERASEALL operations to prevent bulk erasure of protected memory. This introduces a Kconfig option GEN_UICR_ERASEPROTECT that enables blocking of ERASEALL operations on NVR0, preserving UICR settings even if an attacker attempts a full-chip erase. This is a critical security feature for production devices. When enabled together with UICR.LOCK, it becomes impossible to modify the UICR in any way, establishing a permanent device protection scheme. Due to this irreversibility, it should only be enabled during the final stages of production. When enabled, the gen_uicr.py script sets UICR.ERASEPROTECT to 0xFFFFFFFF, which prevents the ERASEALL command from affecting the NVR0 page. Signed-off-by: Sebastian Bøe <[email protected]>
Add support for UICR.APPROTECT configuration, which controls debugger and access-port permissions through the TAMPC peripheral. This introduces three Kconfig options that allow independent control over access port protection for different processor domains: - GEN_UICR_APPROTECT_APPLICATION_PROTECTED: Controls debug access to the application domain processor - GEN_UICR_APPROTECT_RADIOCORE_PROTECTED: Controls debug access to the radio core processor - GEN_UICR_APPROTECT_CORESIGHT_PROTECTED: Controls access to the CoreSight debug infrastructure When enabled, each option sets the corresponding UICR.APPROTECT register to PROTECTED (0xFFFFFFFF), which disables debug access for that domain. When disabled, the registers remain at their erased value (UNPROTECTED), allowing full debug access. This feature is critical for production devices where debug access must be restricted to prevent unauthorized access to sensitive code and data. Signed-off-by: Sebastian Bøe <[email protected]>
Add --permit-permanently-transitioning-device-to-deployed safety flag to gen_uicr.py, required when enabling both UICR.LOCK and UICR.ERASEPROTECT together. This prevents accidental permanent locking of devices since this combination makes the configuration irreversible. Signed-off-by: Sebastian Bøe <[email protected]>
531914a
to
a7dd09f
Compare
|
Increase support for IronSide SE UICR features