Re: [Qemu-devel] [PATCH] ehci: Ensure that device is not NULL before calling usb_ep_get

[Gerd Hoffmann]

On Tue, Aug 06, 2019 at 06:23:38AM -0700, Guenter Roeck wrote:
> On 8/2/19 7:11 AM, Gerd Hoffmann wrote:
> > On Wed, Jul 31, 2019 at 01:08:50PM +0200, Philippe Mathieu-Daudé wrote:
> > > On 7/30/19 7:45 PM, Guenter Roeck wrote:
> > > > The following assert is seen once in a while while resetting the
> > > > Linux kernel.
> > > > 
> > > > qemu-system-x86_64: hw/usb/core.c:734: usb_ep_get:
> > > >         Assertion `dev != NULL' failed.
> > > > 
> > > > The call to usb_ep_get() originates from ehci_execute().
> > > > Analysis and debugging shows that p->queue->dev can indeed be NULL
> > > > in this function. Add check for this condition and return an error
> > > > if it is seen.
> > > 
> > > Your patch is not wrong as it corrects your case, but I wonder why we
> > > get there. This assert seems to have catched a bug.
> > 
> > https://bugzilla.redhat.com//show_bug.cgi?id=1715801 maybe.
> > 
> > > Gerd, shouldn't we call usb_packet_cleanup() in ehci_reset() rather than
> > > ehci_finalize()? Then we shouldn't need this patch.
> > 
> > The two ehci_queues_rip_all() calls in ehci_reset() should clean up 
> > everything
> > properly.
> > 
> > Can you try the patch below to see whenever a ehci_find_device failure is 
> > the
> > root cause?
> > 
> > thanks,
> >    Gerd
> > 
> > diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> > index 62dab0592fa2..2b0a57772ed5 100644
> > --- a/hw/usb/hcd-ehci.c
> > +++ b/hw/usb/hcd-ehci.c
> > @@ -1644,6 +1644,10 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState 
> > *ehci, int async)
> >           q->dev = ehci_find_device(q->ehci,
> >                                     get_field(q->qh.epchar, 
> > QH_EPCHAR_DEVADDR));
> >       }
> > +    if (q->dev == NULL) {
> > +        fprintf(stderr, "%s: device %d not found\n", __func__,
> > +                get_field(q->qh.epchar, QH_EPCHAR_DEVADDR));
> > +    }
> Turns out I end up seeing that message hundreds of times early on each boot,
> no matter which architecture. It is quite obviously a normal operating 
> condition.

Yep, as long as the queue is not active this is completely harmless.
So we need to check a bit later.  In execute() looks a bit too late
though, we don't have a good backup plan then.

Does the patch below solve the problem without bad side effects?

thanks,
  Gerd

>From 5980eaad23f675a2d509d0c55e288793619761bc Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <address@hidden>
Date: Tue, 13 Aug 2019 13:37:09 +0200
Subject: [PATCH] ehci: try fix queue->dev null ptr dereference

Reported-by: Guenter Roeck <address@hidden>
Signed-off-by: Gerd Hoffmann <address@hidden>
---
 hw/usb/hcd-ehci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 62dab0592fa2..5f089f30054b 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1834,6 +1834,9 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
             ehci_set_state(q->ehci, q->async, EST_EXECUTING);
             break;
         }
+    } else if (q->dev == NULL) {
+        ehci_trace_guest_bug(q->ehci, "no device attached to queue");
+        ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
     } else {
         p = ehci_alloc_packet(q);
         p->qtdaddr = q->qtdaddr;
-- 
2.18.1