usb: xhci: Fix a potential NULL pointer dereference

PATCH New Sergey Temerkhanov 2 participants 7 messages Aug 14, 2015

Subsystems (2)

USB USB xHCI driver

lore

Share & tools

lore

Sergey Temerkhanov

author

2015-08-14 12:13 UTC #0 lore raw

PATCH 1/1 usb: xhci: Fix a potential NULL pointer dereference

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

Signed-off-by Sergey Temerkhanov <[email protected]>

Signed-off-by Radha Mohan Chintakuntla <[email protected]>

---

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

drivers/usb/host/xhci.c +6 -4

index 0b09643..a6c6659 100644
@@ -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;
 }

Signature · 1 line

2.2.0

Sergei Temerkhanov

author

2015-08-14 13:00 UTC #2 lore raw

PATCH 1/1 usb: xhci: Fix a potential NULL pointer dereference

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

Quoted 49 lines · Marek Vasut

Quoted 2 lines · Sergey Temerkhanov
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 ?
Quoted 25 lines
Signed-off-by Sergey Temerkhanov <[email protected]>
Signed-off-by Radha Mohan Chintakuntla <[email protected]>
---
 drivers/usb/host/xhci.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/usb/host/xhci.c +1 -2
index 0b09643..a6c6659 100644
@@ -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);
drivers/usb/host/xhci.c int usb_lowlevel_stop(int index) · line 1103
Quoted 10 lines
drivers/usb/host/xhci.c +5 -2
-     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

Marek Vasut

2015-08-14 13:44 UTC #3 lore raw

PATCH 1/1 usb: xhci: Fix a potential NULL pointer dereference

Quoted 2 lines · 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.

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 ?

Quoted 60 lines

Regards,
Sergey
Quoted 56 lines · Marek Vasut
Quoted 2 lines · Sergey Temerkhanov
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 ?
Quoted 42 lines
Signed-off-by Sergey Temerkhanov <[email protected]>
Signed-off-by Radha Mohan Chintakuntla <[email protected]>
---
 drivers/usb/host/xhci.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/usb/host/xhci.c +1 -1
index 0b09643..a6c6659 100644
@@ -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);
drivers/usb/host/xhci.c +5 -3
@@ -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;
Quoted 1 line
}
Best regards,
Marek Vasut

Best regards,
Marek Vasut

Sergei Temerkhanov

author

2015-08-14 14:29 UTC #4 lore raw

PATCH 1/1 usb: xhci: Fix a potential NULL pointer dereference

Quoted 6 lines · Marek Vasut

Quoted 2 lines · 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.
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.

Quoted 7 lines

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:

drivers/usb/host/xhci.c +0 -0

index a6c6659..f8e2d70 100644
@@ -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;

}

Quoted 64 lines

Quoted 61 lines
Regards,
Sergey
Quoted 56 lines · Marek Vasut
Quoted 2 lines · Sergey Temerkhanov
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 ?
Quoted 42 lines
Signed-off-by Sergey Temerkhanov <[email protected]>
Signed-off-by Radha Mohan Chintakuntla <[email protected]>
---
 drivers/usb/host/xhci.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/usb/host/xhci.c +1 -1
index 0b09643..a6c6659 100644
@@ -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);
drivers/usb/host/xhci.c +5 -3
@@ -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;
Quoted 1 line
}
Best regards,
Marek Vasut
Best regards,
Marek Vasut

Marek Vasut

2015-08-14 14:45 UTC #5 lore raw

PATCH 1/1 usb: xhci: Fix a potential NULL pointer dereference

Quoted 9 lines · Sergei Temerkhanov

Quoted 6 lines · Marek Vasut
Quoted 2 lines · 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.
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 :)

Quoted 23 lines

Quoted 7 lines
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:
drivers/usb/host/xhci.c +0 -0
index a6c6659..f8e2d70 100644
@@ -1079,6 +1079,11 @@ int usb_lowlevel_init(int index, enum
 usb_init_type init, void **controller)

drivers/usb/host/xhci.c int usb_lowlevel_init(int index, enum · line 1089

Quoted 10 lines

drivers/usb/host/xhci.c +5 -0

 
        *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 ?

Sergei Temerkhanov

author

2015-08-14 14:52 UTC #6 lore raw

PATCH 1/1 usb: xhci: Fix a potential NULL pointer dereference

Quoted 39 lines · Marek Vasut

Quoted 9 lines · Sergei Temerkhanov
Quoted 6 lines · Marek Vasut
Quoted 2 lines · 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.
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 :)
Quoted 26 lines
Quoted 7 lines
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:
drivers/usb/host/xhci.c +0 -0
index a6c6659..f8e2d70 100644
@@ -1079,6 +1079,11 @@ int usb_lowlevel_init(int index, enum
 usb_init_type init, void **controller)
 
        *controller = &xhcic[index];
 

Quoted 10 lines

drivers/usb/host/xhci.c int usb_lowlevel_init(int index, enum · line 1089
Quoted 7 lines
drivers/usb/host/xhci.c +5 -0
+       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.