Subject: [PATCH 1/2] 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).

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

Signed-off-by: Matt DeVillier 
---
 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..cedec0b 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,7 +60,7 @@ 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)
         return -1;

// Enable "boot" protocol.
@@ -163,11 +164,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 +258,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 +279,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 +315,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;

[Paul Menzel]

Dear Matt,

On 2019-12-13 04:51, 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, zeroizing
> it before use, and using the key array size of the usbkeyinfo

This could have been a separate commit.
> 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
>
> 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..cedec0b 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,7 +60,7 @@ 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)

A debug message would be nice, so it’s easier to spot in the future.
Could be a follow-up commit though.
>      return -1;
>
>  // Enable "boot" protocol.
>
> @@ -163,11 +164,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 +258,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 +279,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 +315,8 @@ usb_check_key(void)
>  for (;;) {
>      struct keyevent data;
>    //zeroize struct as most keyboards won't fill it

Please add a space after the comment characters.
>    memset(&data, 0, sizeof(data));
>    int ret = usb_poll_intr(pipe, &data);
>    if (ret)
>        break;

Kind regards,

Paul