[PATCH v2] hw/usb-hid: handle devices with illegal max packet size

[Matt DeVillier]

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, zeroizing
it before use, 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) to
prevent buffer overflow.

Test: built/boot on Google Pixel Slate, observe keyboard functional

Signed-off-by: Matt DeVillier 
---
 src/hw/usb-hid.c | 19 ++++++++++++++-----
 1 file changed, 14 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;
@@ -310,6 +317,8 @@ usb_check_key(void)

for (;;) {
         struct keyevent data;
+        // zeroize struct as most keyboards won't fill it
+        memset(&data, 0, sizeof(data));
         int ret = usb_poll_intr(pipe, &data);
         if (ret)
             break;