* [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1) @ 2021-08-18 12:05 Gerd Hoffmann 2021-08-18 12:05 ` [PATCH 1/1] uas: add stream number sanity checks Gerd Hoffmann ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Gerd Hoffmann @ 2021-08-18 12:05 UTC (permalink / raw) To: qemu-devel; +Cc: 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/1] uas: add stream number sanity checks. 2021-08-18 12:05 [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1) Gerd Hoffmann @ 2021-08-18 12:05 ` Gerd Hoffmann 2021-08-18 15:12 ` Philippe Mathieu-Daudé 2021-12-09 20:16 ` Guenter Roeck 2021-08-20 11:24 ` [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1) Philippe Mathieu-Daudé 2021-08-20 12:12 ` Peter Maydell 2 siblings, 2 replies; 9+ messages in thread From: Gerd Hoffmann @ 2021-08-18 12:05 UTC (permalink / raw) To: qemu-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] uas: add stream number sanity checks. 2021-08-18 12:05 ` [PATCH 1/1] uas: add stream number sanity checks Gerd Hoffmann @ 2021-08-18 15:12 ` Philippe Mathieu-Daudé 2021-12-09 20:16 ` Guenter Roeck 1 sibling, 0 replies; 9+ messages in thread From: Philippe Mathieu-Daudé @ 2021-08-18 15:12 UTC (permalink / raw) To: Gerd Hoffmann, qemu-devel; +Cc: Chen Zhe, Tan Jingguo 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] uas: add stream number sanity checks. 2021-08-18 12:05 ` [PATCH 1/1] uas: add stream number sanity checks Gerd Hoffmann 2021-08-18 15:12 ` Philippe Mathieu-Daudé @ 2021-12-09 20:16 ` Guenter Roeck 1 sibling, 0 replies; 9+ messages in thread From: Guenter Roeck @ 2021-12-09 20:16 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: qemu-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1) 2021-08-18 12:05 [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1) Gerd Hoffmann 2021-08-18 12:05 ` [PATCH 1/1] uas: add stream number sanity checks Gerd Hoffmann @ 2021-08-20 11:24 ` Philippe Mathieu-Daudé 2021-08-20 12:12 ` Peter Maydell 2 siblings, 0 replies; 9+ messages in thread From: Philippe Mathieu-Daudé @ 2021-08-20 11:24 UTC (permalink / raw) To: Gerd Hoffmann, qemu-devel, Peter Maydell 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1) 2021-08-18 12:05 [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1) Gerd Hoffmann 2021-08-18 12:05 ` [PATCH 1/1] uas: add stream number sanity checks Gerd Hoffmann 2021-08-20 11:24 ` [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1) Philippe Mathieu-Daudé @ 2021-08-20 12:12 ` Peter Maydell 2021-08-20 13:07 ` Philippe Mathieu-Daudé 2 siblings, 1 reply; 9+ messages in thread From: Peter Maydell @ 2021-08-20 12:12 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: QEMU Developers 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1) 2021-08-20 12:12 ` Peter Maydell @ 2021-08-20 13:07 ` Philippe Mathieu-Daudé 2021-08-23 9:59 ` Mauro Matteo Cascella 0 siblings, 1 reply; 9+ messages in thread From: Philippe Mathieu-Daudé @ 2021-08-20 13:07 UTC (permalink / raw) To: Peter Maydell, Gerd Hoffmann, Mauro Matteo Cascella; +Cc: QEMU Developers 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1) 2021-08-20 13:07 ` Philippe Mathieu-Daudé @ 2021-08-23 9:59 ` Mauro Matteo Cascella 2021-08-23 10:18 ` Peter Maydell 0 siblings, 1 reply; 9+ messages in thread From: Mauro Matteo Cascella @ 2021-08-23 9:59 UTC (permalink / raw) To: Philippe Mathieu-Daudé; +Cc: Peter Maydell, Gerd Hoffmann, QEMU Developers 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1) 2021-08-23 9:59 ` Mauro Matteo Cascella @ 2021-08-23 10:18 ` Peter Maydell 0 siblings, 0 replies; 9+ messages in thread From: Peter Maydell @ 2021-08-23 10:18 UTC (permalink / raw) To: Mauro Matteo Cascella Cc: Philippe Mathieu-Daudé, Gerd Hoffmann, QEMU Developers 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-12-09 20:18 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-18 12:05 [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1) Gerd Hoffmann 2021-08-18 12:05 ` [PATCH 1/1] uas: add stream number sanity checks Gerd Hoffmann 2021-08-18 15:12 ` Philippe Mathieu-Daudé 2021-12-09 20:16 ` Guenter Roeck 2021-08-20 11:24 ` [PATCH 0/1] uas: add stream number sanity checks (maybe 6.1) Philippe Mathieu-Daudé 2021-08-20 12:12 ` Peter Maydell 2021-08-20 13:07 ` Philippe Mathieu-Daudé 2021-08-23 9:59 ` Mauro Matteo Cascella 2021-08-23 10:18 ` Peter Maydell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.