* [PATCH 1/4] hw/usb/u2f: Add `start` and `stop` callbacks to U2F key class
2024-06-25 14:53 [PATCH 0/4] hw/usb/u2f-passthru: U2F keepalive fixes David Bouman
@ 2024-06-25 14:53 ` David Bouman
2024-06-25 14:53 ` [PATCH 2/4] hw/usb/u2f-passthru: Do not needlessly retain handle to hidraw chardev David Bouman
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: David Bouman @ 2024-06-25 14:53 UTC (permalink / raw)
To: qemu-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/4] hw/usb/u2f-passthru: Do not needlessly retain handle to hidraw chardev
2024-06-25 14:53 [PATCH 0/4] hw/usb/u2f-passthru: U2F keepalive fixes David Bouman
2024-06-25 14:53 ` [PATCH 1/4] hw/usb/u2f: Add `start` and `stop` callbacks to U2F key class David Bouman
@ 2024-06-25 14:53 ` David Bouman
2024-06-25 14:53 ` [PATCH 3/4] hw/usb/u2f-passthru: Clean up code David Bouman
2024-06-25 14:53 ` [PATCH 4/4] hw/usb/u2f-passthru: Implement FIDO U2FHID keep-alive David Bouman
3 siblings, 0 replies; 5+ messages in thread
From: David Bouman @ 2024-06-25 14:53 UTC (permalink / raw)
To: qemu-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 3/4] hw/usb/u2f-passthru: Clean up code
2024-06-25 14:53 [PATCH 0/4] hw/usb/u2f-passthru: U2F keepalive fixes David Bouman
2024-06-25 14:53 ` [PATCH 1/4] hw/usb/u2f: Add `start` and `stop` callbacks to U2F key class David Bouman
2024-06-25 14:53 ` [PATCH 2/4] hw/usb/u2f-passthru: Do not needlessly retain handle to hidraw chardev David Bouman
@ 2024-06-25 14:53 ` David Bouman
2024-06-25 14:53 ` [PATCH 4/4] hw/usb/u2f-passthru: Implement FIDO U2FHID keep-alive David Bouman
3 siblings, 0 replies; 5+ messages in thread
From: David Bouman @ 2024-06-25 14:53 UTC (permalink / raw)
To: qemu-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 4/4] hw/usb/u2f-passthru: Implement FIDO U2FHID keep-alive
2024-06-25 14:53 [PATCH 0/4] hw/usb/u2f-passthru: U2F keepalive fixes David Bouman
` (2 preceding siblings ...)
2024-06-25 14:53 ` [PATCH 3/4] hw/usb/u2f-passthru: Clean up code David Bouman
@ 2024-06-25 14:53 ` David Bouman
3 siblings, 0 replies; 5+ messages in thread
From: David Bouman @ 2024-06-25 14:53 UTC (permalink / raw)
To: qemu-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 5+ messages in thread