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

Matt DeVillier posted 1 patch 4 years, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/seabios tags/patchew/CAFTm+6C8_0FuKQPXarfwTXOPRMMzn5agjbVT_7qCLE_MsR7kbA@mail.gmail.com
src/hw/usb-hid.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
[SeaBIOS] [PATCH v3] hw/usb-hid: handle devices with illegal max packet size
Posted by Matt DeVillier 4 years, 3 months ago
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;
-- 
2.20.1
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH v3] hw/usb-hid: handle devices with illegal max packet size
Posted by Kevin O'Connor 4 years, 3 months ago
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);
     }
 }
 
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH v3] hw/usb-hid: handle devices with illegal max packet size
Posted by Matt DeVillier 4 years, 3 months ago
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);
>      }
>  }
>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH v3] hw/usb-hid: handle devices with illegal max packet size
Posted by Kevin O'Connor 4 years ago
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
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org