From 65bbb91d4870cceb47a6064296e99410068ea3ab Mon Sep 17 00:00:00 2001 From: Alan Stokes Date: Wed, 23 Nov 2022 09:39:34 +0000 Subject: [PATCH] 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 | 18 +-- .../benchmark/src/native/benchmarkbinary.cpp | 7 +- .../microdroid/test/MicrodroidTests.java | 5 +- tests/testapk/src/native/testbinary.cpp | 37 +----- .../vm_payload_restricted.h | 10 +- vm_payload/include/vm_payload.h | 18 +-- .../src/{vm_payload_service.rs => api.rs} | 122 ++++++++---------- vm_payload/src/lib.rs | 4 +- 8 files changed, 83 insertions(+), 138 deletions(-) rename vm_payload/src/{vm_payload_service.rs => api.rs} (76%) diff --git a/compos/compos_key_helper/compos_key_main.cpp b/compos/compos_key_helper/compos_key_main.cpp index 9417584e..f1637e0d 100644 --- a/compos/compos_key_helper/compos_key_main.cpp +++ b/compos/compos_key_helper/compos_key_main.cpp @@ -38,11 +38,8 @@ constexpr const char* kSigningKeySeedIdentifier = "CompOS signing key seed"; Result 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 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 e43025c9..66b41a19 100644 --- a/tests/benchmark/src/native/benchmarkbinary.cpp +++ b/tests/benchmark/src/native/benchmarkbinary.cpp @@ -160,12 +160,7 @@ private: Result run_io_benchmark_tests() { auto test_service = ndk::SharedRefBase::make(); - 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 17574c74..71a9e3b6 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 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")); } diff --git a/tests/testapk/src/native/testbinary.cpp b/tests/testapk/src/native/testbinary.cpp index 694f452f..5c217ff6 100644 --- a/tests/testapk/src/native/testbinary.cpp +++ b/tests/testapk/src/native/testbinary.cpp @@ -76,40 +76,22 @@ Result start_test_service() { ndk::ScopedAStatus insecurelyExposeVmInstanceSecret(std::vector* 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* 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* 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 start_test_service() { }; auto testService = ndk::SharedRefBase::make(); - 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 8170a645..0b785416 100644 --- a/vm_payload/include-restricted/vm_payload_restricted.h +++ b/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 diff --git a/vm_payload/include/vm_payload.h b/vm_payload/include/vm_payload.h index 48518ff9..0ad4c642 100644 --- a/vm_payload/include/vm_payload.h +++ b/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); /** 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 874b7e19..febc2be1 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 @@ lazy_static! { static ref PAYLOAD_CONNECTION: Mutex>> = 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> { @@ -51,22 +54,32 @@ fn get_vm_payload_service() -> Result> { /// 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(result: Result) -> 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> { - 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> { @@ -202,35 +199,26 @@ fn try_get_dice_attestation_chain() -> Result> { } /// 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> { + 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> { - 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 be6cf939..5c3ee316 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, };