feat: support SEV-SNP Report Version 5 #115
Conversation
Reviewer's GuideIntroduce support for SEV-SNP Report Version 5 by bumping the sev crate, extending the key-derivation API to include a launch mitigation vector, and updating the CLI and documentation accordingly. Entity relationship diagram for Guest Field Select bits updateerDiagram
DERIVED_KEY {
u64 guest_field_select
u64 launch_mit_vector
}
DERIVED_KEY ||--o| REPORT_V5 : includes
REPORT_V5 {
u64 launch_mit_vector
}
Class diagram for updated KeyArgs struct and key derivationclassDiagram
class KeyArgs {
+Option<u32> vmpl
+Option<String> gfs
+Option<u32> gsvn
+Option<u64> tcbv
+Option<u64> lmv
}
class DerivedKey {
+new(root_key_select, GuestFieldSelect, vmpl, gsvn, tcbv, lmv)
}
KeyArgs --> DerivedKey: used in get_derived_key()
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Buildgit clone https://github.com/hyperfinitism/snpguest -b support-v5-format
cd snpguest
cargo build -rTest 1 (Report Ver 5)Environment
1. Keysudo snpguest key key.bin vcek -v 0 -g 110000
# failed (as intended?)error messagesudo snpguest key key.bin vcek -v 0 -g 110000 -l 0
# successfully fetched2. Reportsudo snpguest report report.bin report-data.bin -r
# successfully fetched3. Displaysnpguest display report report.bin
# successfully displayedresults (selected)Test 2 (Report Version <= 4)Environment
1. Keysudo snpguest key key.bin vcek -v 0 -g 110000
# failed (not intended)error messageThis behavior is not intended, because the sudo snpguest key key.bin vcek -v 0 -g 110000 -l 0
# failed (as intended)error message2. Reportsudo snpguest report report.bin report-data.bin -r
# successfully fetched3. Displaysnpguest display report report.bin
# successfully displayedresults (selected) |
hyperfinitism
force-pushed
the
support-v5-format
branch
from
6da72b5 to
a9cc470
Compare
|
The following changes were made to the initial commit.
Test 1 (Report Ver 5)
sudo snpguest key key.bin vcek -v 0 -g 110000
# => successfully fetched (Msg ver. 1)
sudo snpguest key key.bin vcek -v 0 -g 1100001
# => successfully fetched (Msg ver. 1)
sudo snpguesr key key.bin vcek -v 0 -g 1100001 -l 1
# => successfully fetched (Msg ver. 2)Test 2 (Report Ver 4)
sudo snpguest key key.bin vcek -v 0 -g 110000
# => successfully fetched (Msg ver. 1)
sudo snpguest key key.bin vcek -v 0 -g 1100001
# => Error (Msg ver. 1)
sudo snpguest key key.bin vcek -v 0 -g 110000 -l 1
# => Error (Msg ver. 2) & SEV-guest FW crashederror message (common)error message (after FW crashed) |
|
This is only waiting on a new release of |
| } | ||
| if gfs.chars().any(|c| c != '0' && c != '1') { | ||
| return Err(anyhow::anyhow!( | ||
| "Invalid Guest Field Select option. Must be a binary string." |
There was a problem hiding this comment.
Do we perhaps want to make this input hex? Seems like that would be more reasonable.
There was a problem hiding this comment.
I agree — specifying this as a fixed-length hex string (16 characters for 64 bits) could reduce confusion compared to the current 6–7 bit binary format.
There was a problem hiding this comment.
If we switch the --guest-field-select argument to hex, should we also unify other inputs such as --launch-mit-vector and --tcbv, which are currently parsed as decimal, into hex as well?
Alternatively, we could provide a parser like:
fn parse_u64_multi_format(s: &str) -> Result<u64, ParseIntError> {
if let Some(hex) = s.strip_prefix("0x") {
u64::from_str_radix(hex, 16)
} else if let Some(bin) = s.strip_prefix("0b") {
u64::from_str_radix(bin, 2)
} else {
u64::from_str_radix(s, 10)
}
}and then use it as follows:
#[arg(short, long = "guest_field_select", value_parser = parse_u64_multi_format)]
pub gfs: Option<u64>,This would allow us to support multiple formats. (Though requiring a 0x prefix for hex values might be slightly inconvenient.)
There was a problem hiding this comment.
I've been thinking about this, and I like the idea of supporting multiple formats. There isn’t really a single obvious way to provide this information.
The reason GuestFieldSelect was made a binary string is that each field is represented by a binary number—1 meaning enabled and 0 meaning disabled. Meanwhile, the TCB is also a u64 field, but it has a completely different meaning since you’re providing values of the TCB, where each value can be expressed as a decimal. Then, the MIT VECTOR is a vector of values, so hex makes the most sense there.
If you could extend the parser so we can see what that would look like, we could then implement the same approach in other functionality like this:
#117
There was a problem hiding this comment.
Thanks for the feedback! I'm thinking of implementing this as a separate module (e.g., src/clparser/*) so that it can be reused easily across different subcommands. Using it from each subcommand should be straightforward.
For this PR, since its primary purpose is to add support for the new ABI version, I would suggest keeping (inheriting) the current implementation (--guest_field_select as binary), and then opening a follow-up PR to introduce the multi-format parser. Do you think that separation makes sense?
There was a problem hiding this comment.
Yes, maybe we will cut a minor release when this PR is merged and then make bigger changes when 7.0.0 and the new parser implementation is merged.
|
Yes, once a new release of |
|
Great, thank you. We're currently preparing the |
|
Recently I hit the issue #113 on one of turin servers, applying this patch resolved the issue with report. |
|
Hello @hyperfinitism, We decided to release a 6.3.0 version of the library while we make other major changes to the api prior to 7.0.0. You can probably use this to update the tooling now! Thank you for your patience and contribution |
hyperfinitism
force-pushed
the
support-v5-format
branch
from
a9cc470 to
3e29281
Compare
|
Hi @DGonzalezVillal, Thank you for letting us know about the release. I have replaced Once the argument format issue is addressed, I will mark this PR as ready-for-review. However, if you prefer, I can mark this PR as ready-for-review first and discuss/address the argument format issue in a separate issue/PR. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Replace the
unwrap()onu64::from_str_radixwith proper error handling to prevent potential panics and return a clear user-facing error. - Add validation for the
--launch_mit_vectorvalue (e.g., range or bit‐length checks) similar to how other key parameters are validated. - Pin the Git dependency for the
sevcrate to a specific commit SHA in Cargo.toml for reproducible builds until the official release is available.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Replace the `unwrap()` on `u64::from_str_radix` with proper error handling to prevent potential panics and return a clear user-facing error.
- Add validation for the `--launch_mit_vector` value (e.g., range or bit‐length checks) similar to how other key parameters are validated.
- Pin the Git dependency for the `sev` crate to a specific commit SHA in Cargo.toml for reproducible builds until the official release is available.
## Individual Comments
### Comment 1
<location> `src/key.rs:90` </location>
<code_context>
+ args.lmv,
+ );
+
+ let msg_ver = if args.lmv.is_some() { 2 } else { 1 };
+
let mut sev_fw = Firmware::open().context("failed to open SEV firmware device.")?;
</code_context>
<issue_to_address>
The logic for msg_ver may need clarification for future maintainers.
Please document the dependency between msg_ver and lmv, or ensure updates to lmv semantics are consistently reflected in this logic.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
let msg_ver = if args.lmv.is_some() { 2 } else { 1 };
=======
// The message version (msg_ver) is determined by the presence of the 'lmv' argument.
// If 'lmv' is Some, we use version 2; otherwise, we use version 1.
// IMPORTANT: If the semantics of 'lmv' change, ensure this logic is updated accordingly.
let msg_ver = if args.lmv.is_some() { 2 } else { 1 };
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `README.md:179` </location>
<code_context>
| `$ROOT_KEY_SELECT` | is the root key from which to derive the key (either "vcek" or "vmrk"). | required |
| `-v, --vmpl $VMPL` | option specifies the VMPL level the Guest is running on. | 1 |
-| `-g, --guest_field_select $GFS` | option specifies which Guest Field Select bits to enable as a 6-digit binary string. For each bit, 0 denotes off, and 1 denotes on. | — |
+| `-g, --guest_field_select $GFS` | option specifies which Guest Field Select bits to enable as a binary string (length 6 or 7). For each bit, 0 denotes off, and 1 denotes on. | all 0s |
| `-s, --guest_svn $GSVN` | option specifies the guest SVN to mix into the key. | — |
| `-t, --tcb_version $TCBV` | option specifies the TCB version to mix into the derived key. | — |
</code_context>
<issue_to_address>
Clarify binary string length and update earlier references.
Please ensure all documentation consistently describes the Guest Field Select as a binary string of length 6 or 7.
Suggested implementation:
```
Creates the derived key based on input parameters and stores it. `$KEY_PATH` is the path to store the derived key. `$ROOT_KEY_SELECT` is the root key from which to derive the key (either "vcek" or "vmrk"). The `--guest_field_select` option specifies which Guest Field Select bits to enable as a binary string (length 6 or 7). Each of the bits from *right to left* correspond to Guest Policy, Image ID, Family ID, Measurement, SVN and TCB Version, Launch Mitigation Vector, respectively. For each bit, 0 denotes off, and 1 denotes on. The `--guest_svn` option specifies the guest SVN to mix into the key, the `--tcb_version` option specifies the TCB version to mix into the derived key, and the `--launch_mit_vector` option specifies the launch mitigation vector value to mix into the derived key. The `--vmpl` option specifies the VMPL level the Guest is running on and defaults to 1.
```
```
| `-g, --guest_field_select $GFS` | option specifies which Guest Field Select bits to enable as a binary string (length 6 or 7). For each bit, 0 denotes off, and 1 denotes on. | all 0s |
```
If there are other sections in the README.md that reference Guest Field Select, ensure they also describe it as a binary string of length 6 or 7 for consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| args.lmv, | ||
| ); | ||
|
|
||
| let msg_ver = if args.lmv.is_some() { 2 } else { 1 }; |
There was a problem hiding this comment.
suggestion: The logic for msg_ver may need clarification for future maintainers.
Please document the dependency between msg_ver and lmv, or ensure updates to lmv semantics are consistently reflected in this logic.
| let msg_ver = if args.lmv.is_some() { 2 } else { 1 }; | |
| // The message version (msg_ver) is determined by the presence of the 'lmv' argument. | |
| // If 'lmv' is Some, we use version 2; otherwise, we use version 1. | |
| // IMPORTANT: If the semantics of 'lmv' change, ensure this logic is updated accordingly. | |
| let msg_ver = if args.lmv.is_some() { 2 } else { 1 }; |
tylerfanelli
left a comment
tylerfanelli
left a comment
There was a problem hiding this comment.
LGTM but this PR seems to be mainly relevant for getting the derived key. What does it have to do with the new report version?
|
You're right — the new report version (format) itself is handled by the sev crate bump, which resolves report parsing errors. The main substantive change in this PR is indeed the derived key–related functionality. If you think it would help, I can update the PR title to something more explicit like "Support SEV-SNP ABI Spec 1.58". |
- Bump sev crate to v6.3.0 for supporting ABI Spec Rev 1.58 - Update Cargo.toml and Cargo.lock - Modify the `key` subcommand to handle `launch_mit_vector` - Add description of launch_mit_vector in README.md Signed-off-by: Takuma IMAMURA <[email protected]>
hyperfinitism
force-pushed
the
support-v5-format
branch
from
3e29281 to
fa925c4
Compare
|
Currently as users when we checkout latest tagged version which is v0.9.1, which does not have this fix. Is there a plan to tag v0.9.2 near term so that we can check that out instead of main branch? |
|
@bssrikanth Not sure if you've already been made aware, but we release v0.9.2 of this tool a few days ago. You should be able to use that now. |
|
@tylerfanelli I got notified, thank you. |
5 participants
This PR adds support for SEV-SNP Report Version 5 by updating the code and bumping the
sevcrate.Fixes #113
What's changed
sevcrate to the latest versionlaunch_mit_vector)README.mdto reflect these changesThis PR (draft) is a temporary workaround using an unreleased version of
sev. Once an official release of thesevcrate with Report V5 support becomes available, I will:Cargo.tomlto a versioned release.Cargo.lockaccordingly.Added on 28 Aug 2025
The
sevcrate has been replaced with the latest release v6.3.0. This PR is now ready-for-review.Summary by Sourcery
Add support for SEV-SNP Report Version 5 by upgrading the sev crate, extending the CLI and request logic to include a new launch mitigation vector parameter, and updating documentation to reflect the changes.
New Features:
Enhancements:
Build:
Documentation: