Merge "Panic on non-actionable failures"
diff --git a/compos/compos_key_helper/compos_key_main.cpp b/compos/compos_key_helper/compos_key_main.cpp
index 9417584..f1637e0 100644
--- a/compos/compos_key_helper/compos_key_main.cpp
+++ b/compos/compos_key_helper/compos_key_main.cpp
@@ -38,11 +38,8 @@
Result<Ed25519KeyPair> getSigningKey() {
Seed seed;
- if (!AVmPayload_getVmInstanceSecret(kSigningKeySeedIdentifier,
- strlen(kSigningKeySeedIdentifier), seed.data(),
- seed.size())) {
- return Error() << "Failed to get signing key seed";
- }
+ AVmPayload_getVmInstanceSecret(kSigningKeySeedIdentifier, strlen(kSigningKeySeedIdentifier),
+ seed.data(), seed.size());
return compos_key::keyFromSeed(seed);
}
@@ -60,16 +57,9 @@
}
int write_bcc() {
- size_t bcc_size;
- if (!AVmPayload_getDiceAttestationChain(nullptr, 0, &bcc_size)) {
- LOG(ERROR) << "Failed to measure attestation chain";
- return 1;
- }
+ size_t bcc_size = AVmPayload_getDiceAttestationChain(nullptr, 0);
std::vector<uint8_t> bcc(bcc_size);
- if (!AVmPayload_getDiceAttestationChain(bcc.data(), bcc.size(), &bcc_size)) {
- LOG(ERROR) << "Failed to get attestation chain";
- return 1;
- }
+ AVmPayload_getDiceAttestationChain(bcc.data(), bcc.size());
if (!WriteFully(STDOUT_FILENO, bcc.data(), bcc.size())) {
PLOG(ERROR) << "Write failed";
diff --git a/tests/benchmark/src/native/benchmarkbinary.cpp b/tests/benchmark/src/native/benchmarkbinary.cpp
index e43025c..66b41a1 100644
--- a/tests/benchmark/src/native/benchmarkbinary.cpp
+++ b/tests/benchmark/src/native/benchmarkbinary.cpp
@@ -160,12 +160,7 @@
Result<void> run_io_benchmark_tests() {
auto test_service = ndk::SharedRefBase::make<IOBenchmarkService>();
- auto callback = []([[maybe_unused]] void* param) {
- if (!AVmPayload_notifyPayloadReady()) {
- LOG(ERROR) << "failed to notify payload ready to virtualizationservice";
- abort();
- }
- };
+ auto callback = []([[maybe_unused]] void* param) { AVmPayload_notifyPayloadReady(); };
if (!AVmPayload_runVsockRpcServer(test_service->asBinder().get(), test_service->SERVICE_PORT,
callback, nullptr)) {
return Error() << "RPC Server failed to run";
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 17574c7..71a9e3b 100644
--- a/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java
+++ b/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java
@@ -28,7 +28,6 @@
import android.content.Context;
import android.os.Build;
-import android.os.ServiceSpecificException;
import android.os.SystemProperties;
import android.system.virtualmachine.VirtualMachine;
import android.system.virtualmachine.VirtualMachineCallback;
@@ -354,7 +353,7 @@
listener.runToFinish(TAG, vm);
Exception e = exception.getNow(null);
if (e != null) {
- throw e;
+ throw new RuntimeException(e);
}
return vmCdis;
}
@@ -470,7 +469,7 @@
.build();
mInner.forceCreateNewVirtualMachine("test_vm", config);
- assertThrows(ServiceSpecificException.class, () -> launchVmAndGetCdis("test_vm"));
+ assertThrows(Exception.class, () -> launchVmAndGetCdis("test_vm"));
}
diff --git a/tests/testapk/src/native/testbinary.cpp b/tests/testapk/src/native/testbinary.cpp
index 694f452..5c217ff 100644
--- a/tests/testapk/src/native/testbinary.cpp
+++ b/tests/testapk/src/native/testbinary.cpp
@@ -76,40 +76,22 @@
ndk::ScopedAStatus insecurelyExposeVmInstanceSecret(std::vector<uint8_t>* out) override {
const uint8_t identifier[] = {1, 2, 3, 4};
out->resize(32);
- if (!AVmPayload_getVmInstanceSecret(identifier, sizeof(identifier), out->data(),
- out->size())) {
- return ndk::ScopedAStatus::
- fromServiceSpecificErrorWithMessage(0, "Failed to get VM instance secret");
- }
+ AVmPayload_getVmInstanceSecret(identifier, sizeof(identifier), out->data(),
+ out->size());
return ndk::ScopedAStatus::ok();
}
ndk::ScopedAStatus insecurelyExposeAttestationCdi(std::vector<uint8_t>* out) override {
- size_t cdi_size;
- if (!AVmPayload_getDiceAttestationCdi(nullptr, 0, &cdi_size)) {
- return ndk::ScopedAStatus::
- fromServiceSpecificErrorWithMessage(0, "Failed to measure attestation cdi");
- }
+ size_t cdi_size = AVmPayload_getDiceAttestationCdi(nullptr, 0);
out->resize(cdi_size);
- if (!AVmPayload_getDiceAttestationCdi(out->data(), out->size(), &cdi_size)) {
- return ndk::ScopedAStatus::
- fromServiceSpecificErrorWithMessage(0, "Failed to get attestation cdi");
- }
+ AVmPayload_getDiceAttestationCdi(out->data(), out->size());
return ndk::ScopedAStatus::ok();
}
ndk::ScopedAStatus getBcc(std::vector<uint8_t>* out) override {
- size_t bcc_size;
- if (!AVmPayload_getDiceAttestationChain(nullptr, 0, &bcc_size)) {
- return ndk::ScopedAStatus::
- fromServiceSpecificErrorWithMessage(0,
- "Failed to measure attestation chain");
- }
+ size_t bcc_size = AVmPayload_getDiceAttestationChain(nullptr, 0);
out->resize(bcc_size);
- if (!AVmPayload_getDiceAttestationChain(out->data(), out->size(), &bcc_size)) {
- return ndk::ScopedAStatus::
- fromServiceSpecificErrorWithMessage(0, "Failed to get attestation chain");
- }
+ AVmPayload_getDiceAttestationChain(out->data(), out->size());
return ndk::ScopedAStatus::ok();
}
@@ -136,12 +118,7 @@
};
auto testService = ndk::SharedRefBase::make<TestService>();
- auto callback = []([[maybe_unused]] void* param) {
- if (!AVmPayload_notifyPayloadReady()) {
- std::cerr << "failed to notify payload ready to virtualizationservice" << std::endl;
- abort();
- }
- };
+ auto callback = []([[maybe_unused]] void* param) { AVmPayload_notifyPayloadReady(); };
if (!AVmPayload_runVsockRpcServer(testService->asBinder().get(), testService->SERVICE_PORT,
callback, nullptr)) {
return Error() << "RPC Server failed to run";
diff --git a/vm_payload/include-restricted/vm_payload_restricted.h b/vm_payload/include-restricted/vm_payload_restricted.h
index 8170a64..0b78541 100644
--- a/vm_payload/include-restricted/vm_payload_restricted.h
+++ b/vm_payload/include-restricted/vm_payload_restricted.h
@@ -36,21 +36,19 @@
*
* \param data pointer to size bytes where the chain is written.
* \param size number of bytes that can be written to data.
- * \param total outputs the total size of the chain if the function succeeds
*
- * \return true on success and false on failure.
+ * \return the total size of the chain
*/
-bool AVmPayload_getDiceAttestationChain(void *data, size_t size, size_t *total);
+size_t AVmPayload_getDiceAttestationChain(void *data, size_t size);
/**
* Get the VM's DICE attestation CDI.
*
* \param data pointer to size bytes where the CDI is written.
* \param size number of bytes that can be written to data.
- * \param total outputs the total size of the CDI if the function succeeds
*
- * \return true on success and false on failure.
+ * \return the total size of the CDI
*/
-bool AVmPayload_getDiceAttestationCdi(void *data, size_t size, size_t *total);
+size_t AVmPayload_getDiceAttestationCdi(void *data, size_t size);
__END_DECLS
diff --git a/vm_payload/include/vm_payload.h b/vm_payload/include/vm_payload.h
index 48518ff..0ad4c64 100644
--- a/vm_payload/include/vm_payload.h
+++ b/vm_payload/include/vm_payload.h
@@ -30,9 +30,13 @@
/**
* Notifies the host that the payload is ready.
*
- * \return true if the notification succeeds else false.
+ * If the host app has set a `VirtualMachineCallback` for the VM, its
+ * `onPayloadReady` method will be called.
+ *
+ * Note that subsequent calls to this function after the first have no effect;
+ * `onPayloadReady` is never called more than once.
*/
-bool AVmPayload_notifyPayloadReady(void);
+void AVmPayload_notifyPayloadReady(void);
/**
* Runs a binder RPC server, serving the supplied binder service implementation on the given vsock
@@ -57,17 +61,15 @@
/**
* Get a secret that is uniquely bound to this VM instance. The secrets are
- * values up to 32 bytes long and the value associated with an identifier will
- * not change over the lifetime of the VM instance.
+ * 32-byte values and the value associated with an identifier will not change
+ * over the lifetime of the VM instance.
*
* \param identifier identifier of the secret to return.
* \param identifier_size size of the secret identifier.
* \param secret pointer to size bytes where the secret is written.
- * \param size number of bytes of the secret to get, up to the secret size.
- *
- * \return true on success and false on failure.
+ * \param size number of bytes of the secret to get, <= 32.
*/
-bool AVmPayload_getVmInstanceSecret(const void *identifier, size_t identifier_size, void *secret,
+void AVmPayload_getVmInstanceSecret(const void *identifier, size_t identifier_size, void *secret,
size_t size);
/**
diff --git a/vm_payload/src/vm_payload_service.rs b/vm_payload/src/api.rs
similarity index 76%
rename from vm_payload/src/vm_payload_service.rs
rename to vm_payload/src/api.rs
index 874b7e1..febc2be 100644
--- a/vm_payload/src/vm_payload_service.rs
+++ b/vm_payload/src/api.rs
@@ -16,15 +16,16 @@
use android_system_virtualization_payload::aidl::android::system::virtualization::payload::IVmPayloadService::{
IVmPayloadService, VM_PAYLOAD_SERVICE_SOCKET_NAME, VM_APK_CONTENTS_PATH};
-use anyhow::{Context, Result};
+use anyhow::{ensure, Context, Result};
use binder::{Strong, unstable_api::{AIBinder, new_spibinder}};
use lazy_static::lazy_static;
use log::{error, info, Level};
use rpcbinder::{get_unix_domain_rpc_interface, RpcServer};
use std::ffi::CString;
+use std::fmt::Debug;
use std::os::raw::{c_char, c_void};
use std::ptr;
-use std::sync::Mutex;
+use std::sync::{Mutex, atomic::{AtomicBool, Ordering}};
lazy_static! {
static ref VM_APK_CONTENTS_PATH_C: CString =
@@ -32,6 +33,8 @@
static ref PAYLOAD_CONNECTION: Mutex<Option<Strong<dyn IVmPayloadService>>> = Mutex::default();
}
+static ALREADY_NOTIFIED: AtomicBool = AtomicBool::new(false);
+
/// Return a connection to the payload service in Microdroid Manager. Uses the existing connection
/// if there is one, otherwise attempts to create a new one.
fn get_vm_payload_service() -> Result<Strong<dyn IVmPayloadService>> {
@@ -51,22 +54,32 @@
/// Make sure our logging goes to logcat. It is harmless to call this more than once.
fn initialize_logging() {
android_logger::init_once(
- android_logger::Config::default().with_tag("vm_payload").with_min_level(Level::Debug),
+ android_logger::Config::default().with_tag("vm_payload").with_min_level(Level::Info),
);
}
+/// In many cases clients can't do anything useful if API calls fail, and the failure
+/// generally indicates that the VM is exiting or otherwise doomed. So rather than
+/// returning a non-actionable error indication we just log the problem and abort
+/// the process.
+fn unwrap_or_abort<T, E: Debug>(result: Result<T, E>) -> T {
+ result.unwrap_or_else(|e| {
+ let msg = format!("{:?}", e);
+ error!("{msg}");
+ panic!("{msg}")
+ })
+}
+
/// Notifies the host that the payload is ready.
-/// Returns true if the notification succeeds else false.
+/// Panics on failure.
#[no_mangle]
-pub extern "C" fn AVmPayload_notifyPayloadReady() -> bool {
+pub extern "C" fn AVmPayload_notifyPayloadReady() {
initialize_logging();
- if let Err(e) = try_notify_payload_ready() {
- error!("{:?}", e);
- false
- } else {
+ if !ALREADY_NOTIFIED.swap(true, Ordering::Relaxed) {
+ unwrap_or_abort(try_notify_payload_ready());
+
info!("Notified host payload ready successfully");
- true
}
}
@@ -125,6 +138,7 @@
}
/// Get a secret that is uniquely bound to this VM instance.
+/// Panics on failure.
///
/// # Safety
///
@@ -133,68 +147,51 @@
/// * `identifier` must be [valid] for reads of `identifier_size` bytes.
/// * `secret` must be [valid] for writes of `size` bytes.
///
-/// [valid]: std::ptr#safety
+/// [valid]: ptr#safety
#[no_mangle]
pub unsafe extern "C" fn AVmPayload_getVmInstanceSecret(
identifier: *const u8,
identifier_size: usize,
secret: *mut u8,
size: usize,
-) -> bool {
+) {
initialize_logging();
let identifier = std::slice::from_raw_parts(identifier, identifier_size);
- match try_get_vm_instance_secret(identifier, size) {
- Err(e) => {
- error!("{:?}", e);
- false
- }
- Ok(vm_secret) => {
- if vm_secret.len() != size {
- return false;
- }
- std::ptr::copy_nonoverlapping(vm_secret.as_ptr(), secret, size);
- true
- }
- }
+ let vm_secret = unwrap_or_abort(try_get_vm_instance_secret(identifier, size));
+ ptr::copy_nonoverlapping(vm_secret.as_ptr(), secret, size);
}
fn try_get_vm_instance_secret(identifier: &[u8], size: usize) -> Result<Vec<u8>> {
- get_vm_payload_service()?
+ let vm_secret = get_vm_payload_service()?
.getVmInstanceSecret(identifier, i32::try_from(size)?)
- .context("Cannot get VM instance secret")
+ .context("Cannot get VM instance secret")?;
+ ensure!(
+ vm_secret.len() == size,
+ "Returned secret has {} bytes, expected {}",
+ vm_secret.len(),
+ size
+ );
+ Ok(vm_secret)
}
/// Get the VM's attestation chain.
-/// Returns true on success, else false.
+/// Panics on failure.
///
/// # Safety
///
/// Behavior is undefined if any of the following conditions are violated:
///
/// * `data` must be [valid] for writes of `size` bytes.
-/// * `total` must be [valid] for writes.
///
-/// [valid]: std::ptr#safety
+/// [valid]: ptr#safety
#[no_mangle]
-pub unsafe extern "C" fn AVmPayload_getDiceAttestationChain(
- data: *mut u8,
- size: usize,
- total: *mut usize,
-) -> bool {
+pub unsafe extern "C" fn AVmPayload_getDiceAttestationChain(data: *mut u8, size: usize) -> usize {
initialize_logging();
- match try_get_dice_attestation_chain() {
- Err(e) => {
- error!("{:?}", e);
- false
- }
- Ok(chain) => {
- total.write(chain.len());
- std::ptr::copy_nonoverlapping(chain.as_ptr(), data, std::cmp::min(chain.len(), size));
- true
- }
- }
+ let chain = unwrap_or_abort(try_get_dice_attestation_chain());
+ ptr::copy_nonoverlapping(chain.as_ptr(), data, std::cmp::min(chain.len(), size));
+ chain.len()
}
fn try_get_dice_attestation_chain() -> Result<Vec<u8>> {
@@ -202,35 +199,26 @@
}
/// Get the VM's attestation CDI.
-/// Returns true on success, else false.
+/// Panics on failure.
///
/// # Safety
///
/// Behavior is undefined if any of the following conditions are violated:
///
/// * `data` must be [valid] for writes of `size` bytes.
-/// * `total` must be [valid] for writes.
///
-/// [valid]: std::ptr#safety
+/// [valid]: ptr#safety
#[no_mangle]
-pub unsafe extern "C" fn AVmPayload_getDiceAttestationCdi(
- data: *mut u8,
- size: usize,
- total: *mut usize,
-) -> bool {
+pub unsafe extern "C" fn AVmPayload_getDiceAttestationCdi(data: *mut u8, size: usize) -> usize {
initialize_logging();
- match try_get_dice_attestation_cdi() {
- Err(e) => {
- error!("{:?}", e);
- false
- }
- Ok(cdi) => {
- total.write(cdi.len());
- std::ptr::copy_nonoverlapping(cdi.as_ptr(), data, std::cmp::min(cdi.len(), size));
- true
- }
- }
+ let cdi = unwrap_or_abort(try_get_dice_attestation_cdi());
+ ptr::copy_nonoverlapping(cdi.as_ptr(), data, std::cmp::min(cdi.len(), size));
+ cdi.len()
+}
+
+fn try_get_dice_attestation_cdi() -> Result<Vec<u8>> {
+ get_vm_payload_service()?.getDiceAttestationCdi().context("Cannot get attestation CDI")
}
/// Gets the path to the APK contents.
@@ -245,7 +233,3 @@
// TODO(b/254454578): Return a real path if storage is present
ptr::null()
}
-
-fn try_get_dice_attestation_cdi() -> Result<Vec<u8>> {
- get_vm_payload_service()?.getDiceAttestationCdi().context("Cannot get attestation CDI")
-}
diff --git a/vm_payload/src/lib.rs b/vm_payload/src/lib.rs
index be6cf93..5c3ee31 100644
--- a/vm_payload/src/lib.rs
+++ b/vm_payload/src/lib.rs
@@ -14,9 +14,9 @@
//! Library for payload to communicate with the Microdroid Manager.
-mod vm_payload_service;
+mod api;
-pub use vm_payload_service::{
+pub use api::{
AVmPayload_getDiceAttestationCdi, AVmPayload_getDiceAttestationChain,
AVmPayload_getVmInstanceSecret, AVmPayload_notifyPayloadReady,
};