| Message ID | Z4TfMOrZz6IQYl_h@Sun (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/usb/canokey: Fix buffer overflow for OUT packet | expand |
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 >
On Mon, 27 Jan 2025 at 14:48, Hongren Zheng <i@zenithal.me> wrote: > > 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. I can pick this up; I'm not super familiar with either our USB code or the canokey device, but the change looks OK to me, and I assume you've tested it. Reviewed-by: Peter Maydell <peter.maydell@linaro.org> and I'll put it into my upcoming target-arm pullreq. thanks -- PMM
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 */
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(-)