[SeaBIOS] Re: Subject: [PATCH 1/2] hw/usb-hid: handle devices with illegal max packet size

[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