[PATCH 0/4] hw/usb/u2f-passthru: U2F keepalive fixes

[PATCH 0/4] hw/usb/u2f-passthru: U2F keepalive fixes

[David Bouman]

Hello,

The u2f-passthru device is currently broken for (at least) the Yubikey 5.
(gitlab: https://gitlab.com/qemu-project/qemu/-/issues/2293)

This patchset aims to fix the issue by properly handling the
U2F keepalive response in the u2f-passthru device.

I initially suspected the hidraw chardev handle management to be the
culprit, so I also implemented a more efficient strategy where the host
hidraw chardev is only opened as long as the guest actually needs it to be opened.

This turned out to not be the root cause, but regardless, it's probably the 
right way to be doing it. Hence, I also included these patches here.

The patches were tested with `pamu2fcfg` in an x86_64 Linux guest
with the u2f-passthru device backed by a physical Yubikey 5 (type C model). 

---

I apologize for any anticipatory errors, I'm not really acquainted with the
USB/HID/U2F protocols, nor with QEMU. Happy to hear your feedback!

David Bouman (4):
  hw/usb/u2f: Add `start` and `stop` callbacks to U2F key class
  hw/usb/u2f-passthru: Do not needlessly retain handle to hidraw chardev
  hw/usb/u2f-passthru: Clean up code
  hw/usb/u2f-passthru: Implement FIDO U2FHID keep-alive

 hw/usb/u2f-passthru.c | 163 ++++++++++++++++++++++++++++--------------
 hw/usb/u2f.c          |  23 ++++++
 hw/usb/u2f.h          |   5 ++
 3 files changed, 138 insertions(+), 53 deletions(-)

-- 
2.34.1

[PATCH 1/4] hw/usb/u2f: Add `start` and `stop` callbacks to U2F key class

[David Bouman]

Preparation for improved u2f-passthru hidraw handle lifetimes:

These callbacks can be implemented by the backing implementation,
i.e. u2f-passthru or u2f-emulated.

The start callback is invoked when the device receives an INTR IN, and
the stop callback is invoked when the endpoint has been unlinked/stopped.

For a Linux guest, the start callback corresponds to the `hid_hw_open` and
`hid_hw_close` routines, invoked whenever the first user opens, respectively
the last user closes the emulated hidraw device.

Signed-off-by: David Bouman <dbouman03@gmail.com>
---
 hw/usb/u2f.c | 23 +++++++++++++++++++++++
 hw/usb/u2f.h |  5 +++++
 2 files changed, 28 insertions(+)

diff --git a/hw/usb/u2f.c b/hw/usb/u2f.c
index 1fb59cf404..cc1abd6c7d 100644
--- a/hw/usb/u2f.c
+++ b/hw/usb/u2f.c
@@ -253,6 +253,12 @@ static void u2f_key_handle_data(USBDevice *dev, USBPacket *p)
     case USB_TOKEN_IN:
         packet_in = u2f_pending_in_get(key);
         if (packet_in == NULL) {
+            U2FKeyClass *kc = U2F_KEY_GET_CLASS(key);
+
+            /* An early INTR IN is sent to kick the device. */
+            if (!key->started) {
+                key->started = (kc->start ? kc->start(key) : true);
+            }
             p->status = USB_RET_NAK;
             return;
         }
@@ -317,6 +323,21 @@ const VMStateDescription vmstate_u2f_key = {
     }
 };
 
+static void u2f_ep_stopped(USBDevice *dev, USBEndpoint *ep)
+{
+    U2FKeyState *key = U2F_KEY(dev);
+    U2FKeyClass *kc = U2F_KEY_GET_CLASS(key);
+
+    if (!key->started) {
+        return;
+    }
+    if (kc->stop) {
+        kc->stop(key);
+    }
+    key->started = false;
+}
+
+
 static void u2f_key_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -332,6 +353,8 @@ static void u2f_key_class_init(ObjectClass *klass, void *data)
     uc->unrealize      = u2f_key_unrealize;
     dc->desc           = "QEMU U2F key";
     dc->vmsd           = &vmstate_u2f_key;
