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

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

[Guenter Roeck]

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.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 hw/usb/hcd-ehci.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 62dab05..c759f3e 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1348,6 +1348,11 @@ static int ehci_execute(EHCIPacket *p, const char *action)
         return -1;
     }
 
+    if (p->queue->dev == NULL) {
+        ehci_trace_guest_bug(p->queue->ehci, "No device attached to queue\n");
+        return -1;
+    }
+
     if (get_field(p->qtd.token, QTD_TOKEN_TBYTES) > BUFF_SIZE) {
         ehci_trace_guest_bug(p->queue->ehci,
                              "guest requested more bytes than allowed");
-- 
2.7.4

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

[Philippe Mathieu-Daudé]

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.

Gerd, shouldn't we call usb_packet_cleanup() in ehci_reset() rather than
ehci_finalize()? Then we shouldn't need this patch.

> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  hw/usb/hcd-ehci.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 62dab05..c759f3e 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -1348,6 +1348,11 @@ static int ehci_execute(EHCIPacket *p, const char *action)
>          return -1;
>      }
>  
> +    if (p->queue->dev == NULL) {
> +        ehci_trace_guest_bug(p->queue->ehci, "No device attached to queue\n");
> +        return -1;
> +    }
> +
>      if (get_field(p->qtd.token, QTD_TOKEN_TBYTES) > BUFF_SIZE) {
>          ehci_trace_guest_bug(p->queue->ehci,
>                               "guest requested more bytes than allowed");
> 

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

[Guenter Roeck]

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.
> 
> Gerd, shouldn't we call usb_packet_cleanup() in ehci_reset() rather than
> ehci_finalize()? Then we shouldn't need this patch.
> 


If you send me an alternate patch, I'll be happy to insert it
into my build to give it some test coverage.

Thanks,
Guenter

> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  hw/usb/hcd-ehci.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> > index 62dab05..c759f3e 100644
> > --- a/hw/usb/hcd-ehci.c
> > +++ b/hw/usb/hcd-ehci.c
> > @@ -1348,6 +1348,11 @@ static int ehci_execute(EHCIPacket *p, const char *action)
> >          return -1;
> >      }
> >  
> > +    if (p->queue->dev == NULL) {
> > +        ehci_trace_guest_bug(p->queue->ehci, "No device attached to queue\n");
> > +        return -1;
> > +    }
> > +
> >      if (get_field(p->qtd.token, QTD_TOKEN_TBYTES) > BUFF_SIZE) {
> >          ehci_trace_guest_bug(p->queue->ehci,
> >                               "guest requested more bytes than allowed");
> > 

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

[Gerd Hoffmann]

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));
+    }
 
     if (async && (q->qh.epchar & QH_EPCHAR_H)) {
 

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

[Guenter Roeck]

On Fri, Aug 02, 2019 at 04:11:49PM +0200, 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));
> +    }

I had tried that, but this does happen as standard behavior for some
architectures (I didn't write down where exactly since I thought it
must be normal). But, sure, I'll add a log message.

Guenter

>  
>      if (async && (q->qh.epchar & QH_EPCHAR_H)) {
>  

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

[Guenter Roeck]

On Fri, Aug 02, 2019 at 09:46:26AM -0700, Guenter Roeck wrote:
> On Fri, Aug 02, 2019 at 04:11:49PM +0200, 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));
> > +    }
> 
> I had tried that, but this does happen as standard behavior for some
> architectures (I didn't write down where exactly since I thought it
> must be normal). But, sure, I'll add a log message.
> 

With the log message added, I see it a lot when booting riscv64 images
from usb-ehci.

ehci_state_fetchqh: device 0 not found

It looks like this happens for each usb access (a whopping 800+ times
for a simple boot test). So it is definitely a very common condition.
The relevant qemu command line is something like

	-usb -device usb-ehci,id=ehci -device usb-storage,bus=ehci.0,drive=d0 \
	-drive file=rootfs.ext2,if=none,id=d0,format=raw

The image works fine otherwise, so I thought that the condition is normal.

Guenter

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

[Guenter Roeck]

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.

Guenter

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 <kraxel@redhat.com>
Date: Tue, 13 Aug 2019 13:37:09 +0200
Subject: [PATCH] ehci: try fix queue->dev null ptr dereference

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 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

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 <kraxel@redhat.com>
> Date: Tue, 13 Aug 2019 13:37:09 +0200
> Subject: [PATCH] ehci: try fix queue->dev null ptr dereference
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  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
> 

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

[Guenter Roeck]

On 8/13/19 4:42 AM, 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?
> 
> thanks,
>    Gerd
> 
>>From 5980eaad23f675a2d509d0c55e288793619761bc Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Tue, 13 Aug 2019 13:37:09 +0200
> Subject: [PATCH] ehci: try fix queue->dev null ptr dereference
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   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;
> 

That seems to be working as intended. I have not seen a crash
since I applied it. I tested it on top of v4.0 and v4.1.

Tested-by: Guenter Roeck <linux@roeck-us.net>

Guenter

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

[Gerd Hoffmann]

  Hi,

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

> That seems to be working as intended. I have not seen a crash
> since I applied it. I tested it on top of v4.0 and v4.1.

Thanks.  Send as formal patch now.

cheers,
  Gerd