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

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

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.