Merge changes from topic "instance_id_as_salt" into main
* changes:
Backup instance_id & reuse it for rerunning VM
pvmfw: Defer rbp checks & instance.img is obsolete
diff --git a/microdroid_manager/src/vm_secret.rs b/microdroid_manager/src/vm_secret.rs
index ed8ab1d..ec40b45 100644
--- a/microdroid_manager/src/vm_secret.rs
+++ b/microdroid_manager/src/vm_secret.rs
@@ -109,15 +109,14 @@
.ok_or(anyhow!("Missing explicit dice chain, this is unusual"))?;
let policy = sealing_policy(explicit_dice_chain)
.map_err(|e| anyhow!("Failed to build a sealing_policy: {e}"))?;
- if super::is_new_instance() {
- // New instance -> create a secret & store in Secretkeeper.
- *skp_secret = rand::random();
- store_secret(&mut session, id, skp_secret.clone(), policy)
- .context("Failed to store secret in Secretkeeper")?;
+ if let Some(secret) = get_secret(&mut session, id, Some(policy.clone()))? {
+ *skp_secret = secret;
} else {
- // Subsequent run of the pVM -> get the secret stored in Secretkeeper.
- *skp_secret = get_secret(&mut session, id, Some(policy))
- .context("Failed to get secret from Secretkeeper")?;
+ log::warn!(
+ "No entry found in Secretkeeper for this VM instance, creating new secret."
+ );
+ *skp_secret = rand::random();
+ store_secret(&mut session, id, skp_secret.clone(), policy)?;
}
}
Ok(Self::V2 {
@@ -248,21 +247,24 @@
session: &mut SkSession,
id: [u8; ID_SIZE],
updated_sealing_policy: Option<Vec<u8>>,
-) -> Result<[u8; SECRET_SIZE]> {
+) -> Result<Option<[u8; SECRET_SIZE]>> {
let get_request = GetSecretRequest { id: Id(id), updated_sealing_policy };
log::info!("Secretkeeper operation: {:?}", get_request);
let get_request = get_request.serialize_to_packet().to_vec().map_err(anyhow_err)?;
let get_response = session.secret_management_request(&get_request)?;
let get_response = ResponsePacket::from_slice(&get_response).map_err(anyhow_err)?;
let response_type = get_response.response_type().map_err(anyhow_err)?;
- ensure!(
- response_type == ResponseType::Success,
- "Secretkeeper get failed with error: {:?}",
- *SecretkeeperError::deserialize_from_packet(get_response).map_err(anyhow_err)?
- );
- let get_response =
- *GetSecretResponse::deserialize_from_packet(get_response).map_err(anyhow_err)?;
- Ok(get_response.secret.0)
+ if response_type == ResponseType::Success {
+ let get_response =
+ *GetSecretResponse::deserialize_from_packet(get_response).map_err(anyhow_err)?;
+ Ok(Some(get_response.secret.0))
+ } else {
+ let error = SecretkeeperError::deserialize_from_packet(get_response).map_err(anyhow_err)?;
+ if *error == SecretkeeperError::EntryNotFound {
+ return Ok(None);
+ }
+ Err(anyhow!("Secretkeeper get failed: {error:?}"))
+ }
}
#[inline]
diff --git a/pvmfw/src/dice.rs b/pvmfw/src/dice.rs
index 540fd03..67865e5 100644
--- a/pvmfw/src/dice.rs
+++ b/pvmfw/src/dice.rs
@@ -93,7 +93,8 @@
rkp_vm_marker: bool,
salt: [u8; HIDDEN_SIZE],
}
-
+ // TODO(b/291213394): Include `defer_rollback_protection` flag in the Hidden Input to
+ // differentiate the secrets in both cases.
hash(HiddenInput { rkp_vm_marker: self.rkp_vm_marker, salt: *salt }.as_bytes())
}
diff --git a/pvmfw/src/main.rs b/pvmfw/src/main.rs
index 12d63d5..2af19c4 100644
--- a/pvmfw/src/main.rs
+++ b/pvmfw/src/main.rs
@@ -42,10 +42,12 @@
use crate::instance::{get_recorded_entry, record_instance_entry};
use alloc::borrow::Cow;
use alloc::boxed::Box;
+use bssl_avf::Digester;
use core::ops::Range;
-use diced_open_dice::{bcc_handover_parse, DiceArtifacts};
+use cstr::cstr;
+use diced_open_dice::{bcc_handover_parse, DiceArtifacts, Hidden};
use fdtpci::{PciError, PciInfo};
-use libfdt::Fdt;
+use libfdt::{Fdt, FdtNode};
use log::{debug, error, info, trace, warn};
use pvmfw_avb::verify_payload;
use pvmfw_avb::Capability;
@@ -129,18 +131,6 @@
}
}
- if verified_boot_data.has_capability(Capability::SecretkeeperProtection) {
- info!("Guest OS is capable of Secretkeeper protection");
- // For Secretkeeper based Antirollback protection, rollback_index of the image > 0
- if verified_boot_data.rollback_index == 0 {
- error!(
- "Expected positive rollback_index, found {:?}",
- verified_boot_data.rollback_index
- );
- return Err(RebootReason::InvalidPayload);
- };
- }
-
let next_bcc = heap::aligned_boxed_slice(NEXT_BCC_SIZE, GUEST_PAGE_SIZE).ok_or_else(|| {
error!("Failed to allocate the next-stage BCC");
RebootReason::InternalError
@@ -153,43 +143,51 @@
RebootReason::InternalError
})?;
- let (recorded_entry, mut instance_img, header_index) =
- get_recorded_entry(&mut pci_root, cdi_seal).map_err(|e| {
- error!("Failed to get entry from instance.img: {e}");
- RebootReason::InternalError
- })?;
- let (new_instance, salt) = if let Some(entry) = recorded_entry {
- // The RKP VM is allowed to run if it has passed the verified boot check and
- // contains the expected version in its AVB footer.
- // The comparison below with the previous boot information is skipped to enable the
- // simultaneous update of the pvmfw and RKP VM.
- // For instance, when both the pvmfw and RKP VM are updated, the code hash of the
- // RKP VM will differ from the one stored in the instance image. In this case, the
- // RKP VM is still allowed to run.
- // This ensures that the updated RKP VM will retain the same CDIs in the next stage.
- if !dice_inputs.rkp_vm_marker {
- ensure_dice_measurements_match_entry(&dice_inputs, &entry).map_err(|e| {
- error!(
- "Dice measurements do not match recorded entry.
- This may be because of update: {e}"
- );
+ let (new_instance, salt) = if cfg!(llpvm_changes)
+ && should_defer_rollback_protection(fdt)?
+ && verified_boot_data.has_capability(Capability::SecretkeeperProtection)
+ {
+ info!("Guest OS is capable of Secretkeeper protection, deferring rollback protection");
+ // rollback_index of the image is used as security_version and is expected to be > 0 to
+ // discourage implicit allocation.
+ if verified_boot_data.rollback_index == 0 {
+ error!("Expected positive rollback_index, found 0");
+ return Err(RebootReason::InvalidPayload);
+ };
+ // `new_instance` cannot be known to pvmfw
+ (false, salt_from_instance_id(fdt)?)
+ } else {
+ let (recorded_entry, mut instance_img, header_index) =
+ get_recorded_entry(&mut pci_root, cdi_seal).map_err(|e| {
+ error!("Failed to get entry from instance.img: {e}");
RebootReason::InternalError
})?;
- }
- (false, entry.salt)
- } else {
- let salt = rand::random_array().map_err(|e| {
- error!("Failed to generated instance.img salt: {e}");
- RebootReason::InternalError
- })?;
- let entry = EntryBody::new(&dice_inputs, &salt);
- record_instance_entry(&entry, cdi_seal, &mut instance_img, header_index).map_err(|e| {
- error!("Failed to get recorded entry in instance.img: {e}");
- RebootReason::InternalError
- })?;
- (true, salt)
+ let (new_instance, salt) = if let Some(entry) = recorded_entry {
+ maybe_check_dice_measurements_match_entry(&dice_inputs, &entry)?;
+ let salt = if cfg!(llpvm_changes) { salt_from_instance_id(fdt)? } else { entry.salt };
+ (false, salt)
+ } else {
+ // New instance!
+ let salt = if cfg!(llpvm_changes) {
+ salt_from_instance_id(fdt)?
+ } else {
+ rand::random_array().map_err(|e| {
+ error!("Failed to generated instance.img salt: {e}");
+ RebootReason::InternalError
+ })?
+ };
+ let entry = EntryBody::new(&dice_inputs, &salt);
+ record_instance_entry(&entry, cdi_seal, &mut instance_img, header_index).map_err(
+ |e| {
+ error!("Failed to get recorded entry in instance.img: {e}");
+ RebootReason::InternalError
+ },
+ )?;
+ (true, salt)
+ };
+ (new_instance, salt)
};
- trace!("Got salt from instance.img: {salt:x?}");
+ trace!("Got salt for instance: {salt:x?}");
let new_bcc_handover = if cfg!(dice_changes) {
Cow::Borrowed(current_bcc_handover)
@@ -241,6 +239,32 @@
Ok(bcc_range)
}
+fn maybe_check_dice_measurements_match_entry(
+ dice_inputs: &PartialInputs,
+ entry: &EntryBody,
+) -> Result<(), RebootReason> {
+ // The RKP VM is allowed to run if it has passed the verified boot check and
+ // contains the expected version in its AVB footer.
+ // The comparison below with the previous boot information is skipped to enable the
+ // simultaneous update of the pvmfw and RKP VM.
+ // For instance, when both the pvmfw and RKP VM are updated, the code hash of the
+ // RKP VM will differ from the one stored in the instance image. In this case, the
+ // RKP VM is still allowed to run.
+ // This ensures that the updated RKP VM will retain the same CDIs in the next stage.
+ if dice_inputs.rkp_vm_marker {
+ return Ok(());
+ }
+ ensure_dice_measurements_match_entry(dice_inputs, entry).map_err(|e| {
+ error!(
+ "Dice measurements do not match recorded entry. \
+ This may be because of update: {e}"
+ );
+ RebootReason::InternalError
+ })?;
+
+ Ok(())
+}
+
fn ensure_dice_measurements_match_entry(
dice_inputs: &PartialInputs,
entry: &EntryBody,
@@ -256,6 +280,56 @@
}
}
+// Get the "salt" which is one of the input for DICE derivation.
+// This provides differentiation of secrets for different VM instances with same payloads.
+fn salt_from_instance_id(fdt: &Fdt) -> Result<Hidden, RebootReason> {
+ let id = instance_id(fdt)?;
+ let salt = Digester::sha512()
+ .digest(&[&b"InstanceId:"[..], id].concat())
+ .map_err(|e| {
+ error!("Failed to get digest of instance-id: {e}");
+ RebootReason::InternalError
+ })?
+ .try_into()
+ .map_err(|_| RebootReason::InternalError)?;
+ Ok(salt)
+}
+
+fn instance_id(fdt: &Fdt) -> Result<&[u8], RebootReason> {
+ let node = avf_untrusted_node(fdt)?;
+ let id = node.getprop(cstr!("instance-id")).map_err(|e| {
+ error!("Failed to get instance-id in DT: {e}");
+ RebootReason::InvalidFdt
+ })?;
+ id.ok_or_else(|| {
+ error!("Missing instance-id");
+ RebootReason::InvalidFdt
+ })
+}
+
+fn should_defer_rollback_protection(fdt: &Fdt) -> Result<bool, RebootReason> {
+ let node = avf_untrusted_node(fdt)?;
+ let defer_rbp = node
+ .getprop(cstr!("defer-rollback-protection"))
+ .map_err(|e| {
+ error!("Failed to get defer-rollback-protection property in DT: {e}");
+ RebootReason::InvalidFdt
+ })?
+ .is_some();
+ Ok(defer_rbp)
+}
+
+fn avf_untrusted_node(fdt: &Fdt) -> Result<FdtNode, RebootReason> {
+ let node = fdt.node(cstr!("/avf/untrusted")).map_err(|e| {
+ error!("Failed to get /avf/untrusted node: {e}");
+ RebootReason::InvalidFdt
+ })?;
+ node.ok_or_else(|| {
+ error!("/avf/untrusted node is missing in DT");
+ RebootReason::InvalidFdt
+ })
+}
+
/// Logs the given PCI error and returns the appropriate `RebootReason`.
fn handle_pci_error(e: PciError) -> RebootReason {
error!("{}", e);
diff --git a/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java b/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java
index 45beb14..4714132 100644
--- a/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java
+++ b/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java
@@ -1040,23 +1040,43 @@
VirtualMachineConfig normalConfig = builder.build();
assertThat(tryBootVmWithConfig(normalConfig, "test_vm").payloadStarted).isTrue();
- // Try to run the VM again with the previous instance.img
+ // Try to run the VM again with the previous instance
// We need to make sure that no changes on config don't invalidate the identity, to compare
// the result with the below "different debug level" test.
+ File vmInstanceBackup = null, vmIdBackup = null;
File vmInstance = getVmFile("test_vm", "instance.img");
- File vmInstanceBackup = File.createTempFile("instance", ".img");
- Files.copy(vmInstance.toPath(), vmInstanceBackup.toPath(), REPLACE_EXISTING);
+ File vmId = getVmFile("test_vm", "instance_id");
+ if (vmInstance.exists()) {
+ vmInstanceBackup = File.createTempFile("instance", ".img");
+ Files.copy(vmInstance.toPath(), vmInstanceBackup.toPath(), REPLACE_EXISTING);
+ }
+ if (vmId.exists()) {
+ vmIdBackup = File.createTempFile("instance_id", "backup");
+ Files.copy(vmId.toPath(), vmIdBackup.toPath(), REPLACE_EXISTING);
+ }
+
forceCreateNewVirtualMachine("test_vm", normalConfig);
- Files.copy(vmInstanceBackup.toPath(), vmInstance.toPath(), REPLACE_EXISTING);
+
+ if (vmInstanceBackup != null) {
+ Files.copy(vmInstanceBackup.toPath(), vmInstance.toPath(), REPLACE_EXISTING);
+ }
+ if (vmIdBackup != null) {
+ Files.copy(vmIdBackup.toPath(), vmId.toPath(), REPLACE_EXISTING);
+ }
assertThat(tryBootVm(TAG, "test_vm").payloadStarted).isTrue();
// Launch the same VM with a different debug level. The Java API prohibits this
// (thankfully).
- // For testing, we do that by creating a new VM with debug level, and copy the old instance
- // image to the new VM instance image.
+ // For testing, we do that by creating a new VM with debug level, and overwriting the old
+ // instance data to the new VM instance data.
VirtualMachineConfig debugConfig = builder.setDebugLevel(toLevel).build();
forceCreateNewVirtualMachine("test_vm", debugConfig);
- Files.copy(vmInstanceBackup.toPath(), vmInstance.toPath(), REPLACE_EXISTING);
+ if (vmInstanceBackup != null) {
+ Files.copy(vmInstanceBackup.toPath(), vmInstance.toPath(), REPLACE_EXISTING);
+ }
+ if (vmIdBackup != null) {
+ Files.copy(vmIdBackup.toPath(), vmId.toPath(), REPLACE_EXISTING);
+ }
assertThat(tryBootVm(TAG, "test_vm").payloadStarted).isFalse();
}