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

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

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.

-- 
Mauro Matteo Cascella, Red Hat Product Security
6F78 E20B 5935 928C F0A8  1A9D 4E55 23B8 BB34 10B0

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

  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