| Message ID | 20201012150356.79670-1-mcascell@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/usb/hcd-dwc2: fix divide-by-zero in dwc2_handle_packet() | expand |
On Mon, Oct 12, 2020 at 8:05 AM Mauro Matteo Cascella <mcascell@redhat.com> wrote: > > Check the value of mps before it is used as divisor. Since HCCHAR_MPS is guest > controllable, this prevents a malicious/buggy guest from crashing the QEMU > process on the host. > > Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com> > Reported-by: Gaoning Pan <gaoning.pgn@antgroup.com> > Reported-by: Xingwei Lin <linyi.lxw@antfin.com> > --- > hw/usb/hcd-dwc2.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/usb/hcd-dwc2.c b/hw/usb/hcd-dwc2.c > index 97688d21bf..91476fd781 100644 > --- a/hw/usb/hcd-dwc2.c > +++ b/hw/usb/hcd-dwc2.c > @@ -324,6 +324,12 @@ babble: > } > } > > + if (mps == 0) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "%s: Bad HCCHAR_MPS set to zero\n", __func__); > + return; > + } > + > tpcnt = actual / mps; > if (actual % mps) { > tpcnt++; > -- > 2.26.2 > Hi Mauro, I think it would be better to move this check earlier in the function, just after 'mps' is read from the register. Otherwise it can get assigned to 'tlen' and 'p->mps', and who knows what mischief an invalid value there might cause. After that, you can add my Reviewed-by: Paul Zimmerman <pauldzim@gmail.com> Thanks, Paul
> > + if (mps == 0) { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "%s: Bad HCCHAR_MPS set to zero\n", __func__); > > + return; > > + } > I think it would be better to move this check earlier in the function, > just after 'mps' is read from the register. Otherwise it can get > assigned to 'tlen' and 'p->mps', and who knows what mischief an > invalid value there might cause. Makes sense. While being at it maybe handle len > DWC2_MAX_XFER_SIZE the same way, the assert looks like it can be triggered by the guest. Also: What would be the effect of simply returning here? Would dwc2 emulation simply stop processing queues? Should we maybe raise an error IRQ? What will real dwc2 hardware do in this case? take care, Gerd
Hi Gerd, On Tue, Oct 13, 2020 at 12:04 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > + if (mps == 0) { > > > + qemu_log_mask(LOG_GUEST_ERROR, > > > + "%s: Bad HCCHAR_MPS set to zero\n", __func__); > > > + return; > > > + } > > > I think it would be better to move this check earlier in the function, > > just after 'mps' is read from the register. Otherwise it can get > > assigned to 'tlen' and 'p->mps', and who knows what mischief an > > invalid value there might cause. > > Makes sense. While being at it maybe handle len > DWC2_MAX_XFER_SIZE > the same way, the assert looks like it can be triggered by the guest. I sent you a patch to fix up several assert()s, including that one, about a month ago. Did you miss it? https://lore.kernel.org/qemu-devel/20200920021449.830-1-pauldzim@gmail.com > Also: What would be the effect of simply returning here? Would dwc2 > emulation simply stop processing queues? Should we maybe raise an > error IRQ? Not entirely sure, I imagine the emulation will just stop working. I can test it tomorrow. Also, can you give me a hint what an error IRQ is? Is that a Qemu thing, or do you mean we should emulate what the real core does in this case? > What will real dwc2 hardware do in this case? No idea. I don't think it's mentioned in the manual. -Paul > take care, > Gerd >
On Tue, Oct 13, 2020 at 12:19:40AM -0700, Paul Zimmerman wrote: > I sent you a patch to fix up several assert()s, including that one, about a > month ago. Did you miss it? > https://lore.kernel.org/qemu-devel/20200920021449.830-1-pauldzim@gmail.com I'll check. There is a bunch of pending stuff in my qemu-patches mailbox I'll try to process this week ... > > Also: What would be the effect of simply returning here? Would dwc2 > > emulation simply stop processing queues? Should we maybe raise an > > error IRQ? > > Not entirely sure, I imagine the emulation will just stop working. I can > test it tomorrow. Also, can you give me a hint what an error IRQ is? > Is that a Qemu thing, or do you mean we should emulate what the > real core does in this case? Same real hardware does. ehci for example has the USBSTS_ERRINT bit in the IRQ status register to signal errors. take care, Gerd
Hi, > I sent you a patch to fix up several assert()s, including that one, about a > month ago. Did you miss it? > https://lore.kernel.org/qemu-devel/20200920021449.830-1-pauldzim@gmail.com Seems I missed that, or deleted by accident. Added to qemu queue now. thanks, Gerd
On Tue, Oct 13, 2020 at 10:41 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > On Tue, Oct 13, 2020 at 12:19:40AM -0700, Paul Zimmerman wrote: > > I sent you a patch to fix up several assert()s, including that one, about a > > month ago. Did you miss it? > > https://lore.kernel.org/qemu-devel/20200920021449.830-1-pauldzim@gmail.com > > I'll check. There is a bunch of pending stuff in my qemu-patches > mailbox I'll try to process this week ... > > > > Also: What would be the effect of simply returning here? Would dwc2 > > > emulation simply stop processing queues? Should we maybe raise an > > > error IRQ? > > > > Not entirely sure, I imagine the emulation will just stop working. I can > > test it tomorrow. Also, can you give me a hint what an error IRQ is? > > Is that a Qemu thing, or do you mean we should emulate what the > > real core does in this case? > > Same real hardware does. ehci for example has the USBSTS_ERRINT bit in > the IRQ status register to signal errors. > > take care, > Gerd > I'll send a new version of the patch with the check moved earlier in the function, as suggested by Paul. If raising an error turns out to be the right thing to do, I think other checks may need to be updated in addition to this one. Hence we can possibly address that in a later patch. Thanks a lot for your comments.
On Wed, Oct 14, 2020 at 12:55 PM Mauro Matteo Cascella <mcascell@redhat.com> wrote: > > On Tue, Oct 13, 2020 at 10:41 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > > > Also: What would be the effect of simply returning here? Would dwc2 > > > > emulation simply stop processing queues? Should we maybe raise an > > > > error IRQ? > > > > > > Not entirely sure, I imagine the emulation will just stop working. I can > > > test it tomorrow. Also, can you give me a hint what an error IRQ is? > > > Is that a Qemu thing, or do you mean we should emulate what the > > > real core does in this case? > > > > Same real hardware does. ehci for example has the USBSTS_ERRINT bit in > > the IRQ status register to signal errors. So I tested this patch (modified as I suggested) in two ways. First, I hacked up the dwc_otg driver in the Raspbian kernel to always set the 'mps' field to zero. Then I booted the 2019-09-26-raspbian-buster Raspbian image with 4 usb devices, keyboard, tablet, storage, and network. In this test, the first USB device failed to enumerate, giving the kernel error message "usb 1-1: device descriptor read/64, error -110". The kernel retried the enumeration several times, finally giving the message "usb usb1-port1: unable to enumerate usb device". After that, there was no more USB activity shown in the kernel log. Otherwise the kernel seemed to keep running normally and the raspi emulation continued to run. In the second test, I changed the hack to only zero the 'mps' field for bulk transfers. In that case, the non-bulk devices (keyboard, tablet) continued to operate normally. The bulk devices (storage, network) enumerated fine, but after that they failed to work, with various error messages in the kernel log. Again the kernel seemed to keep running normally and the raspi emulation continued to run. So I think the patch works fine, and I don't think we need to do anything fancier. - Paul > > > > take care, > > Gerd > > > > I'll send a new version of the patch with the check moved earlier in > the function, as suggested by Paul. If raising an error turns out to > be the right thing to do, I think other checks may need to be updated > in addition to this one. Hence we can possibly address that in a later > patch. Thanks a lot for your comments. > > -- > Mauro Matteo Cascella, Red Hat Product Security > 6F78 E20B 5935 928C F0A8 1A9D 4E55 23B8 BB34 10B0 >
Hi, > So I think the patch works fine, and I don't think we need to do > anything fancier. Cool, thanks for checking. take care, Gerd
diff --git a/hw/usb/hcd-dwc2.c b/hw/usb/hcd-dwc2.c index 97688d21bf..91476fd781 100644 --- a/hw/usb/hcd-dwc2.c +++ b/hw/usb/hcd-dwc2.c @@ -324,6 +324,12 @@ babble: } } + if (mps == 0) { + qemu_log_mask(LOG_GUEST_ERROR, + "%s: Bad HCCHAR_MPS set to zero\n", __func__); + return; + } + tpcnt = actual / mps; if (actual % mps) { tpcnt++;
Check the value of mps before it is used as divisor. Since HCCHAR_MPS is guest controllable, this prevents a malicious/buggy guest from crashing the QEMU process on the host. Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com> Reported-by: Gaoning Pan <gaoning.pgn@antgroup.com> Reported-by: Xingwei Lin <linyi.lxw@antfin.com> --- hw/usb/hcd-dwc2.c | 6 ++++++ 1 file changed, 6 insertions(+)