From 4a2217f025b38a63cf264c1c1d0fdb30f754e7a3 Mon Sep 17 00:00:00 2001 From: Liam Murphy Date: Thu, 4 Aug 2022 22:54:22 +1000 Subject: [PATCH] Interleave sending and receiving of packets to reduce rx latency (#515) * Interleave sending and receiving of packets to reduce latency in receiving of packets * Add patch to CtapUsbSyscallDriver * Minor tweaks from review * Log when overwritting an existing reply * Only log when 'debug_ctap' is enabled * Make ctap mod public, as per review * Rename send_or_recv to send_and_maybe_recv * fix typo * Don't process packets on other transport while doing keepalive * Don't process packets on other transport while doing keepalive * More accurately determine if reply has finished * Move comment closer to appropriate location * Add tests for canceling keepalive packets * Added a TODO for kaczmarczyck re ctap module being public * remove the unnecessary sleep()s * undo messed up commit * address pylint warnings * Fix merge mess up, and patch fido2 Usage Page * Fix up completely borked merge * Remove patch to FIDO usage, after #523. * remove obsolete aspects to diff Co-authored-by: kaczmarczyck <43844792+kaczmarczyck@users.noreply.github.com> --- .../13-send-packet-before-receiving.patch | 32 ++++ src/api/connection.rs | 5 +- src/ctap/hid/send.rs | 17 ++ src/ctap/mod.rs | 21 ++- src/env/test/mod.rs | 2 +- src/env/tock/mod.rs | 8 +- src/lib.rs | 5 +- src/main.rs | 154 ++++++++++++++---- tools/vendor_hid_test.py | 24 +++ 9 files changed, 222 insertions(+), 46 deletions(-) create mode 100644 patches/tock/13-send-packet-before-receiving.patch diff --git a/patches/tock/13-send-packet-before-receiving.patch b/patches/tock/13-send-packet-before-receiving.patch new file mode 100644 index 0000000..d629b61 --- /dev/null +++ b/patches/tock/13-send-packet-before-receiving.patch @@ -0,0 +1,32 @@ +diff --git a/capsules/src/usb/usb_ctap.rs b/capsules/src/usb/usb_ctap.rs +index 2c91c0968..ea8111069 100644 +--- a/capsules/src/usb/usb_ctap.rs ++++ b/capsules/src/usb/usb_ctap.rs +@@ -253,18 +253,19 @@ impl<'a, 'b, C: hil::usb::UsbController<'a>> Driver for CtapUsbSyscallDriver<'a, + if app.waiting { + ReturnCode::EALREADY + } else { ++ // Indicates to the driver that we have a packet to send. ++ let r = self ++ .usb_client ++ .transmit_packet(app.buffer.as_ref().unwrap().as_ref(), endpoint); ++ if r != ReturnCode::SUCCESS { ++ return r; ++ } + // Indicates to the driver that we can receive any pending packet. + app.waiting = true; + self.usb_client.receive_packet(app); + +- if !app.waiting { +- // The call to receive_packet() collected a pending packet. +- ReturnCode::SUCCESS +- } else { +- // Indicates to the driver that we have a packet to send. +- self.usb_client +- .transmit_packet(app.buffer.as_ref().unwrap().as_ref(), endpoint) +- } ++ ReturnCode::SUCCESS ++ + } + } else { + ReturnCode::EINVAL diff --git a/src/api/connection.rs b/src/api/connection.rs index d06ab9c..3970b04 100644 --- a/src/api/connection.rs +++ b/src/api/connection.rs @@ -14,11 +14,12 @@ use crate::clock::ClockInt; use embedded_time::duration::Milliseconds; +use libtock_drivers::usb_ctap_hid::UsbEndpoint; pub enum SendOrRecvStatus { Timeout, Sent, - Received, + Received(UsbEndpoint), } pub struct SendOrRecvError; @@ -26,7 +27,7 @@ pub struct SendOrRecvError; pub type SendOrRecvResult = Result; pub trait HidConnection { - fn send_or_recv_with_timeout( + fn send_and_maybe_recv( &mut self, buf: &mut [u8; 64], timeout: Milliseconds, diff --git a/src/ctap/hid/send.rs b/src/ctap/hid/send.rs index 844d042..afe49ec 100644 --- a/src/ctap/hid/send.rs +++ b/src/ctap/hid/send.rs @@ -32,6 +32,14 @@ impl HidPacketIterator { pub fn none() -> HidPacketIterator { HidPacketIterator(None) } + + pub fn has_data(&self) -> bool { + if let Some(ms) = &self.0 { + ms.finished() + } else { + false + } + } } impl Iterator for HidPacketIterator { @@ -94,6 +102,15 @@ impl MessageSplitter { dst_len } } + + // Is there more data to iterate over? + fn finished(&self) -> bool { + let payload_len = self.message.payload.len(); + match self.seq { + None => true, + Some(_) => self.i < payload_len, + } + } } impl Iterator for MessageSplitter { diff --git a/src/ctap/mod.rs b/src/ctap/mod.rs index f46c2be..dbad9b8 100644 --- a/src/ctap/mod.rs +++ b/src/ctap/mod.rs @@ -83,6 +83,7 @@ use crypto::hmac::hmac_256; use crypto::sha256::Sha256; use crypto::{ecdsa, Hash256}; use embedded_time::duration::Milliseconds; +use libtock_drivers::usb_ctap_hid::UsbEndpoint; use rng256::Rng256; use sk_cbor as cbor; use sk_cbor::cbor_map_options; @@ -274,7 +275,7 @@ fn send_keepalive_up_needed( let keepalive_msg = CtapHid::keepalive(cid, KeepaliveStatus::UpNeeded); for mut pkt in keepalive_msg { let ctap_hid_connection = transport.hid_connection(env); - match ctap_hid_connection.send_or_recv_with_timeout(&mut pkt, timeout) { + match ctap_hid_connection.send_and_maybe_recv(&mut pkt, timeout) { Ok(SendOrRecvStatus::Timeout) => { debug_ctap!(env, "Sending a KEEPALIVE packet timed out"); // TODO: abort user presence test? @@ -283,7 +284,23 @@ fn send_keepalive_up_needed( Ok(SendOrRecvStatus::Sent) => { debug_ctap!(env, "Sent KEEPALIVE packet"); } - Ok(SendOrRecvStatus::Received) => { + Ok(SendOrRecvStatus::Received(endpoint)) => { + let rx_transport = match endpoint { + UsbEndpoint::MainHid => Transport::MainHid, + #[cfg(feature = "vendor_hid")] + UsbEndpoint::VendorHid => Transport::VendorHid, + }; + if rx_transport != transport { + debug_ctap!( + env, + "Received a packet on transport {:?} while sending a KEEPALIVE packet on transport {:?}", + rx_transport, transport + ); + // Ignore this packet. + // TODO(liamjm): Support receiving packets on both interfaces. + continue; + } + // We only parse one packet, because we only care about CANCEL. let (received_cid, processed_packet) = CtapHid::process_single_packet(&pkt); if received_cid != &cid { diff --git a/src/env/test/mod.rs b/src/env/test/mod.rs index e5bac84..163b5e6 100644 --- a/src/env/test/mod.rs +++ b/src/env/test/mod.rs @@ -85,7 +85,7 @@ fn new_storage() -> BufferStorage { } impl HidConnection for TestEnv { - fn send_or_recv_with_timeout( + fn send_and_maybe_recv( &mut self, _buf: &mut [u8; 64], _timeout: Milliseconds, diff --git a/src/env/tock/mod.rs b/src/env/tock/mod.rs index 891883d..ca05027 100644 --- a/src/env/tock/mod.rs +++ b/src/env/tock/mod.rs @@ -42,7 +42,7 @@ pub struct TockHidConnection { } impl HidConnection for TockHidConnection { - fn send_or_recv_with_timeout( + fn send_and_maybe_recv( &mut self, buf: &mut [u8; 64], timeout: Milliseconds, @@ -54,10 +54,8 @@ impl HidConnection for TockHidConnection { ) { Ok(usb_ctap_hid::SendOrRecvStatus::Timeout) => Ok(SendOrRecvStatus::Timeout), Ok(usb_ctap_hid::SendOrRecvStatus::Sent) => Ok(SendOrRecvStatus::Sent), - Ok(usb_ctap_hid::SendOrRecvStatus::Received(recv_endpoint)) - if self.endpoint == recv_endpoint => - { - Ok(SendOrRecvStatus::Received) + Ok(usb_ctap_hid::SendOrRecvStatus::Received(recv_endpoint)) => { + Ok(SendOrRecvStatus::Received(recv_endpoint)) } _ => Err(SendOrRecvError), } diff --git a/src/lib.rs b/src/lib.rs index f189922..3690b57 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -46,11 +46,8 @@ macro_rules! debug_ctap { pub mod api; pub mod clock; -// Implementation details must be public for testing (in particular fuzzing). -#[cfg(feature = "std")] +// TODO(kaczmarczyck): Refactor this so that ctap module isn't public. pub mod ctap; -#[cfg(not(feature = "std"))] -mod ctap; pub mod env; #[cfg(feature = "std")] pub mod test_helpers; diff --git a/src/main.rs b/src/main.rs index 7388a1a..6688627 100644 --- a/src/main.rs +++ b/src/main.rs @@ -32,6 +32,7 @@ use ctap2::api::connection::{HidConnection, SendOrRecvStatus}; #[cfg(feature = "debug_ctap")] use ctap2::clock::CtapClock; use ctap2::clock::{new_clock, Clock, ClockInt, KEEPALIVE_DELAY, KEEPALIVE_DELAY_MS}; +use ctap2::ctap::hid::HidPacketIterator; #[cfg(feature = "with_ctap1")] use ctap2::env::tock::blink_leds; use ctap2::env::tock::{switch_off_leds, wink_leds, TockEnv}; @@ -46,12 +47,72 @@ use libtock_drivers::console::Console; use libtock_drivers::result::FlexUnwrap; use libtock_drivers::timer::Duration; use libtock_drivers::usb_ctap_hid; +use usb_ctap_hid::UsbEndpoint; libtock_core::stack_size! {0x4000} const SEND_TIMEOUT: Milliseconds = Milliseconds(1000); const KEEPALIVE_DELAY_TOCK: Duration = Duration::from_ms(KEEPALIVE_DELAY_MS as isize); +#[cfg(not(feature = "vendor_hid"))] +const NUM_ENDPOINTS: usize = 1; +#[cfg(feature = "vendor_hid")] +const NUM_ENDPOINTS: usize = 2; + +// The reply/replies that are queued for each endpoint. +struct EndpointReply { + endpoint: UsbEndpoint, + transport: Transport, + reply: HidPacketIterator, +} + +impl EndpointReply { + pub fn new(endpoint: UsbEndpoint) -> Self { + EndpointReply { + endpoint, + transport: match endpoint { + UsbEndpoint::MainHid => Transport::MainHid, + #[cfg(feature = "vendor_hid")] + UsbEndpoint::VendorHid => Transport::VendorHid, + }, + reply: HidPacketIterator::none(), + } + } +} + +// A single packet to send. +struct SendPacket { + transport: Transport, + packet: [u8; 64], +} + +struct EndpointReplies { + replies: [EndpointReply; NUM_ENDPOINTS], +} + +impl EndpointReplies { + pub fn new() -> Self { + EndpointReplies { + replies: [ + EndpointReply::new(UsbEndpoint::MainHid), + #[cfg(feature = "vendor_hid")] + EndpointReply::new(UsbEndpoint::VendorHid), + ], + } + } + + pub fn next_packet(&mut self) -> Option { + for ep in self.replies.iter_mut() { + if let Some(packet) = ep.reply.next() { + return Some(SendPacket { + transport: ep.transport, + packet, + }); + } + } + None + } +} fn main() { let clock = new_clock(); @@ -67,6 +128,8 @@ fn main() { let mut led_counter = 0; let mut last_led_increment = boot_time; + let mut replies = EndpointReplies::new(); + // Main loop. If CTAP1 is used, we register button presses for U2F while receiving and waiting. // The way TockOS and apps currently interact, callbacks need a yield syscall to execute, // making consistent blinking patterns and sending keepalives harder. @@ -89,21 +152,53 @@ fn main() { button.enable().flex_unwrap(); } + // Variable for use in both the send_and_maybe_recv and recv cases. + let mut usb_endpoint: Option = None; let mut pkt_request = [0; 64]; - let usb_endpoint = - match usb_ctap_hid::recv_with_timeout(&mut pkt_request, KEEPALIVE_DELAY_TOCK) - .flex_unwrap() - { - usb_ctap_hid::SendOrRecvStatus::Received(endpoint) => { + + if let Some(mut packet) = replies.next_packet() { + // send and receive. + let hid_connection = packet.transport.hid_connection(ctap.env()); + match hid_connection.send_and_maybe_recv(&mut packet.packet, SEND_TIMEOUT) { + Ok(SendOrRecvStatus::Timeout) => { #[cfg(feature = "debug_ctap")] - print_packet_notice("Received packet", &clock); - Some(endpoint) + print_packet_notice("Sending packet timed out", &clock); + // TODO: reset the ctap_hid state. + // Since sending the packet timed out, we cancel this reply. + break; } - usb_ctap_hid::SendOrRecvStatus::Sent => { - panic!("Returned transmit status on receive") + Ok(SendOrRecvStatus::Sent) => { + #[cfg(feature = "debug_ctap")] + print_packet_notice("Sent packet", &clock); } - usb_ctap_hid::SendOrRecvStatus::Timeout => None, - }; + Ok(SendOrRecvStatus::Received(ep)) => { + #[cfg(feature = "debug_ctap")] + print_packet_notice("Received another packet", &clock); + usb_endpoint = Some(ep); + + // Copy to incoming packet to local buffer to be consistent + // with the receive flow. + pkt_request = packet.packet; + } + Err(_) => panic!("Error sending packet"), + } + } else { + // receive + usb_endpoint = + match usb_ctap_hid::recv_with_timeout(&mut pkt_request, KEEPALIVE_DELAY_TOCK) + .flex_unwrap() + { + usb_ctap_hid::SendOrRecvStatus::Received(endpoint) => { + #[cfg(feature = "debug_ctap")] + print_packet_notice("Received packet", &clock); + Some(endpoint) + } + usb_ctap_hid::SendOrRecvStatus::Sent => { + panic!("Returned transmit status on receive") + } + usb_ctap_hid::SendOrRecvStatus::Timeout => None, + }; + } let now = clock.try_now().unwrap(); #[cfg(feature = "with_ctap1")] @@ -127,32 +222,27 @@ fn main() { if let Some(endpoint) = usb_endpoint { let transport = match endpoint { - usb_ctap_hid::UsbEndpoint::MainHid => Transport::MainHid, + UsbEndpoint::MainHid => Transport::MainHid, #[cfg(feature = "vendor_hid")] - usb_ctap_hid::UsbEndpoint::VendorHid => Transport::VendorHid, + UsbEndpoint::VendorHid => Transport::VendorHid, }; let reply = ctap.process_hid_packet(&pkt_request, transport, now); - // This block handles sending packets. - for mut pkt_reply in reply { - let hid_connection = transport.hid_connection(ctap.env()); - match hid_connection.send_or_recv_with_timeout(&mut pkt_reply, SEND_TIMEOUT) { - Ok(SendOrRecvStatus::Timeout) => { - #[cfg(feature = "debug_ctap")] - print_packet_notice("Sending packet timed out", &clock); - // TODO: reset the ctap_hid state. - // Since sending the packet timed out, we cancel this reply. + if reply.has_data() { + // Update endpoint with the reply. + for ep in replies.replies.iter_mut() { + if ep.endpoint == endpoint { + if ep.reply.has_data() { + #[cfg(feature = "debug_ctap")] + writeln!( + Console::new(), + "Warning overwriting existing reply for endpoint {}", + endpoint as usize + ) + .unwrap(); + } + ep.reply = reply; break; } - Ok(SendOrRecvStatus::Sent) => { - #[cfg(feature = "debug_ctap")] - print_packet_notice("Sent packet", &clock); - } - Ok(SendOrRecvStatus::Received) => { - #[cfg(feature = "debug_ctap")] - print_packet_notice("Received an UNEXPECTED packet", &clock); - // TODO: handle this unexpected packet. - } - Err(_) => panic!("Error sending packet"), } } } diff --git a/tools/vendor_hid_test.py b/tools/vendor_hid_test.py index fc559bc..7c72e03 100644 --- a/tools/vendor_hid_test.py +++ b/tools/vendor_hid_test.py @@ -1,4 +1,5 @@ """These tests verify the functionality of the VendorHID interface.""" +import fido2 from fido2 import ctap from fido2.hid import CtapHidDevice from fido2.hid.base import CtapHidConnection @@ -8,6 +9,7 @@ import hid import time from typing import Dict, Iterable import unittest +from unittest.mock import patch _OPENSK_VID = 0x1915 _OPENSK_PID = 0x521F @@ -278,6 +280,12 @@ def get_fido_device() -> CtapHidDevice: raise Exception('Unable to find Fido device') +def get_fido_device_vendor() -> CtapHidDevice: + # Patch for the Vendor Usage Page. + with patch.object(fido2.hid.base, 'FIDO_USAGE_PAGE', 0xFF00): + return get_fido_device() + + class CliInteraction(UserInteraction): """Sends cancel messages while prompting user.""" @@ -306,6 +314,7 @@ class CancelTests(unittest.TestCase): @classmethod def setUpClass(cls): cls.fido = get_fido_device() + cls.vendor = get_fido_device_vendor() def setUp(self) -> None: super().setUp() @@ -348,6 +357,21 @@ class CancelTests(unittest.TestCase): self.assertEqual(context.exception.cause.code, ctap.CtapError.ERR.USER_ACTION_TIMEOUT) + def test_cancel_ignores_wrong_interface(self): + cid = self.fido._channel_id # pylint: disable=protected-access + connection = self.vendor._connection # pylint: disable=protected-access + client = Fido2Client( + self.fido, + 'https://example.com', + user_interaction=CliInteraction(cid, connection)) + + with self.assertRaises(ClientError) as context: + client.make_credential(self.create_options['publicKey']) + + self.assertEqual(context.exception.code, ClientError.ERR.TIMEOUT) + self.assertEqual(context.exception.cause.code, + ctap.CtapError.ERR.USER_ACTION_TIMEOUT) + def test_timeout(self): client = Fido2Client( self.fido, 'https://example.com', user_interaction=CliInteraction())