firmware_uefi: add telemetry on secureboot configuration - #4100
firmware_uefi: add telemetry on secureboot configuration#4100Maheer Aeron (maheeraeron) wants to merge 27 commits into
Conversation
There was a problem hiding this comment.
🟡 Not ready to approve
There are correctness issues to address (notably missing size_of/size_of_val scope and misleading/absent telemetry in some cases) plus missing unit test coverage for the new signature parsing/comparison logic.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds Secure Boot telemetry for firmware_uefi by comparing the PK/KEK/db/dbx variables loaded in NVRAM against the baseline Secure Boot template (when present), and emitting trace events with revision/config metadata.
Changes:
- Introduces a baseline template revision string (
BASELINE_REVISION) and re-exports it viafirmware_uefi_resources. - Parses the base template into
BaseTemplateVarsup-front and adds Secure Boot signature parsing + set-diff logic to evaluate missing baseline signatures. - Adds
cvm_tracingusage for allowed/confidential telemetry fields and exposes template signature access viaBaseTemplateVars::signatures().
File summaries
| File | Description |
|---|---|
| vm/devices/firmware/hyperv_secure_boot_templates/src/lib.rs | Adds a baseline revision constant for built-in Secure Boot templates. |
| vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs | Adds Secure Boot configuration telemetry and signature parsing/comparison helpers; refactors base template parsing flow. |
| vm/devices/firmware/firmware_uefi/Cargo.toml | Adds cvm_tracing dependency for telemetry tagging. |
| vm/devices/firmware/firmware_uefi_resources/src/lib.rs | Re-exports BASELINE_REVISION for consumers. |
| vm/devices/firmware/firmware_uefi_custom_vars/src/lib.rs | Adds BaseTemplateVars::signatures() accessor needed for telemetry evaluation. |
| Cargo.lock | Records the new cvm_tracing dependency in the lockfile. |
Review details
Comments suppressed due to low confidence (1)
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:342
- The warn message "base secure boot template variable is missing" is misleading: in this branch the variable exists, but
missing_entriesindicates some baseline signatures are absent. Consider updating the wording to reflect missing signatures rather than a missing variable.
"base secure boot template variable is missing"
- Files reviewed: 5/6 changed files
- Comments generated: 4
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The updated nvram/mod.rs introduces compile-breaking missing imports and has a couple of correctness/clarity and coverage gaps that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (5)
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:33
size_of/size_of_valare used in this module (e.g.,signature_list_payloadandhandle_nvram_command_inner) but are not imported or qualified, which will not compile.
use std::borrow::Cow;
use std::collections::BTreeSet;
use std::fmt::Debug;
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:331
- This log message indicates the whole variable is "present", but the check is specifically that baseline signatures are not missing (it doesn't assert equality, and extra entries may exist). Updating the message would make the telemetry less ambiguous.
missing_entries,
loaded_variable_bytes,
"base secure boot template variable is present"
);
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:237
- The new secure-boot telemetry path adds non-trivial parsing logic (
collect_signature_set/signature_list_payload) and comparison behavior inreport_secure_boot_configuration, but the test module only exercises first-boot injection/delta parsing. Adding unit tests for auth-header stripping and signature-set comparisons would help prevent regressions.
async fn report_secure_boot_configuration(
&mut self,
base_template_vars: Option<&BaseTemplateVars>,
custom_uefi_config_present: bool,
) {
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:128
base_template_jsonis parsed intoBaseTemplateVars(and then cloned) unconditionally on every non-restore cold boot, even when NVRAM is not empty andinject_vars_on_first_bootwill return early. This adds avoidable JSON parsing/allocation work to boot paths where the base template is only needed for first-boot injection and/or secure boot telemetry.
let base_template_vars = base_template_json
.map(|template_json| {
hyperv_uefi_custom_vars_json::parse_template_json(template_json.as_bytes())
})
.transpose()
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:343
- This log message says the variable is "missing", but
missing_entries > 0means the variable was loaded and parsed successfully and is only missing some baseline signatures. Consider wording it as missing baseline signatures to avoid misinterpretation.
loaded_entries,
missing_entries,
loaded_variable_bytes,
"base secure boot template variable is missing"
);
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
Two new telemetry log messages are misleading (they describe variable presence/absence rather than baseline signature match/mismatch), which can cause incorrect interpretation by telemetry consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (2)
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:348
- The warn-level telemetry message says the baseline secure-boot variable is missing, but the code is reporting that some baseline signatures are missing from the loaded variable (the variable itself was loaded earlier). This wording can mislead telemetry/alerting.
"baseline secure boot template variable is missing"
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:336
- The info-level telemetry message says the baseline secure-boot variable is present, but at this point the variable is already known to exist; the check is actually that no baseline signatures are missing from the loaded variable. This wording can mislead telemetry consumers.
This issue also appears on line 348 of the same file.
"baseline secure boot template variable is present"
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The change is additive (telemetry + helpers), includes targeted unit tests for the new parsing/diff logic, and does not alter Secure Boot enforcement behavior.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
ca0ffb7 to
de654ae
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
Telemetry logging includes at least one misleading message and a small unreachable-path implementation detail that should be corrected to avoid confusing downstream telemetry consumers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:268
baseline_revisionis computed as eitherBASELINE_REVISIONor "none", but the function returns early when there is no baseline template/signatures; as written, the "none" branch is unreachable and can confuse future readers.
// Get the baseline revision
let baseline_revision = if base_template_vars.is_some() {
BASELINE_REVISION
} else {
"none"
};
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:299
- When the variable exists but its data is empty (
Ok((_, data))withdata.is_empty()), the warning says it is "missing in NVRAM". That’s misleading (the variable is present but empty) and makes it hard to distinguish corruption/empty-state from a true NOT_FOUND case.
let loaded_variable = match self.services.get_variable_ucs2(vendor, name).await {
Ok((_, data)) if !data.is_empty() => data,
Ok((_, data)) => {
tracing::warn!(
CVM_ALLOWED,
variable_name,
loaded_variable_bytes = data.len(),
"loaded secure boot template variable is missing in NVRAM"
);
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
It introduces avoidable per-boot JSON parsing/allocation overhead by parsing the base template unconditionally even when it isn’t needed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:152
base_template_jsonis parsed unconditionally inNvramServices::neweven when NVRAM is not empty (not first boot) and Secure Boot is disabled, which adds avoidable JSON parsing and allocations on every boot. Previously the parse was deferred until the first-boot injection path.
if !is_restoring {
let base_template_vars = base_template_json
.map(|template_json| {
hyperv_uefi_custom_vars_json::parse_template_json(template_json.as_bytes())
})
- Files reviewed: 5/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Changes recommended
The new secure boot report test has a likely-failing GUID string match and uses an unwrap() on kmsg parsing that can unnecessarily panic the test.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
vmm_tests/vmm_tests/tests/tests/multiarch/secureboot.rs:91
- The GUID check is using a lowercase literal, but
guid::Guidformats (Debug/Display) with uppercase hex. TheSecureBootConfigReportlog will therefore containtemplate_guid: 272E...and this assert will fail even when the correct template is reported.
assert!(
raw.contains("template_guid: 272e7447-90a4-4563-a4b9-8e4ab00526ce"),
"unexpected Secure Boot template GUID in report: {raw}"
);
vmm_tests/vmm_tests/tests/tests/multiarch/secureboot.rs:83
KmsgParsedEntry::new(&data)returns aResult; usingunwrap()here will panic the whole test if a single malformed kmsg entry is encountered (even if the SecureBootConfigReport entry itself is fine). Skipping invalid entries makes this watcher more robust and matches existing kmsg consumers elsewhere in the repo.
let data = data.context("reading kmsg")?;
let message = kmsg::KmsgParsedEntry::new(&data).unwrap();
let raw = message.message.as_raw();
- Files reviewed: 13/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of concrete robustness/maintainability issues in the new test and NVRAM injection helper that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
vmm_tests/vmm_tests/tests/tests/multiarch/secureboot.rs:83
KmsgParsedEntry::newcan fail (e.g., non-UTF8 or malformed /dev/kmsg entries). Usingunwrap()here will panic the test on a single bad entry, even though the test could just ignore unparsable lines and keep waiting for the telemetry report.
while let Some(data) = kmsg.next().await {
let data = data.context("reading kmsg")?;
let message = kmsg::KmsgParsedEntry::new(&data).unwrap();
let raw = message.message.as_raw();
vm/devices/firmware/firmware_uefi/src/service/nvram/mod.rs:249
inject_initial_varsis documented as injecting into empty NVRAM (and logs "No NVRAM variables (first boot)"), but it no longer checks that the backing store is actually empty. Reintroducing the guard avoids accidental double-injection if this helper is ever called from another path and keeps the log message accurate.
/// Inject hard-coded and configured UEFI variables into empty NVRAM.
async fn inject_initial_vars(
&mut self,
base_template_vars: Option<&BaseTemplateVars>,
custom_uefi_json: Option<UefiVarsDeltaJson>,
- Files reviewed: 13/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🔵 Human review recommended
It changes Secure Boot/NVRAM initialization behavior and adds new telemetry/parsing logic in a security-sensitive area that warrants final human review.
Review details
Suppressed comments (1)
vmm_tests/vmm_tests/tests/tests/multiarch/secureboot.rs:95
- The
template_versionassertion uses a substring match ("template_version: 4"), which would also pass for versions like40or41, potentially letting an incorrect template version slip through. Making the match delimiter-aware (e.g., including the trailing comma fromDebugoutput) makes this check precise and less flaky if versions change in the future.
assert!(
raw.contains("template_guid: 272e7447-90a4-4563-a4b9-8e4ab00526ce"),
"unexpected Secure Boot template GUID in report: {raw}"
);
assert!(
raw.contains("template_version: 4"),
"unexpected Secure Boot template version in report: {raw}"
);
- Files reviewed: 13/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a likely wire-format breaking change by changing UefiConfig field #1 from transparent bytes to a message type, which needs an explicit compatibility strategy before landing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
vm/devices/firmware/firmware_uefi_resources/src/lib.rs:160
UefiConfigis a#[derive(Protobuf)]message where field numbers default to struct field order (1, 2, ...). Changing field #1 fromOption<BaseTemplateJson>(a#[mesh(transparent)]bytes field) toOption<BaseTemplate>(a message) is a wire-format breaking change: older binaries will decode field 1 as bytes and fail when they receive a message, and vice-versa.
If any UefiConfig payloads cross version-skew boundaries (mesh/ttrpc, saved state, host<->OpenHCL, etc.), this needs an explicit compatibility strategy (e.g. keep the existing bytes field number and add identity as new fields at the end, or introduce base_template under a new field number while continuing to accept the legacy bytes field).
/// Static configuration for the UEFI device.
#[derive(Clone, Protobuf)]
pub struct UefiConfig {
pub base_template: Option<BaseTemplate>,
pub custom_uefi_json: Option<UefiVarsDeltaJson>,
vmm_tests/vmm_tests/tests/tests/multiarch/secureboot.rs:84
kmsg::KmsgParsedEntry::new(&data).unwrap()will panic (and potentially mask the real failure) if the kmsg line is malformed or non-UTF8. Since this is a test helper, returning a contextualanyhowerror will produce better diagnostics and avoid an unconditional panic.
let data = data.context("reading kmsg")?;
let message = kmsg::KmsgParsedEntry::new(&data).unwrap();
let raw = message.message.as_raw();
vmm_tests/vmm_tests/tests/tests/multiarch/secureboot.rs:99
- The test asserts
template_versionusing{:x}formatting. The firmware-side telemetry printstemplate_versionvia derivedDebugfor au16(decimal), so this is easy to desync and will start failing if the version ever becomes >= 10 (e.g.10vsa). Using decimal formatting keeps the assertion aligned with the actual log output.
assert!(
raw.contains(&format!(
"template_version: {:x},",
template_identity.version
)),
"unexpected Secure Boot template version in report: {raw}"
);
- Files reviewed: 14/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🔵 Human review recommended
It modifies core Secure Boot/NVRAM initialization and introduces new telemetry parsing/comparison logic on a sensitive boot-time path that warrants final human review.
Review details
- Files reviewed: 14/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Note
This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.
Adds logic to evaluate each authenticated secure boot variable against the baseline template, if applicable, and reports telemetry based on the results and other secure boot configuration parameters