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

compos/compos_key_helper/compos_key_main.cpp

@@ -38,11 +38,8 @@ constexpr const char* kSigningKeySeedIdentifier = "CompOS signing key seed";
 
 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_public_key() {
 }
 
 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";

tests/benchmark/src/native/benchmarkbinary.cpp

@@ -160,12 +160,7 @@ private:
 
 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";

tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java

@@ -28,7 +28,6 @@ import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
 
 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 @@ public class MicrodroidTests extends MicrodroidDeviceTestBase {
         listener.runToFinish(TAG, vm);
         Exception e = exception.getNow(null);
         if (e != null) {
-            throw e;
+            throw new RuntimeException(e);
         }
         return vmCdis;
     }
@@ -470,7 +469,7 @@ public class MicrodroidTests extends MicrodroidDeviceTestBase {
                 .build();
         mInner.forceCreateNewVirtualMachine("test_vm", config);
 
-        assertThrows(ServiceSpecificException.class, () -> launchVmAndGetCdis("test_vm"));
+        assertThrows(Exception.class, () -> launchVmAndGetCdis("test_vm"));
     }
 
 

tests/testapk/src/native/testbinary.cpp

@@ -76,40 +76,22 @@ Result<void> start_test_service() {
         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 @@ Result<void> start_test_service() {
     };
     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";

vm_payload/include-restricted/vm_payload_restricted.h

@@ -36,21 +36,19 @@ __BEGIN_DECLS
  *
  * \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

vm_payload/include/vm_payload.h

@@ -30,9 +30,13 @@ typedef struct AIBinder AIBinder;
 /**
  * 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 @@ bool AVmPayload_runVsockRpcServer(AIBinder *service, unsigned int port,
 
 /**
  * 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);
 
 /**

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 @@ lazy_static! {
     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 @@ fn get_vm_payload_service() -> Result<Strong<dyn IVmPayloadService>> {
 /// 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 @@ pub unsafe extern "C" fn AVmPayload_runVsockRpcServer(
 }
 
 /// Get a secret that is uniquely bound to this VM instance.
+/// Panics on failure.
 ///
 /// # Safety
 ///
@@ -133,68 +147,51 @@ pub unsafe extern "C" fn AVmPayload_runVsockRpcServer(
 /// * `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 @@ fn try_get_dice_attestation_chain() -> Result<Vec<u8>> {
 }
 
 /// 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 @@ pub extern "C" fn AVmPayload_getEncryptedStoragePath() -> *const c_char {
     // 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")
-}

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,
 };