[PATCH] usb: chipidea: udc: fix NULL ptr dereference in isr_setup_status_phase

[PATCH] usb: chipidea: udc: fix NULL ptr dereference in isr_setup_status_phase

[Clemens Gruber]

Problems with the signal integrity of the high speed USB data lines or
noise on reference ground lines can cause the i.MX6 USB controller to
violate USB specs and exhibit unexpected behavior.

It was observed that USBi_UI interrupts were triggered first and when
isr_setup_status_phase was called, ci->status was NULL, which lead to a
NULL pointer dereference kernel panic.

This patch fixes the kernel panic, emits a warning once and returns
-EPIPE to halt the device and let the host get stalled.
It also adds a comment to point people, who are experiencing this issue,
to their USB hardware design.

Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
 drivers/usb/chipidea/udc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index dfec5a1..b933568 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -949,6 +949,15 @@ static int isr_setup_status_phase(struct ci_hdrc *ci)
 	int retval;
 	struct ci_hw_ep *hwep;
 
+	/*
+	 * Unexpected USB controller behavior, caused by bad signal integrity
+	 * or ground reference problems, can lead to isr_setup_status_phase
+	 * being called with ci->status equal to NULL.
+	 * If this situation occurs, you should review your USB hardware design.
+	 */
+	if (WARN_ON_ONCE(!ci->status))
+		return -EPIPE;
+
 	hwep = (ci->ep0_dir == TX) ? ci->ep0out : ci->ep0in;
 	ci->status->context = ci;
 	ci->status->complete = isr_setup_status_complete;
-- 
2.9.3

Re: [PATCH] usb: chipidea: udc: fix NULL ptr dereference in isr_setup_status_phase

[Peter Chen]

On Mon, Sep 05, 2016 at 07:29:58PM +0200, Clemens Gruber wrote:
> Problems with the signal integrity of the high speed USB data lines or
> noise on reference ground lines can cause the i.MX6 USB controller to
> violate USB specs and exhibit unexpected behavior.
> 
> It was observed that USBi_UI interrupts were triggered first and when
> isr_setup_status_phase was called, ci->status was NULL, which lead to a
> NULL pointer dereference kernel panic.
> 
> This patch fixes the kernel panic, emits a warning once and returns
> -EPIPE to halt the device and let the host get stalled.
> It also adds a comment to point people, who are experiencing this issue,
> to their USB hardware design.
> 
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
>  drivers/usb/chipidea/udc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index dfec5a1..b933568 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -949,6 +949,15 @@ static int isr_setup_status_phase(struct ci_hdrc *ci)
>  	int retval;
>  	struct ci_hw_ep *hwep;
>  
> +	/*
> +	 * Unexpected USB controller behavior, caused by bad signal integrity
> +	 * or ground reference problems, can lead to isr_setup_status_phase
> +	 * being called with ci->status equal to NULL.
> +	 * If this situation occurs, you should review your USB hardware design.
> +	 */
> +	if (WARN_ON_ONCE(!ci->status))
> +		return -EPIPE;
> +
>  	hwep = (ci->ep0_dir == TX) ? ci->ep0out : ci->ep0in;
>  	ci->status->context = ci;
>  	ci->status->complete = isr_setup_status_complete;

Thanks, I will apply it.

-- 

Best Regards,
Peter Chen