+
+    uc->ep_stopped     = u2f_ep_stopped;
 }
 
 static const TypeInfo u2f_key_info = {
diff --git a/hw/usb/u2f.h b/hw/usb/u2f.h
index 8bff13141a..56d989c783 100644
--- a/hw/usb/u2f.h
+++ b/hw/usb/u2f.h
@@ -49,6 +49,9 @@ struct U2FKeyClass {
                             const uint8_t packet[U2FHID_PACKET_SIZE]);
     void (*realize)(U2FKeyState *key, Error **errp);
     void (*unrealize)(U2FKeyState *key);
+
+    bool (*start)(U2FKeyState *key);
+    void (*stop)(U2FKeyState *key);
 };
 
 /*
@@ -64,6 +67,8 @@ struct U2FKeyState {
     uint8_t pending_in_start;
     uint8_t pending_in_end;
     uint8_t pending_in_num;
+
+    bool started;
 };
 
 /*
-- 
2.34.1

[PATCH 2/4] hw/usb/u2f-passthru: Do not needlessly retain handle to hidraw chardev

[David Bouman]

The Linux kernel presumes a hidraw device to be "active" as long
as an open handle to its character device exists, and during that time
will actively poll its bus for new messages.

The u2f-passthru device keeps an open handle to the hidraw character device
for its entire lifetime, wasting power and causing its queue to be
clogged with irrelevant packets that were not initiated by the guest.

To mitigate this, use the u2f `start` and `stop` callbacks to
dynamically open the handle when the guest is about to use the
u2f-passthru device (start callback), and close it again whenever
the guest stops using it (stop callback).

Signed-off-by: David Bouman <dbouman03@gmail.com>
Fixes: 299976b050bf (hw/usb: Add U2F key passthru mode)
---
 hw/usb/u2f-passthru.c | 84 +++++++++++++++++++++++++++++++------------
 1 file changed, 61 insertions(+), 23 deletions(-)

diff --git a/hw/usb/u2f-passthru.c b/hw/usb/u2f-passthru.c
index b7025d303d..54062ab4d5 100644
--- a/hw/usb/u2f-passthru.c
+++ b/hw/usb/u2f-passthru.c
@@ -118,7 +118,10 @@ static inline uint16_t packet_init_get_bcnt(
 static void u2f_passthru_reset(U2FPassthruState *key)
 {
     timer_del(&key->timer);
-    qemu_set_fd_handler(key->hidraw_fd, NULL, NULL, key);
+
+    if (key->hidraw_fd >= 0) {
+        qemu_set_fd_handler(key->hidraw_fd, NULL, NULL, key);
+    }
     key->last_transaction_time = 0;
     key->current_transactions_start = 0;
     key->current_transactions_end = 0;
@@ -456,47 +459,79 @@ static int u2f_passthru_open_from_scan(void)
 }
 #endif
 
-static void u2f_passthru_unrealize(U2FKeyState *base)
-{
-    U2FPassthruState *key = PASSTHRU_U2F_KEY(base);
-
-    u2f_passthru_reset(key);
-    qemu_close(key->hidraw_fd);
-}
 
-static void u2f_passthru_realize(U2FKeyState *base, Error **errp)
+static int u2f_passthru_open_hidraw(U2FPassthruState *key, Error** errp)
 {
-    U2FPassthruState *key = PASSTHRU_U2F_KEY(base);
     int fd;
-
     if (key->hidraw == NULL) {
 #ifdef CONFIG_LIBUDEV
         fd = u2f_passthru_open_from_scan();
-        if (fd < 0) {
+
+        if (fd < 0 && errp) {
             error_setg(errp, "%s: Failed to find a U2F USB device",
                        TYPE_U2F_PASSTHRU);
-            return;
         }
 #else
-        error_setg(errp, "%s: Missing hidraw", TYPE_U2F_PASSTHRU);
-        return;
+        if (errp) {
+            error_setg(errp, "%s: Missing hidraw", TYPE_U2F_PASSTHRU);
+        }
+        return -1;
 #endif
     } else {
         fd = qemu_open_old(key->hidraw, O_RDWR);
-        if (fd < 0) {
+
+        if (fd < 0 && errp) {
             error_setg(errp, "%s: Failed to open %s", TYPE_U2F_PASSTHRU,
                        key->hidraw);
-            return;
-        }
 
-        if (!u2f_passthru_is_u2f_device(fd)) {
+        } else if (!u2f_passthru_is_u2f_device(fd)) {
             qemu_close(fd);
-            error_setg(errp, "%s: Passed hidraw does not represent "
-                       "a U2F HID device", TYPE_U2F_PASSTHRU);
-            return;
+            if (errp) {
+                error_setg(errp, "%s: Passed hidraw does not represent "
+                           "a U2F HID device", TYPE_U2F_PASSTHRU);
+            }
+            return -1;
         }
     }
-    key->hidraw_fd = fd;
+
+    return fd;
+}
+
+static bool u2f_passthru_start(U2FKeyState *base)
+{
+    U2FPassthruState *key = PASSTHRU_U2F_KEY(base);
+
+    if (key->hidraw_fd < 0) {
+        key->hidraw_fd = u2f_passthru_open_hidraw(key, NULL);
+    }
+    return key->hidraw_fd >= 0;
+}
+
+static void u2f_passthru_stop(U2FKeyState *base)
+{
+    U2FPassthruState *key = PASSTHRU_U2F_KEY(base);
+
+    u2f_passthru_reset(key);
+    if (key->hidraw_fd >= 0) {
+        qemu_close(key->hidraw_fd);
+        key->hidraw_fd = -1;
+    }
+}
+
+static void u2f_passthru_unrealize(U2FKeyState *base)
+{
+    u2f_passthru_stop(base);
+}
+static void u2f_passthru_realize(U2FKeyState *base, Error **errp)
+{
+    U2FPassthruState *key = PASSTHRU_U2F_KEY(base);
+    int fd = u2f_passthru_open_hidraw(key, errp);
+
+    /* we only open the fd to error at start */
+    if (fd >= 0) {
+        qemu_close(fd);
+    }
+    key->hidraw_fd = -1;
     u2f_passthru_reset(key);
 }
 
