Re: [PATCH 1/1] uas: add stream number sanity checks.

[Guenter Roeck]

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