Keystore 2.0: Untangle mutex dependencies in add_auth_token.
Some mutexes on the add_auth_token path were more dependent on one
another than necessary. This could lead to a chain were add_auth_token
would block on waiting for a time stamp token, which in turn could stall
the execution for seconds.
Also fix some comments.
Bug: 183676395
Test: N/A
Change-Id: I5c6ae1e47fe232ea9954497108f807bbcd37fef7
diff --git a/keystore2/src/authorization.rs b/keystore2/src/authorization.rs
index 06b5598..87ed795 100644
--- a/keystore2/src/authorization.rs
+++ b/keystore2/src/authorization.rs
@@ -118,7 +118,7 @@
}
fn add_auth_token(&self, auth_token: &HardwareAuthToken) -> Result<()> {
- //check keystore permission
+ // Check keystore permission.
check_keystore_permission(KeystorePerm::add_auth()).context("In add_auth_token.")?;
ENFORCEMENTS.add_auth_token(auth_token.clone())?;
@@ -133,8 +133,8 @@
) -> Result<()> {
match (lock_screen_event, password) {
(LockScreenEvent::UNLOCK, Some(password)) => {
- //This corresponds to the unlock() method in legacy keystore API.
- //check permission
+ // This corresponds to the unlock() method in legacy keystore API.
+ // check permission
check_keystore_permission(KeystorePerm::unlock())
.context("In on_lock_screen_event: Unlock with password.")?;
ENFORCEMENTS.set_device_locked(user_id, false);
@@ -201,7 +201,7 @@
check_keystore_permission(KeystorePerm::get_auth_token())
.context("In get_auth_tokens_for_credstore.")?;
- // if the challenge is zero, return error
+ // If the challenge is zero, return error
if challenge == 0 {
return Err(Error::Rc(ResponseCode::INVALID_ARGUMENT))
.context("In get_auth_tokens_for_credstore. Challenge can not be zero.");
diff --git a/keystore2/src/enforcements.rs b/keystore2/src/enforcements.rs
index 7993c88..378b72f 100644
--- a/keystore2/src/enforcements.rs
+++ b/keystore2/src/enforcements.rs
@@ -61,47 +61,54 @@
state: AuthRequestState,
/// This need to be set to Some to fulfill a AuthRequestState::OpAuth or
/// AuthRequestState::TimeStampedOpAuth.
- hat: Option<HardwareAuthToken>,
+ hat: Mutex<Option<HardwareAuthToken>>,
}
+unsafe impl Sync for AuthRequest {}
+
impl AuthRequest {
- fn op_auth() -> Arc<Mutex<Self>> {
- Arc::new(Mutex::new(Self { state: AuthRequestState::OpAuth, hat: None }))
+ fn op_auth() -> Arc<Self> {
+ Arc::new(Self { state: AuthRequestState::OpAuth, hat: Mutex::new(None) })
}
- fn timestamped_op_auth(receiver: Receiver<Result<TimeStampToken, Error>>) -> Arc<Mutex<Self>> {
- Arc::new(Mutex::new(Self {
+ fn timestamped_op_auth(receiver: Receiver<Result<TimeStampToken, Error>>) -> Arc<Self> {
+ Arc::new(Self {
state: AuthRequestState::TimeStampedOpAuth(receiver),
- hat: None,
- }))
+ hat: Mutex::new(None),
+ })
}
fn timestamp(
hat: HardwareAuthToken,
receiver: Receiver<Result<TimeStampToken, Error>>,
- ) -> Arc<Mutex<Self>> {
- Arc::new(Mutex::new(Self { state: AuthRequestState::TimeStamp(receiver), hat: Some(hat) }))
+ ) -> Arc<Self> {
+ Arc::new(Self { state: AuthRequestState::TimeStamp(receiver), hat: Mutex::new(Some(hat)) })
}
- fn add_auth_token(&mut self, hat: HardwareAuthToken) {
- self.hat = Some(hat)
+ fn add_auth_token(&self, hat: HardwareAuthToken) {
+ *self.hat.lock().unwrap() = Some(hat)
}
- fn get_auth_tokens(&mut self) -> Result<(HardwareAuthToken, Option<TimeStampToken>)> {
- match (&self.state, self.hat.is_some()) {
- (AuthRequestState::OpAuth, true) => Ok((self.hat.take().unwrap(), None)),
- (AuthRequestState::TimeStampedOpAuth(recv), true)
- | (AuthRequestState::TimeStamp(recv), true) => {
+ fn get_auth_tokens(&self) -> Result<(HardwareAuthToken, Option<TimeStampToken>)> {
+ let hat = self
+ .hat
+ .lock()
+ .unwrap()
+ .take()
+ .ok_or(Error::Km(ErrorCode::KEY_USER_NOT_AUTHENTICATED))
+ .context("In get_auth_tokens: No operation auth token received.")?;
+
+ let tst = match &self.state {
+ AuthRequestState::TimeStampedOpAuth(recv) | AuthRequestState::TimeStamp(recv) => {
let result = recv.recv().context("In get_auth_tokens: Sender disconnected.")?;
- let tst = result.context(concat!(
+ Some(result.context(concat!(
"In get_auth_tokens: Worker responded with error ",
"from generating timestamp token."
- ))?;
- Ok((self.hat.take().unwrap(), Some(tst)))
+ ))?)
}
- (_, false) => Err(Error::Km(ErrorCode::KEY_USER_NOT_AUTHENTICATED))
- .context("In get_auth_tokens: No operation auth token received."),
- }
+ AuthRequestState::OpAuth => None,
+ };
+ Ok((hat, tst))
}
}
@@ -127,7 +134,7 @@
/// We block on timestamp tokens, because we can always make progress on these requests.
/// The per-op auth tokens might never come, which means we fail if the client calls
/// update or finish before we got a per-op auth token.
- Waiting(Arc<Mutex<AuthRequest>>),
+ Waiting(Arc<AuthRequest>),
/// In this state we have gotten all of the required tokens, we just cache them to
/// be used when the operation progresses.
Token(HardwareAuthToken, Option<TimeStampToken>),
@@ -169,9 +176,15 @@
const CLEANUP_PERIOD: u8 = 25;
pub fn add_auth_token(&self, hat: HardwareAuthToken) {
- let mut map = self.map_and_cleanup_counter.lock().unwrap();
- let (ref mut map, _) = *map;
- if let Some((_, recv)) = map.remove_entry(&hat.challenge) {
+ let recv = {
+ // Limit the scope of the mutex guard, so that it is not held while the auth token is
+ // added.
+ let mut map = self.map_and_cleanup_counter.lock().unwrap();
+ let (ref mut map, _) = *map;
+ map.remove_entry(&hat.challenge)
+ };
+
+ if let Some((_, recv)) = recv {
recv.add_auth_token(hat);
}
}
@@ -191,7 +204,7 @@
}
#[derive(Debug)]
-struct TokenReceiver(Weak<Mutex<AuthRequest>>);
+struct TokenReceiver(Weak<AuthRequest>);
impl TokenReceiver {
fn is_obsolete(&self) -> bool {
@@ -200,8 +213,7 @@
fn add_auth_token(&self, hat: HardwareAuthToken) {
if let Some(state_arc) = self.0.upgrade() {
- let mut state = state_arc.lock().unwrap();
- state.add_auth_token(hat);
+ state_arc.add_auth_token(hat);
}
}
}
@@ -326,8 +338,7 @@
/// tokens into the DeferredAuthState::Token state for future use.
fn get_auth_tokens(&mut self) -> Result<(Option<HardwareAuthToken>, Option<TimeStampToken>)> {
let deferred_tokens = if let DeferredAuthState::Waiting(ref auth_request) = self.state {
- let mut state = auth_request.lock().unwrap();
- Some(state.get_auth_tokens().context("In AuthInfo::get_auth_tokens.")?)
+ Some(auth_request.get_auth_tokens().context("In AuthInfo::get_auth_tokens.")?)
} else {
None
};
diff --git a/keystore2/src/legacy_migrator.rs b/keystore2/src/legacy_migrator.rs
index e5bcae4..5e0d573 100644
--- a/keystore2/src/legacy_migrator.rs
+++ b/keystore2/src/legacy_migrator.rs
@@ -420,7 +420,7 @@
.context("In list_uid: Trying to list legacy entries.")
}
- /// This is a key migration request that can run in the migrator thread. This should
+ /// This is a key migration request that must run in the migrator thread. This must
/// be passed to do_serialized.
fn check_and_migrate(&mut self, uid: u32, mut key: KeyDescriptor) -> Result<()> {
let alias = key.alias.clone().ok_or_else(|| {