Skip to content

Harden dev mode value #56

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.

Sign up for GitHub

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

Open
diondokter wants to merge 1 commit into
base: mcxa-secure-boot
Choose a base branch
from
Open

Harden dev mode value #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 33 additions & 9 deletions libs/ec-slimloader-mcxa/src/board.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ use embassy_mcxa::rom::NbootBoolValue;
use embassy_sync::blocking_mutex::raw::NoopRawMutex;
use embedded_storage_async::nor_flash::{ErrorType, NorFlash, NorFlashErrorKind, ReadNorFlash};
use heapless::Vec;
use mcxa_ifr::{LifeCycleState, SecBootEn};
use mcxa_security_provisioning::Provisioner;
use partition_manager::{Partition, PartitionManager, RO, RW};
use static_cell::StaticCell;
Expand Down Expand Up @@ -638,8 +637,7 @@ impl<C: McxaConfig + BootStatePolicy> Board for Mcxa<C> {
}

let Err(if_result) = protected_if::<{ NbootBoolValue::True as u32 }, _, _, _>(
|| match verification::verify_authenticity(self.sgi.reborrow(), image_base, || Self::dev_mode() as u32)
{
|| match verification::verify_authenticity(self.sgi.reborrow(), image_base, || Self::dev_mode()) {
Ok(result) => result as u32,
Err(error) => error as u32, // Error has no overlap in value with Nboot bool, so this is safe to do
},
Expand All @@ -666,17 +664,37 @@ impl<C: McxaConfig + BootStatePolicy> Mcxa<C> {
}

/// Check if the device authenticity should be checked in dev mode (where the keys are self-generated instead of taken from the programmed value)
fn dev_mode() -> bool {
/// Dev mode is active when the returned value is [DEV_VAL]
fn dev_mode() -> u32 {

@felipebalbi felipebalbi Oct 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't like returning a raw u32 here. Can we have a Mode struct with PartialEq, Eq, named secure-boot and lifecycle fields, and constants built from the existing enums instead?

Something like below ought to do it? Maybe add a to_raw() -> u32 method to it for the boundary?

#[derive(PartialEq, Eq)]
struct Mode {
    secure_boot: [SecBootEn; 2],
    lifecycle: Result<LifeCycleState, u8>,
    inverse_lifecycle: Result<InvLifeCycleState, u8>,
}

@alamfarjadf alamfarjadf Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ideally, you would return a raw constant here, a specific made-up pattern that meets certain requirements e.g. the likes of NbootBool values.

@felipebalbi felipebalbi Oct 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A specific fault-resistant pattern makes sense, I agree. I’m not proposing replacing it with an ordinary Boolean. My concern is preserving the contract for future maintainers: a typed wrapper with private construction can enforce sample agreement and lifecycle/inverse consistency, while retaining the observed encoding for protected_if.

My original struct sketch does not enforce those relationships by itself, it needs a little more work :-). The goal is to validate at the boundary and preserve those invariants downstream, rather than make every caller understand an unrestricted u32. The exact representation can be designed in parallel, but the point remains: I would like the invariants to preserved at a constructor that refuses invalid representation centrally. Downstream users use the validated, readable representation only.

@alamfarjadf alamfarjadf Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ah yes, I see what you mean. Agreed.

let ifr = unsafe { mcxa_ifr::IFR::current() };
let secure_boot_enabled = ifr.cmpa.secure_boot_cfg.sec_boot_en() != SecBootEn::OnlyPki;
let develop_lifecycle = ifr.cfpa.header.cfpa_lc_state() == Ok(LifeCycleState::Develop);
// Don't enable dev mode if secure boot is active. That way a flashed key can be tested
develop_lifecycle && !secure_boot_enabled

let secure_boot_cfg_ptr = &raw const ifr.cmpa.secure_boot_cfg;

// Read secure boot twice to harden against fault injection where they saturate a register
let secure_boot1 = unsafe { secure_boot_cfg_ptr.read_volatile() }

@felipebalbi felipebalbi Oct 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What fault model are we protecting against here? The two volatile reads will remain separate accesses, but what are we assuming about the duration and scope of the fault you're protecting against?

@alamfarjadf alamfarjadf Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All zero attractors on the bus/register would mimic an AllAllowed for SB level. Idea is to read twice and cross check to guard against sustained glitches, against which the MCU built in GDET sensors can also activate.

@felipebalbi felipebalbi Oct 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All one faults would pass, though. No?

@diondokter diondokter Oct 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's why it's read twice since supposedly the hardware would catch it if the injection was strong enough to do it twice in a row

@alamfarjadf alamfarjadf Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Correct, they would "pass". But all ones would trigger SEC_BOOT_EN as OnlyPki, at which point DEV mode is NOT enabled, which means no root of trust bypass, which means an attacker trust chain cannot authenticate the image. ROM only trusts what is in IFR. So that attack fails safe to exactly what we need.

The whole point of the glitch hardening here is NOT to let an attacker glitch into DEV mode and have their RoT take over.

@felipebalbi felipebalbi Oct 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To clarify my earlier “all ones” question: I meant saturation of the secure-boot samples while the lifecycle bytes remain unaffected, not saturation of the entire packed word. With the policy corrected, all-ones samples should prevent dev mode.

My remaining question is about common-mode faults: two reads can agree on the same incorrect value. The RM documents ROM-configured GDET/aGDET protection, but §63.1 also requires system characterization, and §63.1.3 requires reconfiguration when core voltage changes. That does not establish that every fault affecting both reads will be detected.

Can we just document the assumed fault scope/duration and detector configuration? I’m asking for a bounded hardening claim, not immunity to every injection: redundant sampling helps against faults confined to one sample; coverage of faults affecting both needs separate justification.

@alamfarjadf alamfarjadf Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we just document the assumed fault scope/duration and detector configuration?

IIRC boot is configured on Normal mode (from SYSCON), so GDET should be on VDD_CORE normal mode too. The SBL will operate as such (please ignore the current board.rs config, it's too fast for flexSPI anyway and I have PR coming changing all that back to Normal). But I have asked NXP to clarify and then we can document the boundaries.

@felipebalbi felipebalbi Oct 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you, I think that should be a good addition here.

.sec_boot_en()
.raw_value()
.value() as u32;
let secure_boot2 = unsafe { secure_boot_cfg_ptr.read_volatile() }
.sec_boot_en()
.raw_value()
.value() as u32;

let lifecycle = match ifr.cfpa.header.cfpa_lc_state() {
Ok(val) => val.raw_value(),
Err(e) => e,
} as u32;
let inv_lifecycle = match ifr.cfpa.header.inv_cfpa_lc_state() {
Ok(val) => val.raw_value(),
Err(e) => e,
} as u32;

(secure_boot1 << 24) | (secure_boot2 << 16) | (lifecycle << 8) | (inv_lifecycle)

@felipebalbi felipebalbi Oct 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we split the reads from the decoding/packing so we can actually test this without dereferencing IFR? At least cover the intended dev configurations, Develop + OnlyPki keeping the programmed keys, disagreement between the two secure-boot samples, and invalid or mismatched lifecycle/inverse bytes. MCXA577 RM (MCXAP172M240F70RM, Rev. 1), section 4.1.3, table 18 gives the lifecycle pairs.

The tests need to exercise the helper used here, not a separate copy of the packing expression.

@alamfarjadf alamfarjadf Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a question that sort of circles back to the double read in my original PR: read twice via volatile then compare and if mismatch, return Err. The concern was although read_volatile contractually obligates the compiler to perform two reads, there is no obligation for it to use both values attained (if it can ascertain they might match). Isn't there such a concern here too? Why wouldn't the compiler just repeat the first read for the second one without any explicit guidance or asm guards?

@felipebalbi felipebalbi Oct 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the compiler trully follows its own documentation and that of LLVM, I don't think we can get a situation where the compiler reuses the first value for the second. We will definitely get two reads and two fresh values. @diondokter?

}

/// Check if the secure boot settings are good
fn follows_policy() -> bool {
if Self::dev_mode() {
if Self::dev_mode() == DEV_VAL {
true
} else {
unsafe { Provisioner::new_from_page(true) }
Expand All @@ -686,6 +704,12 @@ impl<C: McxaConfig + BootStatePolicy> Mcxa<C> {
}
}

pub const DEV_VAL: u32 = 0b00000011_00000011_00000011_11111100;

@felipebalbi felipebalbi Oct 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isn't this still the opposite of what #54 fixes? Both secure-boot bytes here are SecBootEn::OnlyPki, so a Develop device takes the self-derived-key path precisely when PKI enforcement is configured. We won't be testing the flashed keys in that configuration. See MCXA577 RM (MCXAP172M240F70RM, Rev. 1), section 8.4.1 for SEC_BOOT_EN.

Also, #54 allows every value except OnlyPki; one constant only matches one exact configuration. Do we want AllAllowed only, or all three non-OnlyPki configurations? Just changing these bytes to zero would not fully carry over #54.

@alamfarjadf alamfarjadf Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

isn't this still the opposite of what #54 fixes? Both secure-boot bytes here are SecBootEn::OnlyPki, so a Develop device takes the self-derived-key path precisely when PKI enforcement is configured. We won't be testing the flashed keys in that configuration. See MCXA577 RM (MCXAP172M240F70RM, Rev. 1), section 8.4.1 for SEC_BOOT_EN.

@felipebalbi is correct here (thanks!!), it reverses the fix from PR 54. We want DEV mode when NOT secure boot on with OnlyPki, the hybrid SB mode.

Also, #54 allows every value except OnlyPki; one constant only matches one exact configuration. Do we want AllAllowed only, or all three non-OnlyPki configurations? Just changing these bytes to zero would not fully carry over #54.

We need "NOT OnlyPki" because OnlyPki is the only one that will authenticate the SBL itself with ECDSA + MLDSA signatures via boot ROM on boot. So OnlyPki = no DEV MODE, other three = DEV mode.

// SecSecBootEn::OnlyPki ------- ^^
// SecSecBootEn::OnlyPki ---------------- ^^
// LifeCycleState::Develop ----------------- ^^^^^^^^
// InvLifeCycleState::Develop ----------------------- ^^^^^^^^

#[cfg(test)]
mod tests {
use super::*;
Expand Down
3 changes: 2 additions & 1 deletion libs/ec-slimloader-mcxa/src/verification.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use fault_rejection::protected_if;
use mcxa_ifr::{IFR, RotkEn, RotkUsageVal};
use mcxa_security_provisioning::certificate::derive_image_rkth_pair;

use crate::board::DEV_VAL;
use crate::error::map_nboot_status_to_boot_error;

fn load_nboot_auth_parms_from_ifr(ifr: &IFR) -> Result<NbootImgAuthParms, BootError> {
Expand Down Expand Up @@ -83,7 +84,7 @@ pub fn verify_authenticity<'d>(
defmt_or_log::trace!("Initializing NBOOT context");
let mut n_boot_api = embassy_mcxa::rom::get().nboot().map_err(|_| BootError::Authenticate)?;

let result = protected_if::<{ true as u32 }, Result<(), BootError>, _, _>(dev_mode, || {
let result = protected_if::<DEV_VAL, Result<(), BootError>, _, _>(dev_mode, || {

@felipebalbi felipebalbi Oct 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This also changes the meaning of the public callback without changing its type. A caller returning 1 still compiles, but no longer selects dev mode. A typed value would make that contract clearer.

We can still encode the observed fields into a word at the protected_if boundary, though.

@alamfarjadf alamfarjadf Oct 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

DEV mode is a very specific scenario that MSFT needs, a normal bootloader ideally should not bypass its secure roots of trust. So, passing a 1 which compiles and bypasses DEV mode here would be international I believe, as you'd want to NOT use DEV mode.

Only the select pattern lets you though, all else verifies against the devices fused root of trust.

// In dev mode, we're going to lie to nboot and derive the rkth's ourselves so they're always correct
// This makes the provisioning process easier. Once no longer in dev mode, this branch is not taken and
// only the flashed keys are used
Expand Down
Loading