Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1226120 > unrolled thread

[PATCH] USB: EHCI: fix dereference of ERR_PTR

Started bySudip Mukherjee <sudipm.mukherjee@gmail.com>
First post2015-09-16 16:10 +0200
Last post2015-09-21 07:00 +0200
Articles 9 — 5 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] USB: EHCI: fix dereference of ERR_PTR Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-09-16 16:10 +0200
    Re: [PATCH] USB: EHCI: fix dereference of ERR_PTR Fabio Estevam <festevam@gmail.com> - 2015-09-16 16:20 +0200
    Re: [PATCH] USB: EHCI: fix dereference of ERR_PTR Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2015-09-16 16:30 +0200
    [PATCH v2] USB: EHCI: fix dereference of ERR_PTR Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-09-16 18:30 +0200
      Re: [PATCH v2] USB: EHCI: fix dereference of ERR_PTR Alan Stern <stern@rowland.harvard.edu> - 2015-09-16 19:00 +0200
        Re: [PATCH v2] USB: EHCI: fix dereference of ERR_PTR Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-09-18 07:50 +0200
          Re: [PATCH v2] USB: EHCI: fix dereference of ERR_PTR Alan Stern <stern@rowland.harvard.edu> - 2015-09-18 16:50 +0200
    Re: [PATCH] USB: EHCI: fix dereference of ERR_PTR "Lu, Baolu" <baolu.lu@linux.intel.com> - 2015-09-21 04:50 +0200
      Re: [PATCH] USB: EHCI: fix dereference of ERR_PTR Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-09-21 07:00 +0200

#1226120 — [PATCH] USB: EHCI: fix dereference of ERR_PTR

FromSudip Mukherjee <sudipm.mukherjee@gmail.com>
Date2015-09-16 16:10 +0200
Subject[PATCH] USB: EHCI: fix dereference of ERR_PTR
Message-ID<q9lzk-6Gb-31@gated-at.bofh.it>
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1226127

FromFabio Estevam <festevam@gmail.com>
Date2015-09-16 16:20 +0200
Message-ID<q9lJ0-6Rv-11@gated-at.bofh.it>
In reply to#1226120
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)?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1226149

FromSergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date2015-09-16 16:30 +0200
Message-ID<q9lSH-734-51@gated-at.bofh.it>
In reply to#1226120
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1226258 — [PATCH v2] USB: EHCI: fix dereference of ERR_PTR

FromSudip Mukherjee <sudipm.mukherjee@gmail.com>
Date2015-09-16 18:30 +0200
Subject[PATCH v2] USB: EHCI: fix dereference of ERR_PTR
Message-ID<q9nKR-1iB-69@gated-at.bofh.it>
In reply to#1226120
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

FromAlan Stern <stern@rowland.harvard.edu>
Date2015-09-16 19:00 +0200
SubjectRe: [PATCH v2] USB: EHCI: fix dereference of ERR_PTR
Message-ID<q9odP-1QO-13@gated-at.bofh.it>
In reply to#1226258
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

FromSudip Mukherjee <sudipm.mukherjee@gmail.com>
Date2015-09-18 07:50 +0200
SubjectRe: [PATCH v2] USB: EHCI: fix dereference of ERR_PTR
Message-ID<q9WIy-1Wn-21@gated-at.bofh.it>
In reply to#1226285
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

FromAlan Stern <stern@rowland.harvard.edu>
Date2015-09-18 16:50 +0200
SubjectRe: [PATCH v2] USB: EHCI: fix dereference of ERR_PTR
Message-ID<qa598-5y9-19@gated-at.bofh.it>
In reply to#1227577
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1229001

From"Lu, Baolu" <baolu.lu@linux.intel.com>
Date2015-09-21 04:50 +0200
Message-ID<qaZkZ-1XG-1@gated-at.bofh.it>
In reply to#1226120

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1229034

FromSudip Mukherjee <sudipm.mukherjee@gmail.com>
Date2015-09-21 07:00 +0200
Message-ID<qb1mO-4P7-5@gated-at.bofh.it>
In reply to#1229001
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web