[PATCH] USB: EHCI: fix dereference of ERR_PTR

[PATCH] USB: EHCI: fix dereference of ERR_PTR

[Sudip Mukherjee]

On error find_tt() returns either a NULL pointer or the error value in
ERR_PTR. But we were dereferencing it directly without even checking if
find_tt() returned a valid pointer or not.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
 drivers/usb/host/ehci-sched.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index f9a3327..27bced7 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -257,6 +257,8 @@ static void reserve_release_intr_bandwidth(struct ehci_hcd *ehci,
 	/* FS/LS bus bandwidth */
 	if (tt_usecs) {
 		tt = find_tt(qh->ps.udev);
+		if (!tt || IS_ERR(tt))
+			return;
 		if (sign > 0)
 			list_add_tail(&qh->ps.ps_list, &tt->ps_list);
 		else
@@ -1373,6 +1375,8 @@ static void reserve_release_iso_bandwidth(struct ehci_hcd *ehci,
 		}
 
 		tt = find_tt(stream->ps.udev);
+		if (!tt || IS_ERR(tt))
+			return;
 		if (sign > 0)
 			list_add_tail(&stream->ps.ps_list, &tt->ps_list);
 		else
-- 
1.9.1

Re: [PATCH] USB: EHCI: fix dereference of ERR_PTR

[Fabio Estevam]

On Wed, Sep 16, 2015 at 11:08 AM, Sudip Mukherjee
<sudipm.mukherjee@gmail.com> wrote:
> On error find_tt() returns either a NULL pointer or the error value in
> ERR_PTR. But we were dereferencing it directly without even checking if
> find_tt() returned a valid pointer or not.
>
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>  drivers/usb/host/ehci-sched.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
> index f9a3327..27bced7 100644
> --- a/drivers/usb/host/ehci-sched.c
> +++ b/drivers/usb/host/ehci-sched.c
> @@ -257,6 +257,8 @@ static void reserve_release_intr_bandwidth(struct ehci_hcd *ehci,
>         /* FS/LS bus bandwidth */
>         if (tt_usecs) {
>                 tt = find_tt(qh->ps.udev);
> +               if (!tt || IS_ERR(tt))
> +                       return;

Could you use IS_ERR_OR_NULL(tt)?

Re: [PATCH] USB: EHCI: fix dereference of ERR_PTR

[Sergei Shtylyov]

Hello.

On 9/16/2015 5:08 PM, Sudip Mukherjee wrote:

> On error find_tt() returns either a NULL pointer or the error value in
> ERR_PTR. But we were dereferencing it directly without even checking if
> find_tt() returned a valid pointer or not.
>
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>   drivers/usb/host/ehci-sched.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
> index f9a3327..27bced7 100644
> --- a/drivers/usb/host/ehci-sched.c
> +++ b/drivers/usb/host/ehci-sched.c
> @@ -257,6 +257,8 @@ static void reserve_release_intr_bandwidth(struct ehci_hcd *ehci,
>   	/* FS/LS bus bandwidth */
>   	if (tt_usecs) {
>   		tt = find_tt(qh->ps.udev);
> +		if (!tt || IS_ERR(tt))

    There's IS_ERR_OR_NULL()?

> +			return;
>   		if (sign > 0)
>   			list_add_tail(&qh->ps.ps_list, &tt->ps_list);
>   		else
> @@ -1373,6 +1375,8 @@ static void reserve_release_iso_bandwidth(struct ehci_hcd *ehci,
>   		}
>
>   		tt = find_tt(stream->ps.udev);
> +		if (!tt || IS_ERR(tt))

    Likewise.

[...]

MBR, Sergei

[PATCH v2] USB: EHCI: fix dereference of ERR_PTR

[Sudip Mukherjee]

On error find_tt() returns either a NULL pointer or the error value in
ERR_PTR. But we were dereferencing it directly without even checking if
find_tt() returned a valid pointer or not.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

v2: used IS_ERR_OR_NULL (didn't know it was there. thanks)


 drivers/usb/host/ehci-sched.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index f9a3327..de75343 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -257,6 +257,8 @@ static void reserve_release_intr_bandwidth(struct ehci_hcd *ehci,
 	/* FS/LS bus bandwidth */
 	if (tt_usecs) {
 		tt = find_tt(qh->ps.udev);
+		if (IS_ERR_OR_NULL(tt))
+			return;
 		if (sign > 0)
 			list_add_tail(&qh->ps.ps_list, &tt->ps_list);
 		else
@@ -1373,6 +1375,8 @@ static void reserve_release_iso_bandwidth(struct ehci_hcd *ehci,
 		}
 
 		tt = find_tt(stream->ps.udev);
+		if (IS_ERR_OR_NULL(tt))
+			return;
 		if (sign > 0)
 			list_add_tail(&stream->ps.ps_list, &tt->ps_list);
 		else
-- 
1.9.1

Re: [PATCH v2] USB: EHCI: fix dereference of ERR_PTR

[Alan Stern]

On Wed, 16 Sep 2015, Sudip Mukherjee wrote:

> On error find_tt() returns either a NULL pointer or the error value in
> ERR_PTR. But we were dereferencing it directly without even checking if
> find_tt() returned a valid pointer or not.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
> 
> v2: used IS_ERR_OR_NULL (didn't know it was there. thanks)
> 
> 
>  drivers/usb/host/ehci-sched.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
> index f9a3327..de75343 100644
> --- a/drivers/usb/host/ehci-sched.c
> +++ b/drivers/usb/host/ehci-sched.c
> @@ -257,6 +257,8 @@ static void reserve_release_intr_bandwidth(struct ehci_hcd *ehci,
>  	/* FS/LS bus bandwidth */
>  	if (tt_usecs) {
>  		tt = find_tt(qh->ps.udev);
> +		if (IS_ERR_OR_NULL(tt))
> +			return;
>  		if (sign > 0)
>  			list_add_tail(&qh->ps.ps_list, &tt->ps_list);
>  		else
> @@ -1373,6 +1375,8 @@ static void reserve_release_iso_bandwidth(struct ehci_hcd *ehci,
>  		}
>  
>  		tt = find_tt(stream->ps.udev);
> +		if (IS_ERR_OR_NULL(tt))
> +			return;
>  		if (sign > 0)
>  			list_add_tail(&stream->ps.ps_list, &tt->ps_list);
>  		else

This patch isn't needed.  In both reserve_release_intr_bandwidth() and 
reserve_release_iso_bandwidth() it is known that find_tt() will return 
a valid pointer.

This is because each of those functions is called from only one place.  
For example, reserve_release_intr_bandwidth() is called only at the end
of qh_schedule().  But near the start of qh_schedule() there is earlier
call to tt_find(), and there we do test for error pointers.  If the
first call doesn't return an error then the second call won't either.

The same sort of thing happens in reserve_release_iso_bandwidth().

Alan Stern

Re: [PATCH v2] USB: EHCI: fix dereference of ERR_PTR

[Sudip Mukherjee]

On Wed, Sep 16, 2015 at 12:54:03PM -0400, Alan Stern wrote:
> On Wed, 16 Sep 2015, Sudip Mukherjee wrote:
> 
> > On error find_tt() returns either a NULL pointer or the error value in
> > ERR_PTR. But we were dereferencing it directly without even checking if
> > find_tt() returned a valid pointer or not.
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > ---
<snip>
> > @@ -1373,6 +1375,8 @@ static void reserve_release_iso_bandwidth(struct ehci_hcd *ehci,
> >  		}
> >  
> >  		tt = find_tt(stream->ps.udev);
> > +		if (IS_ERR_OR_NULL(tt))
> > +			return;
> >  		if (sign > 0)
> >  			list_add_tail(&stream->ps.ps_list, &tt->ps_list);
> >  		else
> 
> This patch isn't needed.  In both reserve_release_intr_bandwidth() and 
> reserve_release_iso_bandwidth() it is known that find_tt() will return 
> a valid pointer.
> 
> This is because each of those functions is called from only one place.  
> For example, reserve_release_intr_bandwidth() is called only at the end
> of qh_schedule().  But near the start of qh_schedule() there is earlier
> call to tt_find(), and there we do test for error pointers.  If the
> first call doesn't return an error then the second call won't either.
> 
> The same sort of thing happens in reserve_release_iso_bandwidth().
Yes, I should have looked more before sending. Sorry for the noise.
But in those checkes for find_tt() only IS_ERR is checked, shouldn't we
check for IS_ERR_OR_NULL as find_tt() can return NULL also?

regards
sudip

Re: [PATCH v2] USB: EHCI: fix dereference of ERR_PTR

[Alan Stern]

On Fri, 18 Sep 2015, Sudip Mukherjee wrote:

> On Wed, Sep 16, 2015 at 12:54:03PM -0400, Alan Stern wrote:
> > On Wed, 16 Sep 2015, Sudip Mukherjee wrote:
> > 
> > > On error find_tt() returns either a NULL pointer or the error value in
> > > ERR_PTR. But we were dereferencing it directly without even checking if
> > > find_tt() returned a valid pointer or not.
> > > 
> > > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > > ---
> <snip>
> > > @@ -1373,6 +1375,8 @@ static void reserve_release_iso_bandwidth(struct ehci_hcd *ehci,
> > >  		}
> > >  
> > >  		tt = find_tt(stream->ps.udev);
> > > +		if (IS_ERR_OR_NULL(tt))
> > > +			return;
> > >  		if (sign > 0)
> > >  			list_add_tail(&stream->ps.ps_list, &tt->ps_list);
> > >  		else
> > 
> > This patch isn't needed.  In both reserve_release_intr_bandwidth() and 
> > reserve_release_iso_bandwidth() it is known that find_tt() will return 
> > a valid pointer.
> > 
> > This is because each of those functions is called from only one place.  
> > For example, reserve_release_intr_bandwidth() is called only at the end
> > of qh_schedule().  But near the start of qh_schedule() there is earlier
> > call to tt_find(), and there we do test for error pointers.  If the
> > first call doesn't return an error then the second call won't either.
> > 
> > The same sort of thing happens in reserve_release_iso_bandwidth().
> Yes, I should have looked more before sending. Sorry for the noise.
> But in those checkes for find_tt() only IS_ERR is checked, shouldn't we
> check for IS_ERR_OR_NULL as find_tt() can return NULL also?

I knew someone would ask about that!  :-)

The NULL case is similar to the ERR_PTR case, but more complicated.  
Basically, find_tt() returns NULL only when the device doesn't lie 
below a TT -- all the other invalid returns are ERR_PTRs.

In reserve_release_intr_bandwidth(), for instance, the call to 
find_tt() occurs only if tt_usecs = qh->ps.tt_usecs is nonzero.  This 
value is guaranteed to be 0 if the device doesn't run at low speed or 
full speed -- see qh_make() in ehci-q.c, where qh->ps.tt_usecs is 
initialized only when urb->dev->speed != USB_SPEED_HIGH.

To see that udev->tt is non-NULL whenever the speed isn't 
USB_SPEED_HIGH, you have to look through the hub driver code.  The 
relevant routine is hub_port_init() in core/hub.c, the section headed 
by the comment:

	/* Set up TT records, if needed  */

Similar reasoning applies to reserve_release_iso_bandwidth(); here the 
condition is that stream->splits is nonzero, which is true only if the 
device is full speed (see iso_stream_init()).

Alan Stern

Re: [PATCH] USB: EHCI: fix dereference of ERR_PTR

[Lu, Baolu]

On 09/16/2015 10:08 PM, Sudip Mukherjee wrote:
> On error find_tt() returns either a NULL pointer or the error value in
> ERR_PTR. But we were dereferencing it directly without even checking if
> find_tt() returned a valid pointer or not.
>
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>   drivers/usb/host/ehci-sched.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
> index f9a3327..27bced7 100644
> --- a/drivers/usb/host/ehci-sched.c
> +++ b/drivers/usb/host/ehci-sched.c
> @@ -257,6 +257,8 @@ static void reserve_release_intr_bandwidth(struct ehci_hcd *ehci,
>   	/* FS/LS bus bandwidth */
>   	if (tt_usecs) {
>   		tt = find_tt(qh->ps.udev);
> +		if (!tt || IS_ERR(tt))

Why not IS_ERR_OR_NULL()?

> +			return;
>   		if (sign > 0)
>   			list_add_tail(&qh->ps.ps_list, &tt->ps_list);
>   		else
> @@ -1373,6 +1375,8 @@ static void reserve_release_iso_bandwidth(struct ehci_hcd *ehci,
>   		}
>   
>   		tt = find_tt(stream->ps.udev);
> +		if (!tt || IS_ERR(tt))
> +			return;
>   		if (sign > 0)
>   			list_add_tail(&stream->ps.ps_list, &tt->ps_list);
>   		else

Re: [PATCH] USB: EHCI: fix dereference of ERR_PTR

[Sudip Mukherjee]

On Mon, Sep 21, 2015 at 10:48:52AM +0800, Lu, Baolu wrote:
> 
> 
> On 09/16/2015 10:08 PM, Sudip Mukherjee wrote:
> >On error find_tt() returns either a NULL pointer or the error value in
> >ERR_PTR. But we were dereferencing it directly without even checking if
> >find_tt() returned a valid pointer or not.
> >
> >Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> >---
> >  drivers/usb/host/ehci-sched.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> >diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
> >index f9a3327..27bced7 100644
> >--- a/drivers/usb/host/ehci-sched.c
> >+++ b/drivers/usb/host/ehci-sched.c
> >@@ -257,6 +257,8 @@ static void reserve_release_intr_bandwidth(struct ehci_hcd *ehci,
> >  	/* FS/LS bus bandwidth */
> >  	if (tt_usecs) {
> >  		tt = find_tt(qh->ps.udev);
> >+		if (!tt || IS_ERR(tt))
> 
> Why not IS_ERR_OR_NULL()?
This was v1, corrected in v2. And Alan has already explained why this
patch is not required.

regards
sudip