Some USB keyboards report 9 or 10-byte max packet sizes, instead of the 8-byte max specified by the USB HID spec. Handle this by increasing the size of the keyevent struct to 10 bytes, and using the key array size of the usbkeyinfo struct as loop bounds rather than that of the keyevent struct (since the former will always be smaller, and within spec).
Test: built/boot on Google Pixel Slate, observe keyboard functional
Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- src/hw/usb-hid.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/src/hw/usb-hid.c b/src/hw/usb-hid.c index fa4d9a2..bce1b5f 100644 --- a/src/hw/usb-hid.c +++ b/src/hw/usb-hid.c @@ -8,6 +8,7 @@ #include "config.h" // CONFIG_* #include "output.h" // dprintf #include "ps2port.h" // ATKBD_CMD_GETID +#include <string.h> //memset #include "usb.h" // usb_ctrlrequest #include "usb-hid.h" // usb_keyboard_setup #include "util.h" // process_key @@ -59,8 +60,10 @@ usb_kbd_setup(struct usbdevice_s *usbdev // XXX - this enables the first found keyboard (could be random) return -1;
- if (epdesc->wMaxPacketSize != 8) + if (epdesc->wMaxPacketSize > 10) { + dprintf(1, "USB keyboard endpoint wMaxPacketSize > 10; aborting\n"); return -1; + }
// Enable "boot" protocol. int ret = set_protocol(usbdev->defpipe, 0); @@ -163,11 +166,15 @@ static u16 ModifierToScanCode[] VAR16 = {
#define RELEASEBIT 0x80
-// Format of USB keyboard event data +// Format of USB keyboard event data. +// Some keyboards use a 9/10 byte packet size, +// so account for that here to prevent buffer +// overflow. We'll ignore the 9th/10th bytes +// as it's out of spec. struct keyevent { u8 modifiers; u8 reserved; - u8 keys[6]; + u8 keys[8]; };
// Translate data from KeyToScanCode[] to calls to process_key(). @@ -253,7 +260,7 @@ handle_key(struct keyevent *data) break; int j; for (j=0;; j++) { - if (j>=ARRAY_SIZE(data->keys)) { + if (j>=ARRAY_SIZE(old.keys)) { // Key released. procscankey(key, RELEASEBIT, data->modifiers); if (i+1 >= ARRAY_SIZE(old.keys) || !old.keys[i+1]) @@ -274,7 +281,7 @@ handle_key(struct keyevent *data) // Process new keys procmodkey(data->modifiers & ~old.modifiers, 0); old.modifiers = data->modifiers; - for (i=0; i<ARRAY_SIZE(data->keys); i++) { + for (i=0; i<ARRAY_SIZE(old.keys); i++) { u8 key = data->keys[i]; if (!key) continue;
On Sat, Dec 14, 2019 at 11:26:17AM -0600, Matt DeVillier wrote:
Some USB keyboards report 9 or 10-byte max packet sizes, instead of the 8-byte max specified by the USB HID spec. Handle this by increasing the size of the keyevent struct to 10 bytes, and using the key array size of the usbkeyinfo struct as loop bounds rather than that of the keyevent struct (since the former will always be smaller, and within spec).
Test: built/boot on Google Pixel Slate, observe keyboard functional
Thanks. I think it would be good if the relationship between the test and the struct sizes was more clear. What about the below (totally untested)?
-Kevin
--- a/src/hw/usb-hid.c +++ b/src/hw/usb-hid.c @@ -49,6 +49,15 @@ set_idle(struct usb_pipe *pipe, int ms) #define KEYREPEATWAITMS 500 #define KEYREPEATMS 33
+// Format of USB keyboard event data +struct keyevent { + u8 modifiers; + u8 reserved; + u8 keys[6]; +}; + +#define MAX_KBD_EVENT 10 + static int usb_kbd_setup(struct usbdevice_s *usbdev , struct usb_endpoint_descriptor *epdesc) @@ -59,8 +68,12 @@ usb_kbd_setup(struct usbdevice_s *usbdev // XXX - this enables the first found keyboard (could be random) return -1;
- if (epdesc->wMaxPacketSize != 8) + if (epdesc->wMaxPacketSize < sizeof(struct keyevent) + || epdesc->wMaxPacketSize > MAX_KBD_EVENT) { + dprintf(1, "USB keyboard wMaxPacketSize=%d; aborting\n" + , epdesc->wMaxPacketSize); return -1; + }
// Enable "boot" protocol. int ret = set_protocol(usbdev->defpipe, 0); @@ -79,6 +92,14 @@ usb_kbd_setup(struct usbdevice_s *usbdev return 0; }
+// Format of USB mouse event data +struct mouseevent { + u8 buttons; + u8 x, y; +}; + +#define MAX_MOUSE_EVENT 8 + static int usb_mouse_setup(struct usbdevice_s *usbdev , struct usb_endpoint_descriptor *epdesc) @@ -89,8 +110,12 @@ usb_mouse_setup(struct usbdevice_s *usbdev // XXX - this enables the first found mouse (could be random) return -1;
- if (epdesc->wMaxPacketSize < 3 || epdesc->wMaxPacketSize > 8) + if (epdesc->wMaxPacketSize < sizeof(struct mouseevent) + || epdesc->wMaxPacketSize > MAX_MOUSE_EVENT) { + dprintf(1, "USB mouse wMaxPacketSize=%d; aborting\n" + , epdesc->wMaxPacketSize); return -1; + }
// Enable "boot" protocol. int ret = set_protocol(usbdev->defpipe, 0); @@ -163,13 +188,6 @@ static u16 ModifierToScanCode[] VAR16 = {
#define RELEASEBIT 0x80
-// Format of USB keyboard event data -struct keyevent { - u8 modifiers; - u8 reserved; - u8 keys[6]; -}; - // Translate data from KeyToScanCode[] to calls to process_key(). static void prockeys(u16 scancode, u8 key_release, u8 mods) @@ -309,11 +327,11 @@ usb_check_key(void) return;
for (;;) { - struct keyevent data; - int ret = usb_poll_intr(pipe, &data); + u8 data[MAX_KBD_EVENT]; + int ret = usb_poll_intr(pipe, data); if (ret) break; - handle_key(&data); + handle_key((void*)data); } }
@@ -349,13 +367,6 @@ usb_kbd_command(int command, u8 *param) * Mouse events ****************************************************************/
-// Format of USB mouse event data -struct mouseevent { - u8 buttons; - u8 x, y; - u8 reserved[5]; -}; - // Process USB mouse data. static void handle_mouse(struct mouseevent *data) @@ -381,11 +392,11 @@ usb_check_mouse(void) return;
for (;;) { - struct mouseevent data; - int ret = usb_poll_intr(pipe, &data); + u8 data[MAX_MOUSE_EVENT]; + int ret = usb_poll_intr(pipe, data); if (ret) break; - handle_mouse(&data); + handle_mouse((void*)data); } }
On Sun, Dec 15, 2019 at 6:26 PM Kevin O'Connor kevin@koconnor.net wrote:
On Sat, Dec 14, 2019 at 11:26:17AM -0600, Matt DeVillier wrote:
Some USB keyboards report 9 or 10-byte max packet sizes, instead of the 8-byte max specified by the USB HID spec. Handle this by increasing the size of the keyevent struct to 10 bytes, and using the key array size of the usbkeyinfo struct as loop bounds rather than that of the keyevent struct (since the former will always be smaller, and within spec).
Test: built/boot on Google Pixel Slate, observe keyboard functional
Thanks. I think it would be good if the relationship between the test and the struct sizes was more clear. What about the below (totally untested)?
-Kevin
your version produces a working keyboard on the Pixel Slate (when used in conjunction with my other patch), but unsure how to test the mouse TBH.
thanks, Matt
--- a/src/hw/usb-hid.c +++ b/src/hw/usb-hid.c @@ -49,6 +49,15 @@ set_idle(struct usb_pipe *pipe, int ms) #define KEYREPEATWAITMS 500 #define KEYREPEATMS 33
+// Format of USB keyboard event data +struct keyevent {
- u8 modifiers;
- u8 reserved;
- u8 keys[6];
+};
+#define MAX_KBD_EVENT 10
static int usb_kbd_setup(struct usbdevice_s *usbdev , struct usb_endpoint_descriptor *epdesc) @@ -59,8 +68,12 @@ usb_kbd_setup(struct usbdevice_s *usbdev // XXX - this enables the first found keyboard (could be random) return -1;
- if (epdesc->wMaxPacketSize != 8)
if (epdesc->wMaxPacketSize < sizeof(struct keyevent)
|| epdesc->wMaxPacketSize > MAX_KBD_EVENT) {dprintf(1, "USB keyboard wMaxPacketSize=%d; aborting\n", epdesc->wMaxPacketSize); return -1;}
// Enable "boot" protocol. int ret = set_protocol(usbdev->defpipe, 0);
@@ -79,6 +92,14 @@ usb_kbd_setup(struct usbdevice_s *usbdev return 0; }
+// Format of USB mouse event data +struct mouseevent {
- u8 buttons;
- u8 x, y;
+};
+#define MAX_MOUSE_EVENT 8
static int usb_mouse_setup(struct usbdevice_s *usbdev , struct usb_endpoint_descriptor *epdesc) @@ -89,8 +110,12 @@ usb_mouse_setup(struct usbdevice_s *usbdev // XXX - this enables the first found mouse (could be random) return -1;
- if (epdesc->wMaxPacketSize < 3 || epdesc->wMaxPacketSize > 8)
if (epdesc->wMaxPacketSize < sizeof(struct mouseevent)
|| epdesc->wMaxPacketSize > MAX_MOUSE_EVENT) {dprintf(1, "USB mouse wMaxPacketSize=%d; aborting\n", epdesc->wMaxPacketSize); return -1;}
// Enable "boot" protocol. int ret = set_protocol(usbdev->defpipe, 0);
@@ -163,13 +188,6 @@ static u16 ModifierToScanCode[] VAR16 = {
#define RELEASEBIT 0x80
-// Format of USB keyboard event data -struct keyevent {
- u8 modifiers;
- u8 reserved;
- u8 keys[6];
-};
// Translate data from KeyToScanCode[] to calls to process_key(). static void prockeys(u16 scancode, u8 key_release, u8 mods) @@ -309,11 +327,11 @@ usb_check_key(void) return;
for (;;) {
struct keyevent data;int ret = usb_poll_intr(pipe, &data);
u8 data[MAX_KBD_EVENT];int ret = usb_poll_intr(pipe, data); if (ret) break;
handle_key(&data);
handle_key((void*)data);}
@@ -349,13 +367,6 @@ usb_kbd_command(int command, u8 *param)
- Mouse events
****************************************************************/
-// Format of USB mouse event data -struct mouseevent {
- u8 buttons;
- u8 x, y;
- u8 reserved[5];
-};
// Process USB mouse data. static void handle_mouse(struct mouseevent *data) @@ -381,11 +392,11 @@ usb_check_mouse(void) return;
for (;;) {
struct mouseevent data;int ret = usb_poll_intr(pipe, &data);
u8 data[MAX_MOUSE_EVENT];int ret = usb_poll_intr(pipe, data); if (ret) break;
handle_mouse(&data);
handle_mouse((void*)data);}
On Sun, Dec 15, 2019 at 09:26:17PM -0600, Matt DeVillier wrote:
On Sun, Dec 15, 2019 at 6:26 PM Kevin O'Connor kevin@koconnor.net wrote:
On Sat, Dec 14, 2019 at 11:26:17AM -0600, Matt DeVillier wrote:
Some USB keyboards report 9 or 10-byte max packet sizes, instead of the 8-byte max specified by the USB HID spec. Handle this by increasing the size of the keyevent struct to 10 bytes, and using the key array size of the usbkeyinfo struct as loop bounds rather than that of the keyevent struct (since the former will always be smaller, and within spec).
Test: built/boot on Google Pixel Slate, observe keyboard functional
Thanks. I think it would be good if the relationship between the test and the struct sizes was more clear. What about the below (totally untested)?
-Kevin
your version produces a working keyboard on the Pixel Slate (when used in conjunction with my other patch), but unsure how to test the mouse TBH.
Thanks. I committed this change.
-Kevin
-
Kevin O'Connor -
Matt DeVillier