[U-Boot] [PATCH] usb: xhci: Fix a potential NULL pointer dereference

[U-Boot] [PATCH] usb: xhci: Fix a potential NULL pointer dereference

[Sergey Temerkhanov]

This patch fixes a potential NULL pointer dereference arising on
non-present/non-initialized xHCI controllers

Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
Signed-off-by: Radha Mohan Chintakuntla <rchintakuntla@cavium.com>
---

 drivers/usb/host/xhci.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 0b09643..a6c6659 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -199,7 +199,7 @@ int xhci_reset(struct xhci_hcor *hcor)
 	int ret;
 
 	/* Halting the Host first */
-	debug("// Halt the HC\n");
+	debug("// Halt the HC: %p\n", hcor);
 	state = xhci_readl(&hcor->or_usbsts) & STS_HALT;
 	if (!state) {
 		cmd = xhci_readl(&hcor->or_usbcmd);
@@ -1093,9 +1093,11 @@ int usb_lowlevel_stop(int index)
 {
 	struct xhci_ctrl *ctrl = (xhcic + index);
 
-	xhci_lowlevel_stop(ctrl);
-	xhci_hcd_stop(index);
-	xhci_cleanup(ctrl);
+	if (ctrl->hcor) {
+		xhci_lowlevel_stop(ctrl);
+		xhci_hcd_stop(index);
+		xhci_cleanup(ctrl);
+	}
 
 	return 0;
 }
-- 
2.2.0

[U-Boot] [PATCH] usb: xhci: Fix a potential NULL pointer dereference

[Marek Vasut]

On Friday, August 14, 2015 at 02:13:06 PM, Sergey Temerkhanov wrote:
> This patch fixes a potential NULL pointer dereference arising on
> non-present/non-initialized xHCI controllers

Hi,

can you please explain how can such a condition even happen ?
I believe the hcor must always be inited at that point. What
is the condition which triggers this ?

> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> Signed-off-by: Radha Mohan Chintakuntla <rchintakuntla@cavium.com>
> ---
> 
>  drivers/usb/host/xhci.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 0b09643..a6c6659 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -199,7 +199,7 @@ int xhci_reset(struct xhci_hcor *hcor)
>  	int ret;
> 
>  	/* Halting the Host first */
> -	debug("// Halt the HC\n");
> +	debug("// Halt the HC: %p\n", hcor);
>  	state = xhci_readl(&hcor->or_usbsts) & STS_HALT;
>  	if (!state) {
>  		cmd = xhci_readl(&hcor->or_usbcmd);
> @@ -1093,9 +1093,11 @@ int usb_lowlevel_stop(int index)
>  {
>  	struct xhci_ctrl *ctrl = (xhcic + index);
> 
> -	xhci_lowlevel_stop(ctrl);
> -	xhci_hcd_stop(index);
> -	xhci_cleanup(ctrl);
> +	if (ctrl->hcor) {
> +		xhci_lowlevel_stop(ctrl);
> +		xhci_hcd_stop(index);
> +		xhci_cleanup(ctrl);
> +	}
> 
>  	return 0;
>  }

Best regards,
Marek Vasut

[U-Boot] [PATCH] usb: xhci: Fix a potential NULL pointer dereference

[Sergei Temerkhanov]

This may happen when, for example, one tries to get a single binary for
similar systems where those controllers may or may not present.

Regards,
Sergey

On Fri, Aug 14, 2015 at 3:15 PM, Marek Vasut <marex@denx.de> wrote:

> On Friday, August 14, 2015 at 02:13:06 PM, Sergey Temerkhanov wrote:
> > This patch fixes a potential NULL pointer dereference arising on
> > non-present/non-initialized xHCI controllers
>
> Hi,
>
> can you please explain how can such a condition even happen ?
> I believe the hcor must always be inited at that point. What
> is the condition which triggers this ?
>
> > Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> > Signed-off-by: Radha Mohan Chintakuntla <rchintakuntla@cavium.com>
> > ---
> >
> >  drivers/usb/host/xhci.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 0b09643..a6c6659 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -199,7 +199,7 @@ int xhci_reset(struct xhci_hcor *hcor)
> >       int ret;
> >
> >       /* Halting the Host first */
> > -     debug("// Halt the HC\n");
> > +     debug("// Halt the HC: %p\n", hcor);
> >       state = xhci_readl(&hcor->or_usbsts) & STS_HALT;
> >       if (!state) {
> >               cmd = xhci_readl(&hcor->or_usbcmd);
> > @@ -1093,9 +1093,11 @@ int usb_lowlevel_stop(int index)
> >  {
> >       struct xhci_ctrl *ctrl = (xhcic + index);
> >
> > -     xhci_lowlevel_stop(ctrl);
> > -     xhci_hcd_stop(index);
> > -     xhci_cleanup(ctrl);
> > +     if (ctrl->hcor) {
> > +             xhci_lowlevel_stop(ctrl);
> > +             xhci_hcd_stop(index);
> > +             xhci_cleanup(ctrl);
> > +     }
> >
> >       return 0;
> >  }
>
> Best regards,
> Marek Vasut
>

[U-Boot] [PATCH] usb: xhci: Fix a potential NULL pointer dereference

[Marek Vasut]

On Friday, August 14, 2015 at 03:00:31 PM, Sergei Temerkhanov wrote:
> This may happen when, for example, one tries to get a single binary for
> similar systems where those controllers may or may not present.

Please DO STOP TOP-POSTING, I mentioned it already, it is really not helpful.

I think your patch is missing one small bit in usb_lowlevel_init() in xhci.c
In case xhci_lowlevel_init() fails and thus the controller is NOT inited, the
usb_lowlevel_stop() will still try to unregister such controller, which will
likely fail.

You might want to add a check which sets the HCOR and HCCR to NULL if the
xhci_lowlevel_init() fails. What do you think ?

> Regards,
> Sergey
> 
> On Fri, Aug 14, 2015 at 3:15 PM, Marek Vasut <marex@denx.de> wrote:
> > On Friday, August 14, 2015 at 02:13:06 PM, Sergey Temerkhanov wrote:
> > > This patch fixes a potential NULL pointer dereference arising on
> > > non-present/non-initialized xHCI controllers
> > 
> > Hi,
> > 
> > can you please explain how can such a condition even happen ?
> > I believe the hcor must always be inited at that point. What
> > is the condition which triggers this ?
> > 
> > > Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
> > > Signed-off-by: Radha Mohan Chintakuntla <rchintakuntla@cavium.com>
> > > ---
> > > 
> > >  drivers/usb/host/xhci.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > > index 0b09643..a6c6659 100644
> > > --- a/drivers/usb/host/xhci.c
> > > +++ b/drivers/usb/host/xhci.c
> > > @@ -199,7 +199,7 @@ int xhci_reset(struct xhci_hcor *hcor)
> > > 
> > >       int ret;
> > >       
> > >       /* Halting the Host first */
> > > 
> > > -     debug("// Halt the HC\n");
> > > +     debug("// Halt the HC: %p\n", hcor);
> > > 
> > >       state = xhci_readl(&hcor->or_usbsts) & STS_HALT;
> > >       if (!state) {
> > >       
> > >               cmd = xhci_readl(&hcor->or_usbcmd);
> > > 
> > > @@ -1093,9 +1093,11 @@ int usb_lowlevel_stop(int index)
> > > 
> > >  {
> > >  
> > >       struct xhci_ctrl *ctrl = (xhcic + index);
> > > 
> > > -     xhci_lowlevel_stop(ctrl);
> > > -     xhci_hcd_stop(index);
> > > -     xhci_cleanup(ctrl);
> > > +     if (ctrl->hcor) {
> > > +             xhci_lowlevel_stop(ctrl);
> > > +             xhci_hcd_stop(index);
> > > +             xhci_cleanup(ctrl);
> > > +     }
> > > 
> > >       return 0;
> > >  
> > >  }
> > 
> > Best regards,
> > Marek Vasut

Best regards,
Marek Vasut

[U-Boot] [PATCH] usb: xhci: Fix a potential NULL pointer dereference

[Sergei Temerkhanov]

On Fri, Aug 14, 2015 at 4:44 PM, Marek Vasut <marex@denx.de> wrote:
> On Friday, August 14, 2015 at 03:00:31 PM, Sergei Temerkhanov wrote:
>> This may happen when, for example, one tries to get a single binary for
>> similar systems where those controllers may or may not present.
>
> Please DO STOP TOP-POSTING, I mentioned it already, it is really not helpful.
>

Sorry about that, had to struggle with Gmail on my tablet.

> I think your patch is missing one small bit in usb_lowlevel_init() in xhci.c
> In case xhci_lowlevel_init() fails and thus the controller is NOT inited, the
> usb_lowlevel_stop() will still try to unregister such controller, which will
> likely fail.
>
> You might want to add a check which sets the HCOR and HCCR to NULL if the
> xhci_lowlevel_init() fails. What do you think ?

Might be useful - it's more about error handling rather then core
configurations.
For the latter xhci_hcd_init() seems to be an appropriate place to set
hccr and hcor
to NULL.

For error handling, it might be useful to modify usb_low_level_init like this:

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a6c6659..f8e2d70 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1079,6 +1079,11 @@ int usb_lowlevel_init(int index, enum
usb_init_type init, void **controller)

        *controller = &xhcic[index];

+       if (ret) {
+               ctrl->hccr = NULL;
+               ctrl->hcor = NULL;
+       }
+
        return ret;
 }


>
>> Regards,
>> Sergey
>>
>> On Fri, Aug 14, 2015 at 3:15 PM, Marek Vasut <marex@denx.de> wrote:
>> > On Friday, August 14, 2015 at 02:13:06 PM, Sergey Temerkhanov wrote:
>> > > This patch fixes a potential NULL pointer dereference arising on
>> > > non-present/non-initialized xHCI controllers
>> >
>> > Hi,
>> >
>> > can you please explain how can such a condition even happen ?
>> > I believe the hcor must always be inited at that point. What
>> > is the condition which triggers this ?
>> >
>> > > Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>> > > Signed-off-by: Radha Mohan Chintakuntla <rchintakuntla@cavium.com>
>> > > ---
>> > >
>> > >  drivers/usb/host/xhci.c | 10 ++++++----
>> > >  1 file changed, 6 insertions(+), 4 deletions(-)
>> > >
>> > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> > > index 0b09643..a6c6659 100644
>> > > --- a/drivers/usb/host/xhci.c
>> > > +++ b/drivers/usb/host/xhci.c
>> > > @@ -199,7 +199,7 @@ int xhci_reset(struct xhci_hcor *hcor)
>> > >
>> > >       int ret;
>> > >
>> > >       /* Halting the Host first */
>> > >
>> > > -     debug("// Halt the HC\n");
>> > > +     debug("// Halt the HC: %p\n", hcor);
>> > >
>> > >       state = xhci_readl(&hcor->or_usbsts) & STS_HALT;
>> > >       if (!state) {
>> > >
>> > >               cmd = xhci_readl(&hcor->or_usbcmd);
>> > >
>> > > @@ -1093,9 +1093,11 @@ int usb_lowlevel_stop(int index)
>> > >
>> > >  {
>> > >
>> > >       struct xhci_ctrl *ctrl = (xhcic + index);
>> > >
>> > > -     xhci_lowlevel_stop(ctrl);
>> > > -     xhci_hcd_stop(index);
>> > > -     xhci_cleanup(ctrl);
>> > > +     if (ctrl->hcor) {
>> > > +             xhci_lowlevel_stop(ctrl);
>> > > +             xhci_hcd_stop(index);
>> > > +             xhci_cleanup(ctrl);
>> > > +     }
>> > >
>> > >       return 0;
>> > >
>> > >  }
>> >
>> > Best regards,
>> > Marek Vasut
>
> Best regards,
> Marek Vasut

[U-Boot] [PATCH] usb: xhci: Fix a potential NULL pointer dereference

[Marek Vasut]

On Friday, August 14, 2015 at 04:29:43 PM, Sergei Temerkhanov wrote:
> On Fri, Aug 14, 2015 at 4:44 PM, Marek Vasut <marex@denx.de> wrote:
> > On Friday, August 14, 2015 at 03:00:31 PM, Sergei Temerkhanov wrote:
> >> This may happen when, for example, one tries to get a single binary for
> >> similar systems where those controllers may or may not present.
> > 
> > Please DO STOP TOP-POSTING, I mentioned it already, it is really not
> > helpful.
> 
> Sorry about that, had to struggle with Gmail on my tablet.

Thanks :)

> > I think your patch is missing one small bit in usb_lowlevel_init() in
> > xhci.c In case xhci_lowlevel_init() fails and thus the controller is NOT
> > inited, the usb_lowlevel_stop() will still try to unregister such
> > controller, which will likely fail.
> > 
> > You might want to add a check which sets the HCOR and HCCR to NULL if the
> > xhci_lowlevel_init() fails. What do you think ?
> 
> Might be useful - it's more about error handling rather then core
> configurations.
> For the latter xhci_hcd_init() seems to be an appropriate place to set
> hccr and hcor
> to NULL.
> 
> For error handling, it might be useful to modify usb_low_level_init like
> this:
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index a6c6659..f8e2d70 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1079,6 +1079,11 @@ int usb_lowlevel_init(int index, enum
> usb_init_type init, void **controller)
> 
>         *controller = &xhcic[index];
> 
> +       if (ret) {
> +               ctrl->hccr = NULL;
> +               ctrl->hcor = NULL;
> +       }
> +
>         return ret;
>  }

You might also want to set *controller to NULL, just to be explicitly correct.
Can you please send a V2 patch ?

[U-Boot] [PATCH] usb: xhci: Fix a potential NULL pointer dereference

[Sergei Temerkhanov]

On Fri, Aug 14, 2015 at 5:45 PM, Marek Vasut <marex@denx.de> wrote:
> On Friday, August 14, 2015 at 04:29:43 PM, Sergei Temerkhanov wrote:
>> On Fri, Aug 14, 2015 at 4:44 PM, Marek Vasut <marex@denx.de> wrote:
>> > On Friday, August 14, 2015 at 03:00:31 PM, Sergei Temerkhanov wrote:
>> >> This may happen when, for example, one tries to get a single binary for
>> >> similar systems where those controllers may or may not present.
>> >
>> > Please DO STOP TOP-POSTING, I mentioned it already, it is really not
>> > helpful.
>>
>> Sorry about that, had to struggle with Gmail on my tablet.
>
> Thanks :)
>
>> > I think your patch is missing one small bit in usb_lowlevel_init() in
>> > xhci.c In case xhci_lowlevel_init() fails and thus the controller is NOT
>> > inited, the usb_lowlevel_stop() will still try to unregister such
>> > controller, which will likely fail.
>> >
>> > You might want to add a check which sets the HCOR and HCCR to NULL if the
>> > xhci_lowlevel_init() fails. What do you think ?
>>
>> Might be useful - it's more about error handling rather then core
>> configurations.
>> For the latter xhci_hcd_init() seems to be an appropriate place to set
>> hccr and hcor
>> to NULL.
>>
>> For error handling, it might be useful to modify usb_low_level_init like
>> this:
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index a6c6659..f8e2d70 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -1079,6 +1079,11 @@ int usb_lowlevel_init(int index, enum
>> usb_init_type init, void **controller)
>>
>>         *controller = &xhcic[index];
>>
>> +       if (ret) {
>> +               ctrl->hccr = NULL;
>> +               ctrl->hcor = NULL;
>> +       }
>> +
>>         return ret;
>>  }
>
> You might also want to set *controller to NULL, just to be explicitly correct.
> Can you please send a V2 patch ?

OK, in a few minutes.