Panic on non-actionable failures
This is based on Michael's comments on aosp/2280849. For methods which
should never fail unless the VM is already dying, and for which
clients cannot take any meaningful action, panic instead of returning
false. Make sure we log the cause first.
Update client code to match. Update doc comments in the header file.
Also clarify that calling notify read more than once is harmless
(otherwise it would panic).
Incidentally, rename vs_payload_service.rs because it was confusing me
(we have a file of the same name in microdroid manager which actually
implements the service.)
Changes to AVmPayload_runVsockRpcServer will come later.
Bug: 243512108
Test: atest MicrodroidTests
Test: composd_cmd --test-compile
Change-Id: Ie6f6203ba54246cac669f4a68e8ab76f0a5792ae
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,
};