[PATCH 0/1] uas: add stream number sanity checks (maybe 6.1)

[PATCH 0/1] uas: add stream number sanity checks (maybe 6.1)

[Gerd Hoffmann]

Security fix.  Sorry for the last-minute patch, I had completely
forgotten this one until the CVE number for it arrived today.

Given that the classic usb storage device is way more popular than
the uas (usb attached scsi) device the impact should be pretty low
and we might consider to not screw up our release schedule for this.

take care,
  Gerd

Gerd Hoffmann (1):
  uas: add stream number sanity checks.

 hw/usb/dev-uas.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

-- 
2.31.1

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

[Gerd Hoffmann]

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(+)

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)
-- 
2.31.1

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

[Philippe Mathieu-Daudé]

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>

Re: [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1)

[Philippe Mathieu-Daudé]

On 8/18/21 2:05 PM, Gerd Hoffmann wrote:
> Security fix.  Sorry for the last-minute patch, I had completely
> forgotten this one until the CVE number for it arrived today.
> 
> Given that the classic usb storage device is way more popular than
> the uas (usb attached scsi) device the impact should be pretty low
> and we might consider to not screw up our release schedule for this.

So 6.1-rc5 or stable?

Re: [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1)

[Peter Maydell]

On Wed, 18 Aug 2021 at 13:10, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Security fix.  Sorry for the last-minute patch, I had completely
> forgotten this one until the CVE number for it arrived today.
>
> Given that the classic usb storage device is way more popular than
> the uas (usb attached scsi) device the impact should be pretty low
> and we might consider to not screw up our release schedule for this.

What's the impact if the bug is exploited ?

-- PMM

Re: [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1)

[Philippe Mathieu-Daudé]

Cc'ing Mauro to double-check.

On 8/20/21 2:12 PM, Peter Maydell wrote:
> On Wed, 18 Aug 2021 at 13:10, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>
>> Security fix.  Sorry for the last-minute patch, I had completely
>> forgotten this one until the CVE number for it arrived today.
>>
>> Given that the classic usb storage device is way more popular than
>> the uas (usb attached scsi) device the impact should be pretty low
>> and we might consider to not screw up our release schedule for this.
> 
> What's the impact if the bug is exploited ?

Bug class: "guest-triggered user-after-free".

Being privileged (root) in the guest, you can leak some data from
the host process then DoS the host or potentially exploit the
use-after-free to execute code on the host.

Re: [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1)

[Mauro Matteo Cascella]

Hi,

On Fri, Aug 20, 2021 at 3:07 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Cc'ing Mauro to double-check.
>
> On 8/20/21 2:12 PM, Peter Maydell wrote:
> > On Wed, 18 Aug 2021 at 13:10, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >>
> >> Security fix.  Sorry for the last-minute patch, I had completely
> >> forgotten this one until the CVE number for it arrived today.
> >>
> >> Given that the classic usb storage device is way more popular than
> >> the uas (usb attached scsi) device the impact should be pretty low
> >> and we might consider to not screw up our release schedule for this.
> >
> > What's the impact if the bug is exploited ?
>
> Bug class: "guest-triggered user-after-free".
>
> Being privileged (root) in the guest, you can leak some data from
> the host process then DoS the host or potentially exploit the
> use-after-free to execute code on the host.
>

This is actually an out-of-bounds access issue (not UAF). It's still
potentially bad, but I agree with Gerd the impact is low. Plus there's
an assert right before [1] that makes it a DoS if the accessed memory
is not NULL.

[1] https://gitlab.com/qemu-project/qemu/-/blob/master/hw/usb/dev-uas.c#L850

Regards.
--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

Re: [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1)

[Peter Maydell]

On Mon, 23 Aug 2021 at 10:59, Mauro Matteo Cascella <mcascell@redhat.com> wrote:
>
> Hi,
>
> On Fri, Aug 20, 2021 at 3:07 PM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
> >
> > Cc'ing Mauro to double-check.
> >
> > On 8/20/21 2:12 PM, Peter Maydell wrote:
> > > On Wed, 18 Aug 2021 at 13:10, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >>
> > >> Security fix.  Sorry for the last-minute patch, I had completely
> > >> forgotten this one until the CVE number for it arrived today.
> > >>
> > >> Given that the classic usb storage device is way more popular than
> > >> the uas (usb attached scsi) device the impact should be pretty low
> > >> and we might consider to not screw up our release schedule for this.
> > >
> > > What's the impact if the bug is exploited ?
> >
> > Bug class: "guest-triggered user-after-free".
> >
> > Being privileged (root) in the guest, you can leak some data from
> > the host process then DoS the host or potentially exploit the
> > use-after-free to execute code on the host.
> >
>
> This is actually an out-of-bounds access issue (not UAF). It's still
> potentially bad, but I agree with Gerd the impact is low. Plus there's
> an assert right before [1] that makes it a DoS if the accessed memory
> is not NULL.

Thanks. OK, (and following discussion of this on irc on Friday)
we won't put this fix into 6.1. (That is, we treat it the same way
we would if the CVE patch had arrived the day after we tagged 6.1,
ie distros and other interested parties pick up the patch as they
would any other security fix.)

-- PMM

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