A: Include ID block when launching guests - #165
Draft
markg-github wants to merge 19 commits into
Draft
Conversation
markg-github
force-pushed
the
fix/harness-bugs
branch
2 times, most recently
from
August 17, 2026 21:32
be72ac2 to
732ae03
Compare
execute_test assigned the loop's local `profile` onto `ctx.profile` at the top of every iteration. A callable step that replaces the profile — `dataclasses.replace` on a frozen VMProfile returns a new object, so mutating it in place is not possible — had that replacement silently overwritten before the next step ran. Any handler that reconfigures the guest for subsequent steps was therefore a no-op. Read the profile out of the context instead of writing the stale local back over it.
run_vm_launch_step reported "error" unconditionally when vm_launch raised VMLaunchError, ignoring the step's expected_result. That made it impossible to write a step that expects a launch to be refused: QEMU exiting immediately raises rather than returning ok=False, so a step declaring expected_result="exit_code:1" errored on exactly the outcome it was asserting. Evaluate expected_result against exit code 1 and the exception text, and record exit_code=1 so the result is self-describing. Steps that do not declare an expectation still default to "exit_code:0" and so still report error, leaving existing behavior unchanged.
execute_certification folded per-test results with
if tr.result != "pass":
overall = tr.result
which is last-writer-wins, not worst-wins. A run where the first test
errors and a later one merely fails reported the certification as
"fail", losing the error. The bug is latent today because both outcomes
are non-passing, but it silently downgrades severity in reports, and any
new non-passing state makes it worse.
Fold with an explicit severity ordering instead.
vm_launch checks whether QEMU is still alive exactly once, at wait_ready_seconds, and never again. wait_for_guest then polls vsock until vsock_boot_timeout with no reference to the process at all. If QEMU exits after that single check, the harness spends the rest of the boot timeout pinging a VM that no longer exists. Measured on an EPYC 9654 with a deliberately corrupted ID block measurement, sampling the process at 1 Hz: +14.6s .. +17.2s QEMU alive +17.2s .. gone step ran +13.8s .. +195.9s QEMU lived 2.6s; the step took 182.1s. ~179s was spent waiting on a dead process. Policy violations are rejected at SNP_LAUNCH_START before guest memory is loaded, so QEMU dies inside the initial check and those launches already fail in ~2s — which is why only the measurement case was slow. Pass the process handle into wait_for_guest and abort as soon as it exits. The step drops from 182.1s to ~3s and the id-block suite from ~200s to ~21s. The diagnostics matter more than the speed. Firmware had already reported the exact cause: SNP_LAUNCH_FINISH ret=-5 fw_error=11 'Bad measurement' and the harness discarded it in favour of "Vsock agent on CID ... not ready after 180.0s". A negative test asserting exit_code:1 is satisfied by that timeout just as well as by a real rejection, so it would pass identically if the guest merely hung or if the ID block were ignored entirely. The failure message now carries QEMU's exit code and its stderr tail, so a rejected launch states the firmware's reason. process defaults to None, preserving the old behavior for any caller that does not supply it.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> fix: parameterize the QEMU process type annotation guest_vsock used a bare subprocess.Popen while VMLaunchResult.process is annotated subprocess.Popen[bytes]. Bare Popen reads as Popen[Any] to a type checker, losing the stream element type for no benefit — only .poll() and .returncode are used. Match the existing annotation. refactor: split the severity comparison into named locals The single-expression comparison ran to 107 characters and read poorly, with _RESULT_SEVERITY.get(..., unknown_severity) appearing twice on one line. Name both severities and compare them. Behavior is identical: an unrecognised state still ranks above every known one, so a typo surfaces loudly rather than masking a real failure. That rationale moves from a comment into the docstring, where it is visible to callers.
markg-github
force-pushed
the
pr/id-block
branch
from
August 19, 2026 17:22
fb7ec93 to
22d3413
Compare
Referenced qemu-kvm-core package to utilize the qemu package required for sev workflow, and removed qemu package to reduce the host artifact size during its build. Signed-off-by: Harika Nittala <lnittala@amd.com>
Signed-off-by: Harika Nittala <lnittala@amd.com>
Update certification matrix update workflow to comply with conventional commit tag name. Selected docs: as this workflow should only ever touch .md files. Signed-off-by: Amanda Liem <aliem@amd.com>
Signed-off-by: Harika Nittala <lnittala@amd.com>
Convert the host-only TCB config test into a mixed-scope test that verifies config changes are reflected in guest attestation reports. Test by inspecting attestation reports in these cases: 1. Fresh VM (booted after config-set-lower): guest reported == host reported tcb guest current == host platform tcb signed by lowered host reported tcb 2. Live VM (same VM as 1 after config-reset): guest reported == restored host reported tcb guest current == host platform tcb Only the alternate lower tcb VCEK is fetched, as normal is covered by 3.0.0-0 attestation test and don't want to trigger KDS rate-limit. A final info-type config-reset ensures TCB cleanup regardless of test outcome. Signed-off-by: Amanda Liem <aliem@amd.com>
markg-github
force-pushed
the
pr/id-block
branch
from
September 1, 2026 14:44
de430ee to
784475b
Compare
Adds shared callables (calculate_measurement, generate_id_block) in sev_verify/id_block.py for use by any test that requires an ID block. Extends VMProfile with id_block/id_auth fields so vm_launch passes them to QEMU when present. Fixes the step loop in cli.py so callable steps can update ctx.profile before vm_launch sees it. fix: pass policy from ID block to QEMU sev-snp-guest object docs: clarify why profile is re-read from ctx each step iteration feat: add actual ID block test with report verification and negative launches Check expected_result on VMLaunchError in run_vm_launch_step so that vm_launch steps can declare expected_result="exit_code:1" for launches that should be rejected by firmware. Add id_block_test at cert level 3.0.0-2: - Positive: launch with valid ID block, verify guest_svn, policy, family_id, image_id in the attestation report via snpguest display - Negative: bad measurement (digest mismatch) - Negative: SMT=0 policy on SMT-active host (platform incompatibility) - Negative: ABI_MAJOR=255 (impossible firmware version) Add vm_stop (type=info) after each negative launch step so that launch is reset to None regardless of whether VMLaunchError fired or the launch unexpectedly succeeded. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> fix: validate guest measurement at each point of use guest_measurement.txt was read without any length or format check. A present-but-malformed file reached snpguest as an opaque argument, surfacing as a confusing subprocess error, and verify_report_fields read it with no guard at all — an absent file raised FileNotFoundError. Add read_measurement() with MeasurementMissing / MeasurementMalformed, validating the 96-character hex digest (48-byte MEASUREMENT field) at each consumer rather than at write time: the producer runs in an earlier step, so a check there says nothing about what a later step reads. Absence stays distinguishable from corruption. generate_id_block still exits 0 and skips when the file is missing (additive principle), but now fails when it is present and malformed. feat: verify ID block fields by parsing report.bin directly verify_id_block_fields shelled out to `snpguest display report` and recovered four fields with regexes over its human-readable output. That couples a hardware assertion to a CLI's formatting: the labels come from the sev crate's Display impl, not from snpguest itself, so a crate bump can silently turn "field mismatch" into "field not found" and fail the test for a reason that has nothing to do with the platform. Parse the binary structure instead. ATTESTATION_REPORT has a fixed layout, and unlike the CLI text it is self-describing — VERSION is the first four bytes, so every report states its own layout and can be checked on the spot rather than inferred from a tool probe. Layout is version-dependent, tracking firmware and so CPU generation, but versions have only ever appended fields: everything below 0x188 is common to v2 and v3, and the CPUID triple at 0x188 is v3+. All four fields this test needs sit in the stable head.
Addresses the FMC/Turin review comments on AMDEPYC#247. The gap was worse than "unsupported": TCB_VERSION is laid out differently on Turin, and the two layouts are indistinguishable from the eight bytes alone. byte Milan/Genoa Turin/Venice 0 BOOT_LOADER FMC 1 TEE BOOT_LOADER 2 -- TEE 3 -- SNP 6 SNP -- 7 MICROCODE MICROCODE Decoding a Turin report with the legacy layout yielded BOOT_LOADER=FMC, TEE=BOOT_LOADER, SNP=0 — plausible values, no error. The sev crate handles this by dispatching on processor Generation (from_legacy_bytes/from_turin_bytes); we hardcoded one layout. Resolve a generation from the host's CPUID family/model, which is authoritative for a report produced by a guest on this machine and, unlike the report's own CPUID copy, present regardless of report version. parse() takes it as an argument so it stays a pure function of its input, unit-testable without hardware. When the report is v3+ it carries CPUID too, and the two are cross-checked: a mismatch means the report did not come from this machine. SUPPORTED_GENERATIONS records what has been validated on hardware, not what we believe we could decode — currently Genoa alone. An unrecognised processor raises rather than assuming a transcribed layout is right: a certification harness reporting a pass on silicon it has never run on is the failure this gate exists to prevent. Layouts for generations not yet exercised are recorded alongside it, so adding one is an entry plus a validation note. Keyed on family/model pairs rather than family/model/stepping triples: AMD scopes SEV firmware images by family and model only, and snpguest's get_processor_model partitions the same way. The module docstring now also records that this is a short-term stand-in. snpguest already parses reports correctly and generation-aware via the sev crate but exposes them only as human-readable text; once it gains machine-readable output this module should be replaced by consuming that rather than extended to cover further generations. test: assert why a negative launch was rejected, not just that it failed Addresses the review comment asking whether the negative launches could check the reason for failure rather than accepting any launch error. expected_result="exit_code:1" is satisfied by anything that fails, including a boot timeout — so these steps could not distinguish a real firmware rejection from a hung guest, and would have passed even if the ID block were ignored entirely. The harness fix merged as AMDEPYC#289 makes the firmware's own text reachable from both launch-failure paths (the raise path passes str(exc) through _check_expected_values; the ok=False path now carries QEMU's stderr tail), so it can be asserted directly. Observed on an EPYC 9654 at BIOS 1.10.6: bad measurement SNP_LAUNCH_FINISH ret=-5 fw_error=11 'Bad measurement' SMT policy SNP_LAUNCH_START ret=-22 fw_error=0 '' ABI version SNP_LAUNCH_START ret=-5 fw_error=7 'Policy is not allowed' The SMT case is refused by KVM before the firmware sees it, so it has no firmware string to match; it asserts SNP_LAUNCH_START instead, which still rules out a boot timeout. Worth noting that this test therefore exercises KVM's policy validation rather than the firmware's. refactor: drop the duplicated calculate_measurement feat: improve SMT sub-test fix: satisfy crypto module dependency This fixes sev-certify automated runs. For sev_verify only runs, it may or may not. See the updated sev_verify/readme.md. fix: decode reports whose firmware left the CPUID fields empty The parser refused any report whose CPUID bytes did not resolve to a validated generation, reporting it as an unsupported processor. That is wrong when the processor is fine and the firmware simply did not populate the field: SEV firmware 1.55 build 38 leaves all three bytes zero in version-3 reports, where build 39 fills them in. On such a platform every report was rejected, and the error blamed the CPU. The host's CPUID is authoritative — it describes the silicon this code runs on, which is what the TCB_VERSION layout depends on, and it is present regardless of report version. The report's copy is a cross-check, useful only when it resolves. So an unresolvable one is now recorded in a new cpuid_note field and decoding proceeds with the host's generation. The exception is kept for the case that does indicate a problem: both CPUIDs resolve to validated generations and disagree, meaning the report came from another machine. That branch only becomes reachable once a second generation is added to SUPPORTED_GENERATIONS; with one validated generation every disagreeing CPUID is unresolvable instead. That ordering is deliberate — a report is called foreign only when both generations are ones we have actually validated against. With no host generation supplied and an unresolvable CPUID, TCB_VERSION is now left undecoded rather than raising, matching what a v2 report already does. docs: say how to add a report version, not just a processor SUPPORTED_GENERATIONS explains how to validate and add a processor generation; KNOWN_VERSIONS only stated which versions were readable and refused the rest, leaving the reader to work out what adding one involves. Record the procedure, and that versions 4 and 5 already exist and are refused. Note the cheap case explicitly: the sev crate's ReportVariant mapping groups versions sharing a layout, so a version grouped with one already listed reads identically, which makes v4 straightforward and v5 the one needing scrutiny. Also note that additions are not always new offsets — v5's GuestPolicy and PlatformInfo changes are new bits in existing fields. State that version and generation are independent axes, since the two gates look redundant until it is clear that one decides which fields exist and the other how TCB_VERSION is ordered. feat: validate Turin and report version 5 on hardware Adds the second processor generation and the second report version, both confirmed on an EPYC 9575F rather than transcribed. The Turin TCB_VERSION layout had been taken from the sev crate and never exercised. A version 5 report from that machine carried REPORTED_TCB bytes 0103020600000062, which decode under the Turin layout to bootloader=3 tee=2 snp=6 microcode=98 fmc=1 — matching snphost show tcb exactly. The same bytes under the legacy layout give bootloader=1 tee=3 snp=0 microcode=98 and no FMC: plausible values, silently wrong. That is the failure this gate exists to prevent, now demonstrated rather than asserted. That report also showed version 5 moves none of the fields read here: TCB at 0x180 and CPUID at 0x188 both decoded correctly, so v5's additions are the new GuestPolicy and PlatformInfo bits the crate documents rather than relocations. Version 4 remains refused, never having been seen. Two consequences worth noting. The generation mismatch branch in parse() was unreachable while only one generation was validated; with two, a report from the wrong machine is now genuinely detected. And Turin firmware 1.58 populates the report CPUID fields that Genoa's BIOS-supplied 1.55 build 38 leaves zeroed, so both sides of that behaviour are now covered by tests.
The Layout section listed five of the ten modules. attestation_report.py and cvm_props.py are introduced by this branch and were never added; environment.py, os_info.py and output.py predate it. "How it works" described a manifest entry as "name, scope, module path", but TestDefinition also carries level and host_changes. host_changes was documented only in the flags table, which is not where someone writing a test looks — and the judgement it requires is not obvious, so the distinction is stated: launching a guest does not count, changing platform configuration does. feat: decode newer report versions rather than refusing them An unknown report version was refused outright, the same treatment given an unknown processor. The two mistakes are not comparable. Misidentifying the processor generation means misreading bytes that are present: TCB_VERSION decodes to plausible but wrong values with nothing in the data to reveal it. That is worth refusing over. A newer report version is the opposite — fields have only ever been appended, so everything read here stays where it was and the cost is missing what is new rather than misreading what is old. The version is also self-describing, sitting in the first four bytes and always present, which is exactly what the generation is not. The gate had fired exactly once in practice, on Turin's version 5 reports, and it was wrong to fire: it would have refused a report that decodes correctly, which is how the Turin support in this branch was nearly missed. The sev crate is already permissive here, mapping any unrecognised version onto its newest variant. So a version at or above the validated range is decoded with the newest validated offsets and the assumption recorded in a new version_note field. Versions below the range are still refused, since there fields may genuinely not exist rather than merely going unread. KNOWN_VERSIONS now means validated, not accepted.
The ID_BLOCK_* environment variables were read in six places: once to build
the ID block, once to check the resulting report, and four more times across
the negative cases that rebuild it. Each site parsed them independently, and
the checker derived its expectations from the environment rather than from
what the generator had actually used — so the two could drift and the test
would still report a clean pass or an unexplained mismatch.
Read them once, in cvm_props.read_id_block_metadata(), and share the result.
The conversions are now guarded. int() on a non-numeric value or
encode("ascii") on a non-ASCII one previously raised out of the handler as a
bare ValueError or UnicodeEncodeError; they now fail the step with a message
naming the variable at fault.
FAMILY_ID and IMAGE_ID are length-checked against the 16-byte field. ljust()
pads but never truncates, so an over-long value produced an expectation longer
than the report field, which could never match and reported itself as a byte
diff that did not say why.
Policy is carried as an int from the point it is read, rather than being
re-parsed from a string at each negative case. VMProfile already accepts
str | int and formats it, so nothing downstream changes.
markg-github
force-pushed
the
pr/id-block
branch
from
September 3, 2026 20:41
fe98ac2 to
76448c0
Compare
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
3 participants
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.
Review-preview PR against the fork, to see the diff without adding noise upstream. Mirrors the content of AMDEPYC#247.
What this adds
A new certification test
c3_0_0_2 id-block-testthat launches an SEV-SNP guest with an ID block, verifies the hardware attestation report reflects the ID block fields, and confirms three launches that must be rejected: corrupted measurement, platform-incompatible policy (SMT), and an impossible ABI major version.Shared ID block generation lives in
sev_verify/cvm_props.pyso other tests can reuse it. Keys are ephemeral P-384, generated per run and discarded — the signature only has to satisfy the firmware requirement that the ID block be signed by the key named inID_AUTH_INFO.Notable pieces
Measurement validation at the point of use.
guest_measurement.txtwas read with no length or format check, andverify_report_fieldsread it with no guard at all.read_measurement()now validates the 96-character hex digest at each consumer rather than at write time — the producer runs in an earlier step, so a check there says nothing about what a later step reads. Absence stays distinguishable from corruption: a missing file still exits 0 and skips ID block generation (additive principle), a malformed one fails.Report fields are read from
report.bindirectly, not by regexingsnpguest display report. The binary layout is fixed by the SEV-SNP ABI and the report is self-describing — VERSION is the first four bytes — whereas the CLI text comes from thesevcrate’s Display impl and can move under us. Every offset insev_verify/attestation_report.pywas pinned against a real v3 report from an EPYC 9654, cross-checked against independently known values rather than taken from a spec reading.snphost okstaysrequired. An earlier commit downgraded it toinfo; that was a local workaround for one test server and is reverted here.Verification
Run end-to-end on an EPYC 9654 (Genoa, CPUID 19h/11h).
id-block-testpasses, with all three negative launches correctly rejected and the family ID / image ID / SVN / policy confirmed bound in the hardware report.Known gaps
ID_AUTH_INFOis all zeros; author key support is not implemented.Launch with bad measurementstep takes 182s. Diagnosed and fixed separately in branchfix/launch-detect-qemu-exit(PR C) — QEMU exits after 2.6s and the harness keeps polling vsock for the remaining ~179s.Stop VMsteps reporterrorafter a correctly-rejected launch, because there is no guest left to stop. Benign; they areinfotype.