Skip to content

Commit

Permalink
Remove UI status caching and reset
Browse files Browse the repository at this point in the history
Trying to keep track of the UI status before an operation and resetting
it afterwards does not work well for us if these operations overlap.
Therefore we remove that feature for the time being.  This could
probably be solved by using counters to keep track of the number of
ongoing operations of a certain type.
  • Loading branch information
robin-nitrokey committed May 24, 2022
1 parent 269e71a commit 0994607
Showing 1 changed file with 1 addition and 4 deletions.
5 changes: 1 addition & 4 deletions src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,6 @@ impl<P: Platform> ServiceResources<P> {
let starttime = self.platform.user_interface().uptime();
let timeout = core::time::Duration::from_millis(request.timeout_milliseconds as u64);

let previous_status = self.platform.user_interface().status();
self.platform.user_interface().set_status(ui::Status::WaitingForUserPresence);
loop {
self.platform.user_interface().refresh();
Expand Down Expand Up @@ -516,7 +515,7 @@ impl<P: Platform> ServiceResources<P> {
}
}
}
self.platform.user_interface().set_status(previous_status);
self.platform.user_interface().set_status(ui::Status::Idle);

let result = Ok(());
Ok(Reply::RequestUserConsent(reply::RequestUserConsent { result } ))
Expand Down Expand Up @@ -725,13 +724,11 @@ impl<P: Platform> Service<P> {

for ep in eps.iter_mut() {
if let Some(request) = ep.interchange.take_request() {
resources.platform.user_interface().set_status(ui::Status::Processing);
// #[cfg(test)] println!("service got request: {:?}", &request);

// resources.currently_serving = ep.client_id.clone();
let reply_result = resources.reply_to(ep.client_id.clone(), &request);

resources.platform.user_interface().set_status(ui::Status::Idle);
ep.interchange.respond(&reply_result).ok();

}
Expand Down

3 comments on commit 0994607

@szszszsz
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate, on which cases this happens? I would expect that device will just return busy status and deny the request, while processing one already.
AFAIK this was not designed to handle concurrent operations anyway (not to mention limited stack memory).

@robin-nitrokey
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not look into the details, but I encountered two scenarios:

  • process operations during user confirmation requests on webauthn.bin.coffee
  • nested user confirmation requests

I would expect that device will just return busy status and deny the request, while processing one already.

I think in this context, “returning busy” would also be considered “processing”. The alternative would be to just ignore the request and we don’t want to do that. This does not mean that we execute multiple FIDO2 operations in parallel.

@robin-nitrokey
Copy link
Member Author

@robin-nitrokey robin-nitrokey commented on 0994607 May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had another look at the logs and the repeated operation is a RequestUserConsent request, so this is internal to Trussed and not related to external requests.

Please sign in to comment.