Repository navigation
Conversation
Signed-off-by: Satya Benson <satya@benson.earth>
Signed-off-by: Satya Benson <satya@benson.earth>
55b5b35 to
87d3222
Compare
scottjones
left a comment
There was a problem hiding this comment.
Thanks @satchlj, this is the right direction: every change makes a reader refuse rather than guess, and none of them adds a write. We checked the strict readers against the writers that exist today. IdentityIndex::save writes exactly 8 + count * 144 bytes. keybag::mark_refused writes a header-only record with zero length, UUID and secret length, and a CRC over the same preimage the new check computes. Neither writer's format has changed since the driver landed, so we don't see a valid record on an existing Mac that the new readers would reject.
We also ran the new tests and reverted each fix by hand to check they bite. Restoring the NULL collapse in store_shim.c, letting sks_machine_refkey create a key after any open error, dropping the CRC check on the refused-keybag record, and accepting a truncated index each fail a test.
One gap: nothing tests the caller side. With catacomb::is_absent changed back to treat a read error as absent (catacomb.rs, the new is_absent), or with read_stored's error arm in restore_catacomb mapped back to NoStoredFile (sbio.rs:654), both new test files still pass. restore_catacomb's error arm (sbio.rs:654) runs on every boot, so a test that an unreadable catacomb gives Failed, not NoStoredFile, is the one worth adding next to test_identity_preservation.py; the same for the opt-in probe_owner_export (sbio.rs:513).
Small nit in the description: the refused record is written by mark_refused, not write_intent. write_intent writes state 0, which read already treats as EEXIST.
What we ran: both new test files pass (8 tests, rustc 1.93.1). We built drivers/soc/apple/ with the release19 config (LLVM=1 W=1) on a merge of this branch and #180 (at 0dfa5ae), and it builds clean with no new warnings in the SEP files. #180's newest commit (21d2c81) now also touches keybag.rs, shim.rs and store_shim.c, so whichever lands second will need a look at those three files.
We'd be happy to approve once the caller test is in.
catacomb::read() mapped every failure to None, which the enrollment, restore, deletion and owner-probe paths read as "no saved catacomb". Return Result<Option<_>> so that only a missing file reads as absent, add is_saved()/is_absent() for the callers, and make read_stored() propagate read errors so restore reports Failed rather than NoStoredFile. Test both callers that run on these paths: restore_catacomb() must fail without contacting the enclave on an unreadable catacomb or host store, and the owner probe must stop before its enclave check when the owner or master file cannot be read. Drop a source-text assertion made redundant by the native boundary test. Signed-off-by: Satya Benson <satya@benson.earth>
87d3222 to
accecc6
Compare
|
Thanks @scottjones. The caller test is in (
All nine tests pass (rustc 1.93.1, normal and On #180: if it lands first, I'll rebase this onto it and recheck Written by Claude on @satchlj's behalf. |
scottjones
left a comment
There was a problem hiding this comment.
Thanks, that covers it. We checked accecc6 against 87d3222: the only change is the new caller test in test_identity_preservation.py, so the driver code we built and reviewed is unchanged. The test compiles the real restore_catacomb, read_stored, probe_owner_export, is_saved and is_absent bodies out of sbio.rs and catacomb.rs, and it applies both mutations we named, with a guard that each mutation actually changed the code, so it can't pass vacuously. We read the test rather than running it this time. Approving.
An unreadable or partial saved record must not become evidence that nothing is saved. Several readers currently make that substitution, and the callers then act on "absent": creating a replacement reference key, a second keybag, or a fresh enrollment context. Those actions orphan state held by the enclave.
Changes
from_err_ptrconventionopen_hostalready follows.sks_machine_refkeycreates a new key after any open failure. It now creates one only after a genuine ENOENT.IdentityIndex::loadaccepts short input as empty, caps oversized counts and returns a prefix of a truncated file. It now requires a complete header, a bounded count and the exact length the writer produces.keybag::readtrustsSTATE_REFUSEDbefore checking the rest of the record. It now requires the exact size, checksum and zero fields thatmark_refusedwrites. Valid committed records are still accepted.catacomb::readreturnsOptionand maps every failure toNone, which the enrollment, restore, deletion and owner-probe paths read as "no saved catacomb". It now returnsResult<Option<_>>, whereOk(None)means only that the file is missing. Callers useis_saved(present and valid) oris_absent(provably missing).read_storedpropagates read errors, so restore reportsFailedinstead ofNoStoredFile. A host-store read error is no longer treated as "not stored" either.Behavior change: a damaged index, refusal record or catacomb now fails loudly instead of silently reading as empty. That is the intent, but such a device now needs manual repair instead of quietly re-enrolling over its saved state.
Storage formats, open flags, the enrollment protocol and xART policy are unchanged.
Validation
tools/aurora-sep/tests/test_store_open_errors.py: compiles the actual C wrapper bodies against a fakeopen_as_kernel, plus the unchangedshim.rsagainst a small kernel-API facade. Success and six errno values are preserved through C and Rust. Only successful handles are closed. A negative control restores the old NULL collapse and fails.tools/aurora-sep/tests/test_identity_preservation.py: compiles the production reference-key, index, refused-keybag and catacomb readers against in-memory seams, in the style oftest_sbio_exchange.py. Each test also restores the old behavior and confirms the test catches it. A caller test compiles the realrestore_catacomb,read_storedandprobe_owner_export: an unreadable catacomb or host store restores asFailedwithout contacting the enclave, and the owner probe stops before its enclave check when the owner or master file cannot be read. Mapping the error arm back toNoStoredFile, or lettingis_absentaccept a read error, fails it.git diff --checkis clean.make LLVM=1kernel and modules build of this branch (arm64, Rust enabled,CONFIG_APPLE_SEP=m) succeeds, andapple-sep.kolinks.drivers/soc/apple/builds with no warnings atW=1. Clippy reports nothing new on changed lines.Notes for follow-up work (not changed here)
These came from observing one M3 enrollment that saved the owner and user catacombs but not the master. They are recorded to help whoever works on recovery next. No code here depends on them.
Partial-state recovery. After a completed enrollment, the host can be left with some catacombs saved and others missing. Today a missing catacomb at restore is
NoStoredFile, and later enrollment can select a fresh context for that component. For the master, that discards the enclave-side state the saved owner and user catacombs depend on. A recovery design probably needs:NoExistingCatacombalready models this for the owner probe);Enclave acknowledgment handling. In the observed case the master component reported save-pending state (0x7) after the user catacomb was saved and confirmed, but
SAVE_CATACOMB(0x6c) for the master was refused with status 0x6. The existing comment insave_all_componentsdescribes the same pattern on m2fix11. Status 0x6 has no name yet. Open questions:/chosen/apfs-preboot-uuidunavailable. That correlation is not shown to be the cause.Until those are answered, treating any non-OK save status as a hard stop before further saves or confirmations, and reporting which components did persist, seems the safest default.