Re: [PATCH] hw/usb/canokey: Fix buffer overflow for OUT packet

[Hongren Zheng]

On Mon, Jan 13, 2025 at 05:38:56PM +0800, Hongren Zheng wrote:
> When USBPacket in OUT direction has larger payload
> than the ep_out_buffer (of size 512), a buffer overflow
> would occur.
> 
> It could be fixed by limiting the size of usb_packet_copy
> to be at most buffer size. Further optimization gets rid
> of the ep_out_buffer and directly uses ep_out as the target
> buffer.
> 
> This is reported by a security researcher who artificially
> constructed an OUT packet of size 2047. The report has gone
> through the QEMU security process, and as this device is for
> testing purpose and no deployment of it in virtualization
> environment is observed, it is triaged not to be a security bug.
> 
> Reported-by: Juan Jose Lopez Jaimez <thatjiaozi@gmail.com>
> Signed-off-by: Hongren Zheng <i@zenithal.me>
> ---
>  hw/usb/canokey.c | 6 +++---
>  hw/usb/canokey.h | 4 ----
>  2 files changed, 3 insertions(+), 7 deletions(-)

Seems that the USB subsystem has been orphaned
and there is no maintainer now.

I used to ask the USB maintainer to pass the patch
because I could not send a PULL, which requires
gpg signature.

I wonder which maintainer I should ask for now.

> 
> diff --git a/hw/usb/canokey.c b/hw/usb/canokey.c
> index fae212f053..e2d66179e0 100644
> --- a/hw/usb/canokey.c
> +++ b/hw/usb/canokey.c
> @@ -197,8 +197,8 @@ static void canokey_handle_data(USBDevice *dev, USBPacket 
> *p)
>      switch (p->pid) {
>      case USB_TOKEN_OUT:
>          trace_canokey_handle_data_out(ep_out, p->iov.size);
> -        usb_packet_copy(p, key->ep_out_buffer[ep_out], p->iov.size);
>          out_pos = 0;
> +        /* segment packet into (possibly multiple) ep_out */
>          while (out_pos != p->iov.size) {
>              /*
>               * key->ep_out[ep_out] set by prepare_receive
> @@ -207,8 +207,8 @@ static void canokey_handle_data(USBDevice *dev, USBPacket 
> *p)
>               * to be the buffer length
>               */
>              out_len = MIN(p->iov.size - out_pos, key->ep_out_size[ep_out]);
> -            memcpy(key->ep_out[ep_out],
> -                    key->ep_out_buffer[ep_out] + out_pos, out_len);
> +            /* usb_packet_copy would update the pos offset internally */
> +            usb_packet_copy(p, key->ep_out[ep_out], out_len);
>              out_pos += out_len;
>              /* update ep_out_size to actual len */
>              key->ep_out_size[ep_out] = out_len;
> diff --git a/hw/usb/canokey.h b/hw/usb/canokey.h
> index e528889d33..1b60d73485 100644
> --- a/hw/usb/canokey.h
> +++ b/hw/usb/canokey.h
> @@ -24,8 +24,6 @@
>  #define CANOKEY_EP_NUM 3
>  /* BULK/INTR IN can be up to 1352 bytes, e.g. get key info */
>  #define CANOKEY_EP_IN_BUFFER_SIZE 2048
> -/* BULK OUT can be up to 270 bytes, e.g. PIV import cert */
> -#define CANOKEY_EP_OUT_BUFFER_SIZE 512
>  
>  typedef enum {
>      CANOKEY_EP_IN_WAIT,
> @@ -59,8 +57,6 @@ typedef struct CanoKeyState {
>      /* OUT pointer to canokey recv buffer */
>      uint8_t *ep_out[CANOKEY_EP_NUM];
>      uint32_t ep_out_size[CANOKEY_EP_NUM];
> -    /* For large BULK OUT, multiple write to ep_out is needed */
> -    uint8_t ep_out_buffer[CANOKEY_EP_NUM][CANOKEY_EP_OUT_BUFFER_SIZE];
>  
>      /* Properties */
>      char *file; /* canokey-file */
> -- 
> 2.47.1
>