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>

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

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<SendOrRecvStatus, SendOrRecvError>;
 
 pub trait HidConnection {
-    fn send_or_recv_with_timeout(
+    fn send_and_maybe_recv(
         &mut self,
         buf: &mut [u8; 64],
         timeout: Milliseconds<ClockInt>,

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 {

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 {

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<ClockInt>,

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<ClockInt>,
@@ -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))
+            Ok(usb_ctap_hid::SendOrRecvStatus::Received(recv_endpoint)) => {
-                if self.endpoint == recv_endpoint =>
+                Ok(SendOrRecvStatus::Received(recv_endpoint))
-            {
-                Ok(SendOrRecvStatus::Received)
             }
             _ => Err(SendOrRecvError),
         }

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).
+// TODO(kaczmarczyck): Refactor this so that ctap module isn't public.
-#[cfg(feature = "std")]
 pub mod ctap;
-#[cfg(not(feature = "std"))]
-mod ctap;
 pub mod env;
 #[cfg(feature = "std")]
 pub mod test_helpers;

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<ClockInt> = Milliseconds(1000);
 const KEEPALIVE_DELAY_TOCK: Duration<isize> = 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<SendPacket> {
+        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,8 +152,39 @@ fn main() {
             button.enable().flex_unwrap();
         }
 
+        // Variable for use in both the send_and_maybe_recv and recv cases.
+        let mut usb_endpoint: Option<UsbEndpoint> = None;
         let mut pkt_request = [0; 64];
-        let usb_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("Sending packet timed out", &clock);
+                    // TODO: reset the ctap_hid state.
+                    // Since sending the packet timed out, we cancel this reply.
+                    break;
+                }
+                Ok(SendOrRecvStatus::Sent) => {
+                    #[cfg(feature = "debug_ctap")]
+                    print_packet_notice("Sent packet", &clock);
+                }
+                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()
                 {
@@ -104,6 +198,7 @@ fn main() {
                     }
                     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.
+            if reply.has_data() {
-            for mut pkt_reply in reply {
+                // Update endpoint with the reply.
-                let hid_connection = transport.hid_connection(ctap.env());
+                for ep in replies.replies.iter_mut() {
-                match hid_connection.send_or_recv_with_timeout(&mut pkt_reply, SEND_TIMEOUT) {
+                    if ep.endpoint == endpoint {
-                    Ok(SendOrRecvStatus::Timeout) => {
+                        if ep.reply.has_data() {
                             #[cfg(feature = "debug_ctap")]
-                        print_packet_notice("Sending packet timed out", &clock);
+                            writeln!(
-                        // TODO: reset the ctap_hid state.
+                                Console::new(),
-                        // Since sending the packet timed out, we cancel this reply.
+                                "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"),
                 }
             }
         }

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())