tbor(sd): bring the Security Domain command family up on hardware - #688
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Enables the TBOR Security Domain (SD) command family to run on real hardware by fixing a Uno PKA ECC public-key derivation fault and aligning partition SD one-shot state handling with key lifetime, then removing emulator-only gating from the SD + KeyReport integration tests.
Changes:
- Firmware: route
ecc_pub_from_privthrough the provenecc_priv_pub_keypath to avoid the PKA operand-slot fault on silicon. - Firmware: clear
sd_initializedfor all partition reset kinds so the SD one-shot flag cannot outlive the SDMK handle/material. - Tests: remove
#[cfg(feature = "emu")]gating and rename SD/KeyReport tests so they run on hardware-capable backends.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| fw/plat/uno/fw/pal/src/crypto/ecc.rs | Switch public-key derivation to ecc_priv_pub_key to avoid hardware PKA operand addressing fault and add scalar validation. |
| fw/plat/uno/fw/drivers/part_store/src/part_store.rs | Clear sd_initialized in the shared reset path so SD one-shot state follows SDMK lifetime across reset kinds. |
| ddi/tbor/types/tests/commands/sd_sealing_key_gen.rs | Remove emu-only gating; clarify OOB requirements are in setup, not the SD command itself; rename tests. |
| ddi/tbor/types/tests/commands/sd_create_remote_backup.rs | Remove emu-only gating; document SD one-shot behavior and reset interactions; rename tests. |
| ddi/tbor/types/tests/commands/sd_create_peer_backup.rs | Remove emu-only gating and rename tests to run on hardware. |
| ddi/tbor/types/tests/commands/sd_reseal_remote_backup.rs | Remove emu-only gating and rename tests to run on hardware. |
| ddi/tbor/types/tests/commands/sd_restore_local_backup.rs | Remove emu-only gating and rename tests to run on hardware. |
| ddi/tbor/types/tests/commands/sd_restore_peer_backup.rs | Remove emu-only gating and rename tests to run on hardware. |
| ddi/tbor/types/tests/commands/sd_restore_remote_backup.rs | Remove emu-only gating and rename tests to run on hardware. |
| ddi/tbor/types/tests/commands/key_report.rs | Remove emu-only gating and rename tests to validate KeyReport on hardware. |
Suppressed comments (1)
fw/plat/uno/fw/pal/src/crypto/ecc.rs:767
ecc_pub_from_privused to callmap_ecc_curve(curve)?to fail gracefully ifHsmEccCurvegains new variants that this PAL/PKA driver doesn’t support. After switching toecc_priv_pub_key, the function no longer validatescurvebeyond length checks, which could silently accept a future curve variant that happens to share a wire length with an existing curve and compute the public key on the wrong curve.
let wire_pub_len = curve.wire_pub_key_len();
if priv_key.len() != curve.wire_priv_key_len() || pub_key.len() < wire_pub_len {
return Err(HsmError::InvalidArg);
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes hardware-backed cryptographic execution and persistent partition state semantics, which warrants final human review alongside silicon validation evidence.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
Rajib Dutta (radutta99)
changed the base branch from
user/radutta/tbor-part-final
to
main
September 8, 2026 17:29
The handler has been implemented and dispatched at opcode 0x09 for a while, and the host types existed, but all 13 tests sat behind `#![cfg(feature = "emu")]`, so the command had zero silicon coverage. Un-gate the file and drop the now-misleading `_emu` suffixes. No production code changes -- this is purely test gating. It compiles for the hardware profile with no further un-gating because the part_final work already exposed `x509_fixture` and `part_policy_with_pota`. Worth recording for the rest of the SD family: `SdSealingKeyGen` carries no out-of-band data. The request is a session id plus a 1-byte scope and the response is a 180-byte masked key plus a 96-byte public key, so the command itself runs on any transport. What needs OOB is the *setup* -- `finalized_co_session` drives `PartFinal`, whose PTA chain travels out of band -- so the two roundtrip tests need the driver's data-transfer path while the four reject tests do not. Hardware: SdSealingKeyGen 6/6 (both roundtrips included), full TBOR 199/199 (was 193, +6 newly enabled), part_final still 10/10. Emu 6/6 and full emu TBOR 302/302. clippy `-D warnings` and fmt clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6e9403d-692c-4903-97ee-87f2606151c3 (cherry picked from commit 59d8d22)
`ecc_pub_from_priv` called the PKA driver's `ecc_gen_pub_key`, which issues the point-mul opcode with a null (0) scalar operand. The engine then reads the scalar from address 0 rather than GSRAM and faults with an AXI BUS_ERROR (0x08f0c003). This was already known: the deterministic-keygen path hit it, worked around it in `ecc_det.rs`, and left a NOTE at line 872 naming the exact status. This call site kept using the broken primitive, and nothing caught it because `key_report` -- its main consumer -- was emulator-only, and the std PAL does not model PKA operand addressing. Green emu, faulting silicon; the same blind spot as the PRP/SGL descriptor bug in part_final. Delegate to `ecc_priv_pub_key`, which already performs the multiply with every operand in a GSRAM DMA buffer. The curve argument is redundant rather than lost: that helper infers the curve from the scalar length, which is `wire_priv_key_len()` -- defined as `wire_coord_len()` -- so the mapping is identical, and it additionally range-checks the scalar against the curve order. Un-gate the `key_report` tests, which had zero hardware coverage and are what proves the fix. `sd_create_remote_backup` is left emulator-only, with the reason recorded in its module docs: it provisions the SDMK behind a one-shot gate that resets only on partition free / NSSR, so on hardware the 8 tests are mutually exclusive -- the first create after a boot passes and the rest return SdAlreadyInitialized (0x08700108), even across separate test runs. The firmware path itself is fine: the roundtrip passes on silicon after a power cycle. Un-gating needs a harness-driven way to reset the one-shot. Hardware: KeyReport 6/6 (was faulting), full TBOR 205/205, SdSealingKeyGen 6/6, part_final 10/10. Emu 302/302. clippy `-D warnings` and fmt clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6e9403d-692c-4903-97ee-87f2606151c3 (cherry picked from commit a68ab27)
…amily on hw `clear_state` cleared the SD one-shot only under `PartResetKind::Disable`, so it survived an NSSR (`Migrate`). But the same function drops the SDMK handle (`set_sd_mk_key_id(None)`) for *every* reset kind, and the caller wipes the vault material, so after an NSSR the partition claimed a security domain whose masking key no longer existed -- and `SdCreateRemoteBackup`'s one-shot gate then refused to mint a replacement. The flag outlived its key, permanently. This is a divergence from the std PAL, which clears `sd_initialized` in both `part_enable` and `clear_enabled_state`. That is exactly why it was invisible: on emu each reset clears the flag and the tests isolate cleanly, while on uno the state persisted across NSSR -- and across separate `cargo test` runs -- so only silicon could show it. Move the clear into the unconditional section, next to the key-handle drop it has to follow. `bk3_initialized` is deliberately left in the `Disable` block: its `sealed_bk3` blob *is* preserved by `Migrate`, so that flag stays consistent with its material. With the flag fixed the whole SD family becomes self-isolating on hardware, so un-gate all six remaining files. Hardware: SD family 32/32 in a single ordinary suite run with no per-test reset, and repeatable back-to-back. Full TBOR 231/231 (was 205, +26). Emu 302/302. clippy `-D warnings` and fmt clean. Found by a discovery run that lifted every emu gate and re-ran each failure behind a PCI FLR; triage for the remaining non-SD gates is in the session notes (AES CMD_ERROR on 5 crypto paths, PendingKeyGeneration on 20, UnsupportedKeyScope on 7). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6e9403d-692c-4903-97ee-87f2606151c3 (cherry picked from commit 6713fb2)
`sd_create_remote_backup`'s module docs still claimed the file had to stay emulator-only because the SD one-shot survived the harness factory reset. That stopped being true in the same series: clearing `sd_initialized` on every reset kind made the file self-isolating, and it was un-gated and now passes 8/8 on hardware. Replace it with what a reader actually needs — that the one-shot exists and is asserted by `sd_create_remote_backup_is_one_shot`, that `clear_state` clears it on the `Migrate` the harness reset drives, and why the old behaviour was invisible on emu. Docs only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6e9403d-692c-4903-97ee-87f2606151c3 (cherry picked from commit 879c4ed)
Review feedback. `ecc_pub_from_priv` takes a `curve` but delegates to `ecc_priv_pub_key`, which re-derives the curve from the scalar length, so `curve` was only ever checked implicitly via `wire_priv_key_len()`. Its docs promise `InvalidArg` for an unsupported curve, and that contract was not actually enforced. Call `map_ecc_curve(curve)?` up front to restore it. That closes the stated contract, but not the deeper worry: the delegation is only sound while no two curves share a wire scalar length. Today they are 32 / 48 / 68, pairwise distinct. Rather than leave that as a comment, pin it with `wire_priv_key_len_is_unambiguous` in the traits crate, which fails if a future variant collides -- so the failure lands in CI instead of the firmware quietly computing on the wrong curve. Hardware (firmware checksum 07F18538): SD family 32/32, key_report 6/6, full TBOR 231/231. Emu 302/302, traits unit tests 5/5, clippy `-D warnings` and fmt clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6e9403d-692c-4903-97ee-87f2606151c3 (cherry picked from commit 6e6aaf0)
Rajib Dutta (radutta99)
force-pushed
the
user/radutta/tbor-sd-sealing-key
branch
from
September 8, 2026 17:46
6de3e0a to
54e5358
Compare
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes hardware-facing ECC/PKA behavior and partition persistent reset semantics in security-critical paths, which warrants final human review despite strong test validation.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes address concrete silicon-only faults with clear invariants (GSRAM-backed PKA operands, reset-state consistency) and are supported by expanded test coverage now running on hardware.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
The ALL list claimed that adding an HsmEccCurve variant would force it to be updated, but nothing enforced that: a new variant could be added without touching ALL, and wire_priv_key_len_is_unambiguous would still pass while silently skipping the new curve -- exactly the case the test exists to catch. Add index_in_all, a wildcard-free match, so a new variant stops the module compiling until it is given an arm; all_lists_every_curve then fails unless it is also appended to ALL at the matching index. Verified by temporarily adding a fourth variant: the build fails in check_match on index_in_all (E0004). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6e9403d-692c-4903-97ee-87f2606151c3
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are narrowly scoped to hardware-enablement fixes with corresponding test un-gating, and no correctness issues were found in the updated logic or test harness interactions.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
Vishal Soni (vsonims)
previously approved these changes
Sep 8, 2026
Preserve the SD sealing-key transport documentation while incorporating main's independent hardware enablement in #692 and subsequent changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b6e9403d-692c-4903-97ee-87f2606151c3
Vishal Soni (vsonims)
approved these changes
Sep 8, 2026
Vishal Soni (vsonims)
enabled auto-merge
September 8, 2026 19:42
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are narrowly scoped, address confirmed silicon-only failures, and are backed by expanded unit/integration test coverage with no additional issues found in the reviewed diffs.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brings the TBOR Security Domain command family up on real hardware. All 7 SD commands — 32
tests — now run and pass on silicon; they were emulator-only before.
KeyReport(6 tests) comeswith them.
Why these were gated
Two real firmware bugs, both invisible on the emulator because the std PAL models neither PKA
operand addressing nor persistent device state.
1.
ecc_pub_from_privfaulted the PKAIt called the driver's
ecc_gen_pub_key, which issuesECC_POINT_MULwith the operands in thewrong slots:
The engine reads the scalar from address 0 rather than GSRAM and faults with AXI
BUS_ERROR(
0x08f0c003).The PKA has no base-point-multiply command at all —
ECC_KEY_GEN_*makes a fresh keypair,and
ECC_POINT_MULalways needs the point supplied explicitly. So the fix delegates toecc_priv_pub_key, which already does the multiply with every operand in a GSRAM DMA buffer andadditionally range-checks the scalar against the curve order.
This was already known: the deterministic-keygen path hit it and left a NOTE in
ecc_det.rs. Thiscall site kept using the broken primitive because
key_report, its main consumer, had zerohardware coverage.
The now-dead
ecc_gen_pub_keyis deliberately left in place — removing a public driver API isout of scope here, and is filed as a follow-up.
2.
sd_initializedoutlived its keyclear_statecleared the SD one-shot only underPartResetKind::Disable, so it survived an NSSR(
Migrate). But the same function drops the SDMK handle for every reset kind, and the callerwipes the vault — so after an NSSR the partition claimed a security domain whose masking key no
longer existed, and
SdCreateRemoteBackup's one-shot gate then refused to mint a replacement,permanently.
This diverges from the std PAL, which clears the flag in both
part_enableandclear_enabled_state— exactly why the emulator never saw it.bk3_initializedis deliberately left in theDisableblock: itssealed_bk3blob is preservedby
Migrate, so that flag stays consistent with its material.Commits
925314cbSdSealingKeyGenon hardware (test-gating only)e82b1a50key_report9e1eb367sd_initializedfix + un-gate the remaining 6 SD files4c2fdf9e54e53585ecc_pub_from_privThis branch was previously stacked on #666. Now that #666 has merged it is rebased onto
main,and the history above is only the SD work — the superseded #666 commits it used to carry are gone,
along with a stale emulator OOB page builder that #666's final form made wrong. Net diff is
141 insertions across 11 files.
Coverage
SdSealingKeyGenSdCreateRemoteBackupSdCreatePeerBackupSdResealRemoteBackupSdRestoreLocalBackupSdRestorePeerBackupSdRestoreRemoteBackupPlus
KeyReport6/6, which was faulting before and is what proves the PKA fix.Validation
Firmware
080C5974, rebased onto currentmain.KeyReport(hardware)ci-tbor-emu)ci-emu-ddi)--workspace --all-targets -D warningscargo fmt --all --check, both workspaces-D warningsThe SD suite was previously re-verified in 3 consecutive runs with no reset and no driver reload
between them — it is genuinely self-isolating, which is the point of the
sd_initializedfix.Not in scope
Non-SD emulator gates remain, with 36 genuine hardware failures triaged but unfixed: AES
CMD_ERRORon 5 crypto paths,PendingKeyGenerationon 20, andUnsupportedKeyScopeon 7.