@@ -531,6 +566,9 @@ static void u2f_passthru_class_init(ObjectClass *klass, void *data)
     kc->realize = u2f_passthru_realize;
     kc->unrealize = u2f_passthru_unrealize;
     kc->recv_from_guest = u2f_passthru_recv_from_guest;
+    kc->start = u2f_passthru_start;
+    kc->stop  = u2f_passthru_stop;
+
     dc->desc = "QEMU U2F passthrough key";
     dc->vmsd = &u2f_passthru_vmstate;
     device_class_set_props(dc, u2f_passthru_properties);
-- 
2.34.1

[PATCH 3/4] hw/usb/u2f-passthru: Clean up code

[David Bouman]

Prepare for implementing the FIDO-U2F keepalive feature:

Represent all u2fhid frames using one coherent structure,
and make casts explicit.

Signed-off-by: David Bouman <dbouman03@gmail.com>
---
 hw/usb/u2f-passthru.c | 73 ++++++++++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 29 deletions(-)

diff --git a/hw/usb/u2f-passthru.c b/hw/usb/u2f-passthru.c
index 54062ab4d5..d0fb7b377c 100644
--- a/hw/usb/u2f-passthru.c
+++ b/hw/usb/u2f-passthru.c
@@ -87,30 +87,45 @@ struct U2FPassthruState {
 #define PACKET_CONT_HEADER_SIZE 5
 #define PACKET_CONT_DATA_SIZE (U2FHID_PACKET_SIZE - PACKET_CONT_HEADER_SIZE)
 
-struct packet_init {
+/* Frame definition */
+
+#define U2FHID_CMD_PING      0x81
+#define U2FHID_CMD_MSG       0x83
+#define U2FHID_CMD_INIT      0x86
+#define U2FHID_CMD_WINK      0x88
+#define U2FHID_CMD_CBOR      0x90
+#define U2FHID_CMD_CANCEL    0x91
+#define U2FHID_CMD_KEEPALIVE 0xbb
+#define U2FHID_ERROR         0xbf
+
+struct u2fhid_frame {
     uint32_t cid;
-    uint8_t cmd;
-    uint8_t bcnth;
-    uint8_t bcntl;
-    uint8_t data[PACKET_INIT_DATA_SIZE];
+    union {
+        uint8_t type;
+        struct {
+            uint8_t cmd;
+            uint8_t bcnth;
+            uint8_t bcntl;
+            uint8_t data[PACKET_INIT_DATA_SIZE];
+        } init;
+        struct {
+            uint8_t seq;
+            uint8_t data[PACKET_CONT_DATA_SIZE];
+        } cont;
+    };
 } QEMU_PACKED;
 
-static inline uint32_t packet_get_cid(const void *packet)
+static inline bool packet_is_init(struct u2fhid_frame *packet)
 {
-    return *((uint32_t *)packet);
-}
-
-static inline bool packet_is_init(const void *packet)
-{
-    return ((uint8_t *)packet)[4] & (1 << 7);
+    return !!(packet->type & (1 << 7));
 }
 
 static inline uint16_t packet_init_get_bcnt(
-        const struct packet_init *packet_init)
+        const struct u2fhid_frame *packet)
 {
     uint16_t bcnt = 0;
-    bcnt |= packet_init->bcnth << 8;
-    bcnt |= packet_init->bcntl;
+    bcnt |= (uint16_t)(packet->init.bcnth) << 8;
+    bcnt |= packet->init.bcntl;
 
     return bcnt;
 }
@@ -237,13 +252,13 @@ static void u2f_transaction_add(U2FPassthruState *key, uint32_t cid,
 static void u2f_passthru_read(void *opaque);
 
 static void u2f_transaction_start(U2FPassthruState *key,
-                                  const struct packet_init *packet_init)
+                                  const struct u2fhid_frame *packet_init)
 {
     int64_t time;
 
     /* Transaction */
     if (packet_init->cid == BROADCAST_CID) {
-        u2f_transaction_add(key, packet_init->cid, packet_init->data);
+        u2f_transaction_add(key, packet_init->cid, packet_init->init.data);
     } else {
         u2f_transaction_add(key, packet_init->cid, NULL);
     }
@@ -259,20 +274,19 @@ static void u2f_transaction_start(U2FPassthruState *key,
 }
 
 static void u2f_passthru_recv_from_host(U2FPassthruState *key,
-                                    const uint8_t packet[U2FHID_PACKET_SIZE])
+                                const uint8_t raw_packet[U2FHID_PACKET_SIZE])
 {
     struct transaction *transaction;
     uint32_t cid;
 
+    struct u2fhid_frame *packet = (void *)raw_packet;
     /* Retrieve transaction */
-    cid = packet_get_cid(packet);
+    cid = packet->cid;
     if (cid == BROADCAST_CID) {
-        struct packet_init *packet_init;
         if (!packet_is_init(packet)) {
             return;
         }
-        packet_init = (struct packet_init *)packet;
-        transaction = u2f_transaction_get_from_nonce(key, packet_init->data);
+        transaction = u2f_transaction_get_from_nonce(key, packet->init.data);
     } else {
         transaction = u2f_transaction_get(key, cid);
     }
@@ -283,13 +297,12 @@ static void u2f_passthru_recv_from_host(U2FPassthruState *key,
     }
 
     if (packet_is_init(packet)) {
-        struct packet_init *packet_init = (struct packet_init *)packet;
-        transaction->resp_bcnt = packet_init_get_bcnt(packet_init);
+        transaction->resp_bcnt = packet_init_get_bcnt(packet);
         transaction->resp_size = PACKET_INIT_DATA_SIZE;
 
-        if (packet_init->cid == BROADCAST_CID) {
+        if (packet->cid == BROADCAST_CID) {
             /* Nonce checking for legitimate response */
-            if (memcmp(transaction->nonce, packet_init->data, NONCE_SIZE)
+            if (memcmp(transaction->nonce, packet->init.data, NONCE_SIZE)
                 != 0) {
                 return;
             }
@@ -302,7 +315,7 @@ static void u2f_passthru_recv_from_host(U2FPassthruState *key,
     if (transaction->resp_size >= transaction->resp_bcnt) {
         u2f_transaction_close(key, cid);
     }
-    u2f_send_to_guest(&key->base, packet);
+    u2f_send_to_guest(&key->base, raw_packet);
 }
 
 static void u2f_passthru_read(void *opaque)
@@ -333,14 +346,16 @@ static void u2f_passthru_read(void *opaque)
 }
 
 static void u2f_passthru_recv_from_guest(U2FKeyState *base,
-                                    const uint8_t packet[U2FHID_PACKET_SIZE])
+                                const uint8_t raw_packet[U2FHID_PACKET_SIZE])
 {
     U2FPassthruState *key = PASSTHRU_U2F_KEY(base);
     uint8_t host_packet[U2FHID_PACKET_SIZE + 1];
     ssize_t written;
 
+    struct u2fhid_frame *packet = (void *)raw_packet;
+
     if (packet_is_init(packet)) {
-        u2f_transaction_start(key, (struct packet_init *)packet);
+        u2f_transaction_start(key, packet);
     }
 
     host_packet[0] = 0;
-- 
2.34.1

[PATCH 4/4] hw/usb/u2f-passthru: Implement FIDO U2FHID keep-alive

[David Bouman]

FIDO U2FHID features a keep-alive response command (code 0xbb). A
keep-alive response signifies that the request is being processed
and the transaction should not be closed yet.

u2f-passthru does not recognize this command, and hence closes the
transaction prematurely upon receiving it. This prevents some U2F
security keys from being passed through correctly (Yubikey 4/5).

Fix this by recognizing the keep-alive response and, if received,
by keeping the transaction alive regardless of the resp_bcnt limit.

Signed-off-by: David Bouman <dbouman03@gmail.com>
Fixes: 299976b050bf (hw/usb: Add U2F key passthru mode)
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2293
---
 hw/usb/u2f-passthru.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/usb/u2f-passthru.c b/hw/usb/u2f-passthru.c
index d0fb7b377c..c1e4db3467 100644
--- a/hw/usb/u2f-passthru.c
+++ b/hw/usb/u2f-passthru.c
@@ -296,10 +296,14 @@ static void u2f_passthru_recv_from_host(U2FPassthruState *key,
         return;
     }
 
+    bool keepalive = false;
     if (packet_is_init(packet)) {
         transaction->resp_bcnt = packet_init_get_bcnt(packet);
         transaction->resp_size = PACKET_INIT_DATA_SIZE;
 
+        if (packet->init.cmd == U2FHID_CMD_KEEPALIVE)
+            keepalive = true;
+
         if (packet->cid == BROADCAST_CID) {
             /* Nonce checking for legitimate response */
             if (memcmp(transaction->nonce, packet->init.data, NONCE_SIZE)
@@ -312,7 +316,7 @@ static void u2f_passthru_recv_from_host(U2FPassthruState *key,
     }
 
     /* Transaction end check */
-    if (transaction->resp_size >= transaction->resp_bcnt) {
+    if (!keepalive && transaction->resp_size >= transaction->resp_bcnt) {
         u2f_transaction_close(key, cid);
     }
     u2f_send_to_guest(&key->base, raw_packet);
-- 
2.34.1