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

[Guenter Roeck]

On Tue, Aug 13, 2019 at 01:42:03PM +0200, Gerd Hoffmann wrote:
> 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?
> 
I reverted my patch and applied the patch below to my builds of qemu, for both
v4.0 and v4.1, and installed it in my test bed. I'll let you know how it goes.

Thanks,
Guenter

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