From db7ddde2e4819ea51c454790536b64f690cbef34 Mon Sep 17 00:00:00 2001 From: David Drysdale Date: Fri, 7 Jun 2024 16:22:49 +0100 Subject: [PATCH 1/3] Shift to idiomatic use of `map_err` Test: keystore2_test Test: legacykeystore_test Flag: none, pure refactoring Change-Id: I4b9f1b0d47145846764ff46676b10035f7f2fb6a --- keystore2/legacykeystore/lib.rs | 41 +++++++-------- keystore2/src/apc.rs | 44 +++++++--------- keystore2/src/authorization.rs | 85 ++++++++++++++----------------- keystore2/src/error.rs | 90 ++++++++++++++++----------------- keystore2/src/km_compat.rs | 8 +-- keystore2/src/maintenance.rs | 24 +++++---- keystore2/src/metrics.rs | 4 +- keystore2/src/operation.rs | 47 ++++++++--------- keystore2/src/security_level.rs | 14 ++--- keystore2/src/service.rs | 20 ++++---- 10 files changed, 178 insertions(+), 199 deletions(-) diff --git a/keystore2/legacykeystore/lib.rs b/keystore2/legacykeystore/lib.rs index 41b86850..8e6040b8 100644 --- a/keystore2/legacykeystore/lib.rs +++ b/keystore2/legacykeystore/lib.rs @@ -210,25 +210,22 @@ impl Error { } } -/// Translate errors into service specific exceptions. +/// Translate an error into a service specific exception, logging along the way. /// -/// All errors are logged, except for ERROR_ENTRY_NOT_FOUND error. `Error::Error(x)` variants get -/// mapped onto a service specific error code of `x`, other errors are mapped to -/// `ERROR_SYSTEM_ERROR`. -fn map_or_log_err(result: Result) -> BinderResult { - result.map_err(|e| { - let root_cause = e.root_cause(); - let (rc, log_error) = match root_cause.downcast_ref::() { - // Make the entry not found errors silent. - Some(Error::Error(ERROR_ENTRY_NOT_FOUND)) => (ERROR_ENTRY_NOT_FOUND, false), - Some(Error::Error(e)) => (*e, true), - Some(Error::Binder(_, _)) | None => (ERROR_SYSTEM_ERROR, true), - }; - if log_error { - log::error!("{:?}", e); - } - BinderStatus::new_service_specific_error(rc, anyhow_error_to_cstring(&e).as_deref()) - }) +/// `Error::Error(x)` variants get mapped onto a service specific error code of `x`, other errors +/// are mapped to `ERROR_SYSTEM_ERROR`. +fn into_logged_binder(e: anyhow::Error) -> BinderStatus { + let root_cause = e.root_cause(); + let (rc, log_error) = match root_cause.downcast_ref::() { + // Make the entry not found errors silent. + Some(Error::Error(ERROR_ENTRY_NOT_FOUND)) => (ERROR_ENTRY_NOT_FOUND, false), + Some(Error::Error(e)) => (*e, true), + Some(Error::Binder(_, _)) | None => (ERROR_SYSTEM_ERROR, true), + }; + if log_error { + log::error!("{:?}", e); + } + BinderStatus::new_service_specific_error(rc, anyhow_error_to_cstring(&e).as_deref()) } fn ensure_keystore_put_is_enabled() -> Result<()> { @@ -536,19 +533,19 @@ impl binder::Interface for LegacyKeystoreService {} impl ILegacyKeystore for LegacyKeystoreService { fn get(&self, alias: &str, uid: i32) -> BinderResult> { let _wp = wd::watch("ILegacyKeystore::get"); - map_or_log_err(self.legacy_keystore.get(alias, uid)) + self.legacy_keystore.get(alias, uid).map_err(into_logged_binder) } fn put(&self, alias: &str, uid: i32, entry: &[u8]) -> BinderResult<()> { let _wp = wd::watch("ILegacyKeystore::put"); - map_or_log_err(self.legacy_keystore.put(alias, uid, entry)) + self.legacy_keystore.put(alias, uid, entry).map_err(into_logged_binder) } fn remove(&self, alias: &str, uid: i32) -> BinderResult<()> { let _wp = wd::watch("ILegacyKeystore::remove"); - map_or_log_err(self.legacy_keystore.remove(alias, uid)) + self.legacy_keystore.remove(alias, uid).map_err(into_logged_binder) } fn list(&self, prefix: &str, uid: i32) -> BinderResult> { let _wp = wd::watch("ILegacyKeystore::list"); - map_or_log_err(self.legacy_keystore.list(prefix, uid)) + self.legacy_keystore.list(prefix, uid).map_err(into_logged_binder) } } diff --git a/keystore2/src/apc.rs b/keystore2/src/apc.rs index 8fda1f25..fc36a0c1 100644 --- a/keystore2/src/apc.rs +++ b/keystore2/src/apc.rs @@ -73,29 +73,24 @@ impl Error { } } -/// This function should be used by confirmation service calls to translate error conditions -/// into service specific exceptions. -/// -/// All error conditions get logged by this function. +/// Translate an error into a service-specific exception, logging along the way. /// /// `Error::Rc(x)` variants get mapped onto a service specific error code of `x`. /// `selinux::Error::perm()` is mapped on `ResponseCode::PERMISSION_DENIED`. /// /// All non `Error` error conditions get mapped onto ResponseCode::SYSTEM_ERROR`. -pub fn map_or_log_err(result: Result) -> BinderResult { - result.map_err(|e| { - log::error!("{:#?}", e); - let root_cause = e.root_cause(); - let rc = match root_cause.downcast_ref::() { - Some(Error::Rc(rcode)) => rcode.0, - Some(Error::Binder(_, _)) => ResponseCode::SYSTEM_ERROR.0, - None => match root_cause.downcast_ref::() { - Some(selinux::Error::PermissionDenied) => ResponseCode::PERMISSION_DENIED.0, - _ => ResponseCode::SYSTEM_ERROR.0, - }, - }; - BinderStatus::new_service_specific_error(rc, anyhow_error_to_cstring(&e).as_deref()) - }) +pub fn into_logged_binder(e: anyhow::Error) -> BinderStatus { + log::error!("{:#?}", e); + let root_cause = e.root_cause(); + let rc = match root_cause.downcast_ref::() { + Some(Error::Rc(rcode)) => rcode.0, + Some(Error::Binder(_, _)) => ResponseCode::SYSTEM_ERROR.0, + None => match root_cause.downcast_ref::() { + Some(selinux::Error::PermissionDenied) => ResponseCode::PERMISSION_DENIED.0, + _ => ResponseCode::SYSTEM_ERROR.0, + }, + }; + BinderStatus::new_service_specific_error(rc, anyhow_error_to_cstring(&e).as_deref()) } /// Rate info records how many failed attempts a client has made to display a protected @@ -341,23 +336,18 @@ impl IProtectedConfirmation for ApcManager { ) -> BinderResult<()> { // presentPrompt can take more time than other operations. let _wp = wd::watch_millis("IProtectedConfirmation::presentPrompt", 3000); - map_or_log_err(self.present_prompt( - listener, - prompt_text, - extra_data, - locale, - ui_option_flags, - )) + self.present_prompt(listener, prompt_text, extra_data, locale, ui_option_flags) + .map_err(into_logged_binder) } fn cancelPrompt( &self, listener: &binder::Strong, ) -> BinderResult<()> { let _wp = wd::watch("IProtectedConfirmation::cancelPrompt"); - map_or_log_err(self.cancel_prompt(listener)) + self.cancel_prompt(listener).map_err(into_logged_binder) } fn isSupported(&self) -> BinderResult { let _wp = wd::watch("IProtectedConfirmation::isSupported"); - map_or_log_err(Self::is_supported()) + Self::is_supported().map_err(into_logged_binder) } } diff --git a/keystore2/src/authorization.rs b/keystore2/src/authorization.rs index 4636a665..5a3fdbcb 100644 --- a/keystore2/src/authorization.rs +++ b/keystore2/src/authorization.rs @@ -51,10 +51,7 @@ pub enum Error { Binder(ExceptionCode, i32), } -/// This function should be used by authorization service calls to translate error conditions -/// into service specific exceptions. -/// -/// All error conditions get logged by this function. +/// Translate an error into a service specific exception, logging along the way. /// /// `Error::Rc(x)` variants get mapped onto a service specific error code of `x`. /// Certain response codes may be returned from keystore/ResponseCode.aidl by the keystore2 modules, @@ -64,38 +61,36 @@ pub enum Error { /// `selinux::Error::perm()` is mapped on `ResponseCode::PERMISSION_DENIED`. /// /// All non `Error` error conditions get mapped onto ResponseCode::SYSTEM_ERROR`. -pub fn map_or_log_err(result: Result) -> BinderResult { - result.map_err(|e| { - log::error!("{:#?}", e); - let root_cause = e.root_cause(); - if let Some(KeystoreError::Rc(ks_rcode)) = root_cause.downcast_ref::() { - let rc = match *ks_rcode { - // Although currently keystore2/ResponseCode.aidl and - // authorization/ResponseCode.aidl share the same integer values for the - // common response codes, this may deviate in the future, hence the - // conversion here. - KsResponseCode::SYSTEM_ERROR => ResponseCode::SYSTEM_ERROR.0, - KsResponseCode::KEY_NOT_FOUND => ResponseCode::KEY_NOT_FOUND.0, - KsResponseCode::VALUE_CORRUPTED => ResponseCode::VALUE_CORRUPTED.0, - KsResponseCode::INVALID_ARGUMENT => ResponseCode::INVALID_ARGUMENT.0, - // If the code paths of IKeystoreAuthorization aidl's methods happen to return - // other error codes from KsResponseCode in the future, they should be converted - // as well. +pub fn into_logged_binder(e: anyhow::Error) -> BinderStatus { + log::error!("{:#?}", e); + let root_cause = e.root_cause(); + if let Some(KeystoreError::Rc(ks_rcode)) = root_cause.downcast_ref::() { + let rc = match *ks_rcode { + // Although currently keystore2/ResponseCode.aidl and + // authorization/ResponseCode.aidl share the same integer values for the + // common response codes, this may deviate in the future, hence the + // conversion here. + KsResponseCode::SYSTEM_ERROR => ResponseCode::SYSTEM_ERROR.0, + KsResponseCode::KEY_NOT_FOUND => ResponseCode::KEY_NOT_FOUND.0, + KsResponseCode::VALUE_CORRUPTED => ResponseCode::VALUE_CORRUPTED.0, + KsResponseCode::INVALID_ARGUMENT => ResponseCode::INVALID_ARGUMENT.0, + // If the code paths of IKeystoreAuthorization aidl's methods happen to return + // other error codes from KsResponseCode in the future, they should be converted + // as well. + _ => ResponseCode::SYSTEM_ERROR.0, + }; + BinderStatus::new_service_specific_error(rc, anyhow_error_to_cstring(&e).as_deref()) + } else { + let rc = match root_cause.downcast_ref::() { + Some(Error::Rc(rcode)) => rcode.0, + Some(Error::Binder(_, _)) => ResponseCode::SYSTEM_ERROR.0, + None => match root_cause.downcast_ref::() { + Some(selinux::Error::PermissionDenied) => ResponseCode::PERMISSION_DENIED.0, _ => ResponseCode::SYSTEM_ERROR.0, - }; - BinderStatus::new_service_specific_error(rc, anyhow_error_to_cstring(&e).as_deref()) - } else { - let rc = match root_cause.downcast_ref::() { - Some(Error::Rc(rcode)) => rcode.0, - Some(Error::Binder(_, _)) => ResponseCode::SYSTEM_ERROR.0, - None => match root_cause.downcast_ref::() { - Some(selinux::Error::PermissionDenied) => ResponseCode::PERMISSION_DENIED.0, - _ => ResponseCode::SYSTEM_ERROR.0, - }, - }; - BinderStatus::new_service_specific_error(rc, anyhow_error_to_cstring(&e).as_deref()) - } - }) + }, + }; + BinderStatus::new_service_specific_error(rc, anyhow_error_to_cstring(&e).as_deref()) + } } /// This struct is defined to implement the aforementioned AIDL interface. @@ -257,12 +252,12 @@ impl Interface for AuthorizationManager {} impl IKeystoreAuthorization for AuthorizationManager { fn addAuthToken(&self, auth_token: &HardwareAuthToken) -> BinderResult<()> { let _wp = wd::watch("IKeystoreAuthorization::addAuthToken"); - map_or_log_err(self.add_auth_token(auth_token)) + self.add_auth_token(auth_token).map_err(into_logged_binder) } fn onDeviceUnlocked(&self, user_id: i32, password: Option<&[u8]>) -> BinderResult<()> { let _wp = wd::watch("IKeystoreAuthorization::onDeviceUnlocked"); - map_or_log_err(self.on_device_unlocked(user_id, password.map(|pw| pw.into()))) + self.on_device_unlocked(user_id, password.map(|pw| pw.into())).map_err(into_logged_binder) } fn onDeviceLocked( @@ -272,17 +267,18 @@ impl IKeystoreAuthorization for AuthorizationManager { weak_unlock_enabled: bool, ) -> BinderResult<()> { let _wp = wd::watch("IKeystoreAuthorization::onDeviceLocked"); - map_or_log_err(self.on_device_locked(user_id, unlocking_sids, weak_unlock_enabled)) + self.on_device_locked(user_id, unlocking_sids, weak_unlock_enabled) + .map_err(into_logged_binder) } fn onWeakUnlockMethodsExpired(&self, user_id: i32) -> BinderResult<()> { let _wp = wd::watch("IKeystoreAuthorization::onWeakUnlockMethodsExpired"); - map_or_log_err(self.on_weak_unlock_methods_expired(user_id)) + self.on_weak_unlock_methods_expired(user_id).map_err(into_logged_binder) } fn onNonLskfUnlockMethodsExpired(&self, user_id: i32) -> BinderResult<()> { let _wp = wd::watch("IKeystoreAuthorization::onNonLskfUnlockMethodsExpired"); - map_or_log_err(self.on_non_lskf_unlock_methods_expired(user_id)) + self.on_non_lskf_unlock_methods_expired(user_id).map_err(into_logged_binder) } fn getAuthTokensForCredStore( @@ -292,11 +288,8 @@ impl IKeystoreAuthorization for AuthorizationManager { auth_token_max_age_millis: i64, ) -> binder::Result { let _wp = wd::watch("IKeystoreAuthorization::getAuthTokensForCredStore"); - map_or_log_err(self.get_auth_tokens_for_credstore( - challenge, - secure_user_id, - auth_token_max_age_millis, - )) + self.get_auth_tokens_for_credstore(challenge, secure_user_id, auth_token_max_age_millis) + .map_err(into_logged_binder) } fn getLastAuthTime( @@ -305,7 +298,7 @@ impl IKeystoreAuthorization for AuthorizationManager { auth_types: &[HardwareAuthenticatorType], ) -> binder::Result { if aconfig_android_hardware_biometrics_rust::last_authentication_time() { - map_or_log_err(self.get_last_auth_time(secure_user_id, auth_types)) + self.get_last_auth_time(secure_user_id, auth_types).map_err(into_logged_binder) } else { Err(BinderStatus::new_service_specific_error( ResponseCode::PERMISSION_DENIED.0, diff --git a/keystore2/src/error.rs b/keystore2/src/error.rs index b9a22916..cea4d6be 100644 --- a/keystore2/src/error.rs +++ b/keystore2/src/error.rs @@ -21,8 +21,8 @@ //! //! `SerializedError` is used send error codes on the wire. //! -//! `map_or_log_err` is a convenience method used to convert `anyhow::Error` into `SerializedError` -//! wire type. +//! `into_[logged_]binder` is a convenience method used to convert `anyhow::Error` into +//! `SerializedError` wire type. //! //! Keystore functions should use `anyhow::Result` to return error conditions, and context should //! be added every time an error is forwarded. @@ -128,14 +128,11 @@ pub fn map_km_error(r: BinderResult) -> Result { // Non negative error codes cannot be KM error codes. // So we create an `Error::Binder` variant to preserve // the service specific error code for logging. - // `map_or_log_err` will map this on a system error, - // but not before logging the details to logcat. Error::Binder(ExceptionCode::SERVICE_SPECIFIC, se) } } // We create `Error::Binder` to preserve the exception code // for logging. - // `map_or_log_err` will map this on a system error. e_code => Error::Binder(e_code, 0), } }) @@ -163,21 +160,17 @@ pub fn map_binder_status_code(r: Result) -> Result { r.map_err(Error::BinderTransaction) } -/// This function should be used by Keystore service calls to translate error conditions -/// into service specific exceptions. -/// -/// All error conditions get logged by this function, except for KEY_NOT_FOUND error. -pub fn map_or_log_err(result: anyhow::Result) -> BinderResult { - map_err_with(result, |e| { - // Make the key not found errors silent. - if !matches!( - e.root_cause().downcast_ref::(), - Some(Error::Rc(ResponseCode::KEY_NOT_FOUND)) - ) { - log::error!("{:?}", e); - } - e - }) +/// Convert an [`anyhow::Error`] to a [`binder::Status`], logging the value +/// along the way (except if it is `KEY_NOT_FOUND`). +pub fn into_logged_binder(e: anyhow::Error) -> BinderStatus { + // Log everything except key not found. + if !matches!( + e.root_cause().downcast_ref::(), + Some(Error::Rc(ResponseCode::KEY_NOT_FOUND)) + ) { + log::error!("{:?}", e); + } + into_binder(e) } /// This function turns an anyhow error into an optional CString. @@ -194,18 +187,10 @@ pub fn anyhow_error_to_cstring(e: &anyhow::Error) -> Option { } } -/// This function behaves similar to map_or_log_error, but it does not log the errors, instead -/// it calls map_err on the error before mapping it to a binder result allowing callers to -/// log or transform the error before mapping it. -pub fn map_err_with(result: anyhow::Result, map_err: F1) -> BinderResult -where - F1: FnOnce(anyhow::Error) -> anyhow::Error, -{ - result.map_err(|e| { - let e = map_err(e); - let rc = anyhow_error_to_serialized_error(&e); - BinderStatus::new_service_specific_error(rc.0, anyhow_error_to_cstring(&e).as_deref()) - }) +/// Convert an [`anyhow::Error`] to a [`binder::Status`]. +pub fn into_binder(e: anyhow::Error) -> binder::Status { + let rc = anyhow_error_to_serialized_error(&e); + BinderStatus::new_service_specific_error(rc.0, anyhow_error_to_cstring(&e).as_deref()) } /// This type is used to send error codes on the wire. @@ -323,7 +308,9 @@ pub mod tests { for rc in ResponseCode::LOCKED.0..ResponseCode::BACKEND_BUSY.0 { assert_eq!( Result::<(), i32>::Err(rc), - map_or_log_err(nested_rc(ResponseCode(rc))).map_err(|s| s.service_specific_error()) + nested_rc(ResponseCode(rc)) + .map_err(into_logged_binder) + .map_err(|s| s.service_specific_error()) ); } @@ -332,7 +319,9 @@ pub mod tests { for ec in ErrorCode::UNKNOWN_ERROR.0..ErrorCode::ROOT_OF_TRUST_ALREADY_SET.0 { assert_eq!( Result::<(), i32>::Err(ec), - map_or_log_err(nested_ec(ErrorCode(ec))).map_err(|s| s.service_specific_error()) + nested_ec(ErrorCode(ec)) + .map_err(into_logged_binder) + .map_err(|s| s.service_specific_error()) ); } @@ -341,11 +330,10 @@ pub mod tests { for ec in ErrorCode::UNKNOWN_ERROR.0..ErrorCode::ROOT_OF_TRUST_ALREADY_SET.0 { assert_eq!( Result::<(), i32>::Err(ec), - map_or_log_err( - map_km_error(binder_sse_error(ec)) - .with_context(|| format!("Km error code: {}.", ec)) - ) - .map_err(|s| s.service_specific_error()) + map_km_error(binder_sse_error(ec)) + .with_context(|| format!("Km error code: {}.", ec)) + .map_err(into_logged_binder) + .map_err(|s| s.service_specific_error()) ); } @@ -354,42 +342,50 @@ pub mod tests { // service specific error. let sse = map_km_error(binder_sse_error(1)); assert_eq!(Err(Error::Binder(ExceptionCode::SERVICE_SPECIFIC, 1)), sse); - // map_or_log_err then maps it on a service specific error of ResponseCode::SYSTEM_ERROR. + // into_binder then maps it on a service specific error of ResponseCode::SYSTEM_ERROR. assert_eq!( Result::<(), ResponseCode>::Err(ResponseCode::SYSTEM_ERROR), - map_or_log_err(sse.context("Non negative service specific error.")) + sse.context("Non negative service specific error.") + .map_err(into_logged_binder) .map_err(|s| ResponseCode(s.service_specific_error())) ); // map_km_error creates a Error::Binder variant storing the given exception code. let binder_exception = map_km_error(binder_exception(ExceptionCode::TRANSACTION_FAILED)); assert_eq!(Err(Error::Binder(ExceptionCode::TRANSACTION_FAILED, 0)), binder_exception); - // map_or_log_err then maps it on a service specific error of ResponseCode::SYSTEM_ERROR. + // into_binder then maps it on a service specific error of ResponseCode::SYSTEM_ERROR. assert_eq!( Result::<(), ResponseCode>::Err(ResponseCode::SYSTEM_ERROR), - map_or_log_err(binder_exception.context("Binder Exception.")) + binder_exception + .context("Binder Exception.") + .map_err(into_logged_binder) .map_err(|s| ResponseCode(s.service_specific_error())) ); // selinux::Error::Perm() needs to be mapped to ResponseCode::PERMISSION_DENIED assert_eq!( Result::<(), ResponseCode>::Err(ResponseCode::PERMISSION_DENIED), - map_or_log_err(nested_selinux_perm()) + nested_selinux_perm() + .map_err(into_logged_binder) .map_err(|s| ResponseCode(s.service_specific_error())) ); // All other errors get mapped on System Error. assert_eq!( Result::<(), ResponseCode>::Err(ResponseCode::SYSTEM_ERROR), - map_or_log_err(nested_other_error()) + nested_other_error() + .map_err(into_logged_binder) .map_err(|s| ResponseCode(s.service_specific_error())) ); // Result::Ok variants get passed to the ok handler. - assert_eq!(Ok(ResponseCode::LOCKED), map_or_log_err(nested_ok(ResponseCode::LOCKED))); + assert_eq!( + Ok(ResponseCode::LOCKED), + nested_ok(ResponseCode::LOCKED).map_err(into_logged_binder) + ); assert_eq!( Ok(ResponseCode::SYSTEM_ERROR), - map_or_log_err(nested_ok(ResponseCode::SYSTEM_ERROR)) + nested_ok(ResponseCode::SYSTEM_ERROR).map_err(into_logged_binder) ); Ok(()) diff --git a/keystore2/src/km_compat.rs b/keystore2/src/km_compat.rs index 02bcb1aa..5e3bdfa7 100644 --- a/keystore2/src/km_compat.rs +++ b/keystore2/src/km_compat.rs @@ -16,7 +16,7 @@ //! be emulated on back-level devices. use crate::ks_err; -use crate::error::{map_binder_status, map_binder_status_code, map_or_log_err, Error, ErrorCode}; +use crate::error::{map_binder_status, map_binder_status_code, into_logged_binder, Error, ErrorCode}; use android_hardware_security_keymint::binder::{BinderFeatures, StatusCode, Strong}; use android_hardware_security_secureclock::aidl::android::hardware::security::secureclock::TimeStampToken::TimeStampToken; use android_hardware_security_keymint::aidl::android::hardware::security::keymint::{ @@ -226,7 +226,7 @@ where ) -> binder::Result { if self.emu.emulation_required(key_params, &KeyImportData::None) { let mut result = self.soft.generateKey(key_params, attestation_key)?; - result.keyBlob = map_or_log_err(wrap_keyblob(&result.keyBlob))?; + result.keyBlob = wrap_keyblob(&result.keyBlob).map_err(into_logged_binder)?; Ok(result) } else { self.real.generateKey(key_params, attestation_key) @@ -242,7 +242,7 @@ where if self.emu.emulation_required(key_params, &KeyImportData::new(key_format, key_data)?) { let mut result = self.soft.importKey(key_params, key_format, key_data, attestation_key)?; - result.keyBlob = map_or_log_err(wrap_keyblob(&result.keyBlob))?; + result.keyBlob = wrap_keyblob(&result.keyBlob).map_err(into_logged_binder)?; Ok(result) } else { self.real.importKey(key_params, key_format, key_data, attestation_key) @@ -281,7 +281,7 @@ where KeyBlob::Wrapped(keyblob) => { // Re-wrap the upgraded keyblob. let upgraded_keyblob = self.soft.upgradeKey(keyblob, upgrade_params)?; - map_or_log_err(wrap_keyblob(&upgraded_keyblob)) + wrap_keyblob(&upgraded_keyblob).map_err(into_logged_binder) } } } diff --git a/keystore2/src/maintenance.rs b/keystore2/src/maintenance.rs index c384a037..ba92399b 100644 --- a/keystore2/src/maintenance.rs +++ b/keystore2/src/maintenance.rs @@ -15,8 +15,8 @@ //! This module implements IKeystoreMaintenance AIDL interface. use crate::database::{KeyEntryLoadBits, KeyType}; +use crate::error::into_logged_binder; use crate::error::map_km_error; -use crate::error::map_or_log_err; use crate::error::Error; use crate::globals::get_keymint_device; use crate::globals::{DB, LEGACY_IMPORTER, SUPER_KEY}; @@ -303,13 +303,14 @@ impl IKeystoreMaintenance for Maintenance { password.is_some() ); let _wp = wd::watch("IKeystoreMaintenance::onUserPasswordChanged"); - map_or_log_err(Self::on_user_password_changed(user_id, password.map(|pw| pw.into()))) + Self::on_user_password_changed(user_id, password.map(|pw| pw.into())) + .map_err(into_logged_binder) } fn onUserAdded(&self, user_id: i32) -> BinderResult<()> { log::info!("onUserAdded(user={user_id})"); let _wp = wd::watch("IKeystoreMaintenance::onUserAdded"); - map_or_log_err(self.add_or_remove_user(user_id)) + self.add_or_remove_user(user_id).map_err(into_logged_binder) } fn initUserSuperKeys( @@ -320,31 +321,32 @@ impl IKeystoreMaintenance for Maintenance { ) -> BinderResult<()> { log::info!("initUserSuperKeys(user={user_id}, allow_existing={allow_existing})"); let _wp = wd::watch("IKeystoreMaintenance::initUserSuperKeys"); - map_or_log_err(self.init_user_super_keys(user_id, password.into(), allow_existing)) + self.init_user_super_keys(user_id, password.into(), allow_existing) + .map_err(into_logged_binder) } fn onUserRemoved(&self, user_id: i32) -> BinderResult<()> { log::info!("onUserRemoved(user={user_id})"); let _wp = wd::watch("IKeystoreMaintenance::onUserRemoved"); - map_or_log_err(self.add_or_remove_user(user_id)) + self.add_or_remove_user(user_id).map_err(into_logged_binder) } fn onUserLskfRemoved(&self, user_id: i32) -> BinderResult<()> { log::info!("onUserLskfRemoved(user={user_id})"); let _wp = wd::watch("IKeystoreMaintenance::onUserLskfRemoved"); - map_or_log_err(Self::on_user_lskf_removed(user_id)) + Self::on_user_lskf_removed(user_id).map_err(into_logged_binder) } fn clearNamespace(&self, domain: Domain, nspace: i64) -> BinderResult<()> { log::info!("clearNamespace({domain:?}, nspace={nspace})"); let _wp = wd::watch("IKeystoreMaintenance::clearNamespace"); - map_or_log_err(self.clear_namespace(domain, nspace)) + self.clear_namespace(domain, nspace).map_err(into_logged_binder) } fn earlyBootEnded(&self) -> BinderResult<()> { log::info!("earlyBootEnded()"); let _wp = wd::watch("IKeystoreMaintenance::earlyBootEnded"); - map_or_log_err(Self::early_boot_ended()) + Self::early_boot_ended().map_err(into_logged_binder) } fn migrateKeyNamespace( @@ -354,13 +356,13 @@ impl IKeystoreMaintenance for Maintenance { ) -> BinderResult<()> { log::info!("migrateKeyNamespace(src={source:?}, dest={destination:?})"); let _wp = wd::watch("IKeystoreMaintenance::migrateKeyNamespace"); - map_or_log_err(Self::migrate_key_namespace(source, destination)) + Self::migrate_key_namespace(source, destination).map_err(into_logged_binder) } fn deleteAllKeys(&self) -> BinderResult<()> { log::warn!("deleteAllKeys()"); let _wp = wd::watch("IKeystoreMaintenance::deleteAllKeys"); - map_or_log_err(Self::delete_all_keys()) + Self::delete_all_keys().map_err(into_logged_binder) } fn getAppUidsAffectedBySid( @@ -370,6 +372,6 @@ impl IKeystoreMaintenance for Maintenance { ) -> BinderResult> { log::info!("getAppUidsAffectedBySid(secure_user_id={secure_user_id:?})"); let _wp = wd::watch("IKeystoreMaintenance::getAppUidsAffectedBySid"); - map_or_log_err(Self::get_app_uids_affected_by_sid(user_id, secure_user_id)) + Self::get_app_uids_affected_by_sid(user_id, secure_user_id).map_err(into_logged_binder) } } diff --git a/keystore2/src/metrics.rs b/keystore2/src/metrics.rs index bbe12aa2..47577393 100644 --- a/keystore2/src/metrics.rs +++ b/keystore2/src/metrics.rs @@ -14,7 +14,7 @@ //! This module implements the IKeystoreMetrics AIDL interface, which exposes the API method for the //! proxy in the system server to pull the aggregated metrics in keystore. -use crate::error::map_or_log_err; +use crate::error::into_logged_binder; use crate::ks_err; use crate::metrics_store::METRICS_STORE; use crate::permission::KeystorePerm; @@ -52,6 +52,6 @@ impl Interface for Metrics {} impl IKeystoreMetrics for Metrics { fn pullMetrics(&self, atom_id: AtomID) -> BinderResult> { let _wp = wd::watch("IKeystoreMetrics::pullMetrics"); - map_or_log_err(self.pull_metrics(atom_id)) + self.pull_metrics(atom_id).map_err(into_logged_binder) } } diff --git a/keystore2/src/operation.rs b/keystore2/src/operation.rs index 94bd7c33..7d988e1a 100644 --- a/keystore2/src/operation.rs +++ b/keystore2/src/operation.rs @@ -127,7 +127,7 @@ use crate::enforcements::AuthInfo; use crate::error::{ - error_to_serialized_error, map_err_with, map_km_error, map_or_log_err, Error, ErrorCode, + error_to_serialized_error, into_binder, into_logged_binder, map_km_error, Error, ErrorCode, ResponseCode, SerializedError, }; use crate::ks_err; @@ -822,18 +822,20 @@ impl binder::Interface for KeystoreOperation {} impl IKeystoreOperation for KeystoreOperation { fn updateAad(&self, aad_input: &[u8]) -> binder::Result<()> { let _wp = wd::watch("IKeystoreOperation::updateAad"); - map_or_log_err(self.with_locked_operation( + self.with_locked_operation( |op| op.update_aad(aad_input).context(ks_err!("KeystoreOperation::updateAad")), false, - )) + ) + .map_err(into_logged_binder) } fn update(&self, input: &[u8]) -> binder::Result>> { let _wp = wd::watch("IKeystoreOperation::update"); - map_or_log_err(self.with_locked_operation( + self.with_locked_operation( |op| op.update(input).context(ks_err!("KeystoreOperation::update")), false, - )) + ) + .map_err(into_logged_binder) } fn finish( &self, @@ -841,29 +843,28 @@ impl IKeystoreOperation for KeystoreOperation { signature: Option<&[u8]>, ) -> binder::Result>> { let _wp = wd::watch("IKeystoreOperation::finish"); - map_or_log_err(self.with_locked_operation( + self.with_locked_operation( |op| op.finish(input, signature).context(ks_err!("KeystoreOperation::finish")), true, - )) + ) + .map_err(into_logged_binder) } fn abort(&self) -> binder::Result<()> { let _wp = wd::watch("IKeystoreOperation::abort"); - map_err_with( - self.with_locked_operation( - |op| op.abort(Outcome::Abort).context(ks_err!("KeystoreOperation::abort")), - true, - ), - |e| { - match e.root_cause().downcast_ref::() { - // Calling abort on expired operations is something very common. - // There is no reason to clutter the log with it. It is never the cause - // for a true problem. - Some(Error::Km(ErrorCode::INVALID_OPERATION_HANDLE)) => {} - _ => log::error!("{:?}", e), - }; - e - }, - ) + let result = self.with_locked_operation( + |op| op.abort(Outcome::Abort).context(ks_err!("KeystoreOperation::abort")), + true, + ); + result.map_err(|e| { + match e.root_cause().downcast_ref::() { + // Calling abort on expired operations is something very common. + // There is no reason to clutter the log with it. It is never the cause + // for a true problem. + Some(Error::Km(ErrorCode::INVALID_OPERATION_HANDLE)) => {} + _ => log::error!("{:?}", e), + }; + into_binder(e) + }) } } diff --git a/keystore2/src/security_level.rs b/keystore2/src/security_level.rs index 71d6dba6..00e0480a 100644 --- a/keystore2/src/security_level.rs +++ b/keystore2/src/security_level.rs @@ -20,7 +20,7 @@ use crate::audit_log::{ }; use crate::database::{BlobInfo, CertificateInfo, KeyIdGuard}; use crate::error::{ - self, map_km_error, map_or_log_err, wrapped_rkpd_error_to_ks_error, Error, ErrorCode, + self, into_logged_binder, map_km_error, wrapped_rkpd_error_to_ks_error, Error, ErrorCode, }; use crate::globals::{ get_remotely_provisioned_component_name, DB, ENFORCEMENTS, LEGACY_IMPORTER, SUPER_KEY, @@ -996,7 +996,7 @@ impl IKeystoreSecurityLevel for KeystoreSecurityLevel { forced: bool, ) -> binder::Result { let _wp = self.watch("IKeystoreSecurityLevel::createOperation"); - map_or_log_err(self.create_operation(key, operation_parameters, forced)) + self.create_operation(key, operation_parameters, forced).map_err(into_logged_binder) } fn generateKey( &self, @@ -1012,7 +1012,7 @@ impl IKeystoreSecurityLevel for KeystoreSecurityLevel { let result = self.generate_key(key, attestation_key, params, flags, entropy); log_key_creation_event_stats(self.security_level, params, &result); log_key_generated(key, ThreadState::get_calling_uid(), result.is_ok()); - map_or_log_err(result) + result.map_err(into_logged_binder) } fn importKey( &self, @@ -1026,7 +1026,7 @@ impl IKeystoreSecurityLevel for KeystoreSecurityLevel { let result = self.import_key(key, attestation_key, params, flags, key_data); log_key_creation_event_stats(self.security_level, params, &result); log_key_imported(key, ThreadState::get_calling_uid(), result.is_ok()); - map_or_log_err(result) + result.map_err(into_logged_binder) } fn importWrappedKey( &self, @@ -1041,20 +1041,20 @@ impl IKeystoreSecurityLevel for KeystoreSecurityLevel { self.import_wrapped_key(key, wrapping_key, masking_key, params, authenticators); log_key_creation_event_stats(self.security_level, params, &result); log_key_imported(key, ThreadState::get_calling_uid(), result.is_ok()); - map_or_log_err(result) + result.map_err(into_logged_binder) } fn convertStorageKeyToEphemeral( &self, storage_key: &KeyDescriptor, ) -> binder::Result { let _wp = self.watch("IKeystoreSecurityLevel::convertStorageKeyToEphemeral"); - map_or_log_err(self.convert_storage_key_to_ephemeral(storage_key)) + self.convert_storage_key_to_ephemeral(storage_key).map_err(into_logged_binder) } fn deleteKey(&self, key: &KeyDescriptor) -> binder::Result<()> { let _wp = self.watch("IKeystoreSecurityLevel::deleteKey"); let result = self.delete_key(key); log_key_deleted(key, ThreadState::get_calling_uid(), result.is_ok()); - map_or_log_err(result) + result.map_err(into_logged_binder) } } diff --git a/keystore2/src/service.rs b/keystore2/src/service.rs index 2ca9ccf8..37263580 100644 --- a/keystore2/src/service.rs +++ b/keystore2/src/service.rs @@ -35,7 +35,7 @@ use crate::{ error::ResponseCode, }; use crate::{ - error::{self, map_or_log_err, ErrorCode}, + error::{self, into_logged_binder, ErrorCode}, id_rotation::IdRotationState, }; use android_hardware_security_keymint::aidl::android::hardware::security::keymint::SecurityLevel::SecurityLevel; @@ -384,11 +384,11 @@ impl IKeystoreService for KeystoreService { let _wp = wd::watch_millis_with("IKeystoreService::getSecurityLevel", 500, move || { format!("security_level: {}", security_level.0) }); - map_or_log_err(self.get_security_level(security_level)) + self.get_security_level(security_level).map_err(into_logged_binder) } fn getKeyEntry(&self, key: &KeyDescriptor) -> binder::Result { let _wp = wd::watch("IKeystoreService::get_key_entry"); - map_or_log_err(self.get_key_entry(key)) + self.get_key_entry(key).map_err(into_logged_binder) } fn updateSubcomponent( &self, @@ -397,17 +397,17 @@ impl IKeystoreService for KeystoreService { certificate_chain: Option<&[u8]>, ) -> binder::Result<()> { let _wp = wd::watch("IKeystoreService::updateSubcomponent"); - map_or_log_err(self.update_subcomponent(key, public_cert, certificate_chain)) + self.update_subcomponent(key, public_cert, certificate_chain).map_err(into_logged_binder) } fn listEntries(&self, domain: Domain, namespace: i64) -> binder::Result> { let _wp = wd::watch("IKeystoreService::listEntries"); - map_or_log_err(self.list_entries(domain, namespace)) + self.list_entries(domain, namespace).map_err(into_logged_binder) } fn deleteKey(&self, key: &KeyDescriptor) -> binder::Result<()> { let _wp = wd::watch("IKeystoreService::deleteKey"); let result = self.delete_key(key); log_key_deleted(key, ThreadState::get_calling_uid(), result.is_ok()); - map_or_log_err(result) + result.map_err(into_logged_binder) } fn grant( &self, @@ -416,11 +416,11 @@ impl IKeystoreService for KeystoreService { access_vector: i32, ) -> binder::Result { let _wp = wd::watch("IKeystoreService::grant"); - map_or_log_err(self.grant(key, grantee_uid, access_vector.into())) + self.grant(key, grantee_uid, access_vector.into()).map_err(into_logged_binder) } fn ungrant(&self, key: &KeyDescriptor, grantee_uid: i32) -> binder::Result<()> { let _wp = wd::watch("IKeystoreService::ungrant"); - map_or_log_err(self.ungrant(key, grantee_uid)) + self.ungrant(key, grantee_uid).map_err(into_logged_binder) } fn listEntriesBatched( &self, @@ -429,11 +429,11 @@ impl IKeystoreService for KeystoreService { start_past_alias: Option<&str>, ) -> binder::Result> { let _wp = wd::watch("IKeystoreService::listEntriesBatched"); - map_or_log_err(self.list_entries_batched(domain, namespace, start_past_alias)) + self.list_entries_batched(domain, namespace, start_past_alias).map_err(into_logged_binder) } fn getNumberOfEntries(&self, domain: Domain, namespace: i64) -> binder::Result { let _wp = wd::watch("IKeystoreService::getNumberOfEntries"); - map_or_log_err(self.count_num_entries(domain, namespace)) + self.count_num_entries(domain, namespace).map_err(into_logged_binder) } } From 04590324c74f7f8e129689ade49762addd934b1c Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Tue, 11 Jun 2024 15:29:51 +0000 Subject: [PATCH 2/3] libc++fs is part of libc++ now. Change-Id: I49d8f35e1e6b9c149a55ef3033d0af60309d3a35 --- fsverity_init/Android.bp | 1 - 1 file changed, 1 deletion(-) diff --git a/fsverity_init/Android.bp b/fsverity_init/Android.bp index 55884937..212aac4a 100644 --- a/fsverity_init/Android.bp +++ b/fsverity_init/Android.bp @@ -14,7 +14,6 @@ cc_binary { ], static_libs: [ "aconfig_fsverity_init_c_lib", - "libc++fs", "libmini_keyctl_static", ], shared_libs: [ From 3e559628013736e0117c8c5c514fb4a02fbd1740 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Tue, 11 Jun 2024 19:33:33 +0000 Subject: [PATCH 3/3] libc++fs is part of libc++ now. Change-Id: I82f3d84eed71bf9e9fe25c70ff0b4c59fa431ad7 --- ondevice-signing/Android.bp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/ondevice-signing/Android.bp b/ondevice-signing/Android.bp index 6901b174..130babcd 100644 --- a/ondevice-signing/Android.bp +++ b/ondevice-signing/Android.bp @@ -85,10 +85,6 @@ cc_library { "VerityUtils.cpp", ], - static_libs: [ - "libc++fs", - ], - shared_libs: [ "libbase", "libcrypto", @@ -128,7 +124,6 @@ cc_binary { header_libs: ["odrefresh_headers"], static_libs: [ - "libc++fs", "libsigningutils", "lib_odsign_proto", ],