| Message ID | 20210818120505.1258262-2-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | uas: add stream number sanity checks (maybe 6.1) | expand |
On 8/18/21 2:05 PM, Gerd Hoffmann wrote: > The device uses the guest-supplied stream number unchecked, which can > lead to guest-triggered out-of-band access to the UASDevice->data3 and > UASDevice->status3 fields. Add the missing checks. > > Fixes: CVE-2021-3713 Reported-by: Chen Zhe <chenzhe@huawei.com> Reported-by: Tan Jingguo <tanjingguo@huawei.com> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/usb/dev-uas.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c > index 263056231c79..f6309a5ebfdc 100644 > --- a/hw/usb/dev-uas.c > +++ b/hw/usb/dev-uas.c > @@ -840,6 +840,9 @@ static void usb_uas_handle_data(USBDevice *dev, USBPacket *p) > } > break; > case UAS_PIPE_ID_STATUS: > + if (p->stream > UAS_MAX_STREAMS) { > + goto err_stream; > + } > if (p->stream) { > QTAILQ_FOREACH(st, &uas->results, next) { > if (st->stream == p->stream) { > @@ -867,6 +870,9 @@ static void usb_uas_handle_data(USBDevice *dev, USBPacket *p) > break; > case UAS_PIPE_ID_DATA_IN: > case UAS_PIPE_ID_DATA_OUT: > + if (p->stream > UAS_MAX_STREAMS) { > + goto err_stream; > + } > if (p->stream) { > req = usb_uas_find_request(uas, p->stream); > } else { > @@ -902,6 +908,11 @@ static void usb_uas_handle_data(USBDevice *dev, USBPacket *p) > p->status = USB_RET_STALL; > break; > } > + > +err_stream: > + error_report("%s: invalid stream %d", __func__, p->stream); > + p->status = USB_RET_STALL; > + return; > } Thanks for tackling this, I was looking but thought it was too late for 6.1. Beside your obvious checks, I'd also modify usb_uas_alloc_status() to feel safer, by checking 'tag'. Eventually returning NULL and adapt the call sites? Anyway this can wait 6.2, so for this patch: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On Wed, Aug 18, 2021 at 02:05:05PM +0200, Gerd Hoffmann wrote: > The device uses the guest-supplied stream number unchecked, which can > lead to guest-triggered out-of-band access to the UASDevice->data3 and > UASDevice->status3 fields. Add the missing checks. > > Fixes: CVE-2021-3713 > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > Reported-by: Chen Zhe <chenzhe@huawei.com> > Reported-by: Tan Jingguo <tanjingguo@huawei.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/usb/dev-uas.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c > index 263056231c79..f6309a5ebfdc 100644 > --- a/hw/usb/dev-uas.c > +++ b/hw/usb/dev-uas.c > @@ -840,6 +840,9 @@ static void usb_uas_handle_data(USBDevice *dev, USBPacket *p) > } > break; > case UAS_PIPE_ID_STATUS: > + if (p->stream > UAS_MAX_STREAMS) { > + goto err_stream; > + } > if (p->stream) { > QTAILQ_FOREACH(st, &uas->results, next) { > if (st->stream == p->stream) { > @@ -867,6 +870,9 @@ static void usb_uas_handle_data(USBDevice *dev, USBPacket *p) > break; > case UAS_PIPE_ID_DATA_IN: > case UAS_PIPE_ID_DATA_OUT: > + if (p->stream > UAS_MAX_STREAMS) { > + goto err_stream; > + } > if (p->stream) { > req = usb_uas_find_request(uas, p->stream); > } else { > @@ -902,6 +908,11 @@ static void usb_uas_handle_data(USBDevice *dev, USBPacket *p) > p->status = USB_RET_STALL; > break; > } > + > +err_stream: > + error_report("%s: invalid stream %d", __func__, p->stream); > + p->status = USB_RET_STALL; > + return; How is this supposed to work ? It results in messages such as the following. qemu-system-sparc64: usb_uas_handle_data: invalid stream 1 qemu-system-sparc64: usb_uas_handle_data: invalid stream 1 It also sets the status unconditionally to USB_RET_STALL, and UAS is simply broken after this patch is applied because the error handling code is executed literally for each call of usb_uas_handle_data(). Guenter
diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c index 263056231c79..f6309a5ebfdc 100644 --- a/hw/usb/dev-uas.c +++ b/hw/usb/dev-uas.c @@ -840,6 +840,9 @@ static void usb_uas_handle_data(USBDevice *dev, USBPacket *p) } break; case UAS_PIPE_ID_STATUS: + if (p->stream > UAS_MAX_STREAMS) { + goto err_stream; + } if (p->stream) { QTAILQ_FOREACH(st, &uas->results, next) { if (st->stream == p->stream) { @@ -867,6 +870,9 @@ static void usb_uas_handle_data(USBDevice *dev, USBPacket *p) break; case UAS_PIPE_ID_DATA_IN: case UAS_PIPE_ID_DATA_OUT: + if (p->stream > UAS_MAX_STREAMS) { + goto err_stream; + } if (p->stream) { req = usb_uas_find_request(uas, p->stream); } else { @@ -902,6 +908,11 @@ static void usb_uas_handle_data(USBDevice *dev, USBPacket *p) p->status = USB_RET_STALL; break; } + +err_stream: + error_report("%s: invalid stream %d", __func__, p->stream); + p->status = USB_RET_STALL; + return; } static void usb_uas_unrealize(USBDevice *dev)
The device uses the guest-supplied stream number unchecked, which can lead to guest-triggered out-of-band access to the UASDevice->data3 and UASDevice->status3 fields. Add the missing checks. Fixes: CVE-2021-3713 Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/usb/dev-uas.c | 11 +++++++++++ 1 file changed, 11 insertions(+)