usb: xhci: Fix a potential NULL pointer dereference

PATCH Superseded Sergey Temerkhanov 2 participants 6 messages Aug 14, 2015 – Aug 18, 2015

Subsystems (2)

USB USB xHCI driver

lore

Share & tools

lore

Sergey Temerkhanov

author

2015-08-14 15:14 UTC #0 lore raw

PATCH v2 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 and adds some error handling to xHCI code

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

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

---

Changes in v2:
- Add return value check with setting hccr and hcor to NULL

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

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

index 0b09643..f8e2d70 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);

@@ -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;
 }

@@ -1093,9 +1098,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

Marek Vasut

2015-08-14 20:46 UTC #1 lore raw

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

Quoted 29 lines · Sergey Temerkhanov

This patch fixes a potential NULL pointer dereference arising on non-present/non-initialized xHCI controllers and adds some error handling to xHCI code
Signed-off-by Sergey Temerkhanov <[email protected]>
Signed-off-by Radha Mohan Chintakuntla <[email protected]>
---
Changes in v2:
- Add return value check with setting hccr and hcor to NULL
 drivers/usb/host/xhci.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/usb/host/xhci.c +1 -1
index 0b09643..f8e2d70 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 int usb_lowlevel_init(int index, enum usb_init_type · line 1085

Quoted 7 lines

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

@@ -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;

Controller should be set to NULL too, for the sake of being completely precise, don't you think so ?

Quoted 10 lines

drivers/usb/host/xhci.c +2 -1

+	}
+
 	return ret;
 }

@@ -1093,9 +1098,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 1108

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;
 }

Good job, thanks :)

Best regards,
Marek Vasut

Sergei Temerkhanov

author

2015-08-14 22:28 UTC #2 lore raw

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

Quoted 32 lines · Marek Vasut

Quoted 31 lines · Sergey Temerkhanov
This patch fixes a potential NULL pointer dereference arising on non-present/non-initialized xHCI controllers and adds some error handling to xHCI code
Signed-off-by Sergey Temerkhanov <[email protected]>
Signed-off-by Radha Mohan Chintakuntla <[email protected]>
---
Changes in v2:
- Add return value check with setting hccr and hcor to NULL
 drivers/usb/host/xhci.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/usb/host/xhci.c +1 -1
index 0b09643..f8e2d70 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);
@@ -1079,6 +1079,11 @@ int usb_lowlevel_init(int index, enum usb_init_type
 init, void **controller)
 

Quoted 8 lines

drivers/usb/host/xhci.c int usb_lowlevel_init(int index, enum usb_init_type init, void **controller) · line 1084
Quoted 5 lines
drivers/usb/host/xhci.c +3 -0
      *controller = &xhcic[index];
 
+     if (ret) {
+             ctrl->hccr = NULL;
+             ctrl->hcor = NULL;
Controller should be set to NULL too, for the sake of being completely precise, don't you think so ?

Maybe. Though the only place it's actually used at the moment (there is also some USB gadget stuff
which seems to rely on EHCI) passes a pointer to a local variable and checks the return value.

Quoted 26 lines

Quoted 11 lines

drivers/usb/host/xhci.c +2 -1

+     }
+
      return ret;
 }

@@ -1093,9 +1098,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 1108

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;
 }

Good job, thanks :)

Best regards,
Marek Vasut

Marek Vasut

2015-08-16 16:55 UTC #3 lore raw

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

Quoted 47 lines · Sergei Temerkhanov

Quoted 37 lines · Marek Vasut
Quoted 36 lines · Sergey Temerkhanov
This patch fixes a potential NULL pointer dereference arising on non-present/non-initialized xHCI controllers and adds some error handling to xHCI code
Signed-off-by Sergey Temerkhanov <[email protected]>
Signed-off-by Radha Mohan Chintakuntla <[email protected]>
---
Changes in v2:
- Add return value check with setting hccr and hcor to NULL
 drivers/usb/host/xhci.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/usb/host/xhci.c +1 -1
index 0b09643..f8e2d70 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 +0 -0
@@ -1079,6 +1079,11 @@ int usb_lowlevel_init(int index, enum
 usb_init_type init, void **controller)
 
Quoted 8 lines
drivers/usb/host/xhci.c int usb_lowlevel_init(int index, enum usb_init_type init, void **controller) · line 1084
Quoted 5 lines
drivers/usb/host/xhci.c +3 -0
      *controller = &xhcic[index];
 
+     if (ret) {
+             ctrl->hccr = NULL;
+             ctrl->hcor = NULL;
Controller should be set to NULL too, for the sake of being completely precise, don't you think so ?

Quoted 4 lines

Maybe. Though the only place it's actually used at the moment (there is also some USB gadget stuff
which seems to rely on EHCI) passes a pointer to a local variable and checks the return value.

I think it might be even better to shuffle the code around a little, so that controller is only set if ret == 0. Can you please do this last bit and send a V3 ? I'd like to pick the patch then. Thanks!

Sergei Temerkhanov

author

2015-08-18 12:16 UTC #4 lore raw

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

Quoted 48 lines · Marek Vasut

Quoted 47 lines · Sergei Temerkhanov
Quoted 45 lines · Marek Vasut
Quoted 34 lines · Sergey Temerkhanov
This patch fixes a potential NULL pointer dereference arising on non-present/non-initialized xHCI controllers and adds some error handling to xHCI code
Signed-off-by Sergey Temerkhanov <[email protected]>
Signed-off-by Radha Mohan Chintakuntla <[email protected]>
---
Changes in v2:
- Add return value check with setting hccr and hcor to NULL
 drivers/usb/host/xhci.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/usb/host/xhci.c +1 -1
index 0b09643..f8e2d70 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 int usb_lowlevel_init(int index, enum · line 1085
Quoted 7 lines
drivers/usb/host/xhci.c +3 -0
@@ -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;
Controller should be set to NULL too, for the sake of being completely precise, don't you think so ?

Quoted 8 lines

Quoted 4 lines
Maybe. Though the only place it's actually used at the moment (there is also some USB gadget stuff
which seems to rely on EHCI) passes a pointer to a local variable and checks the return value.
I think it might be even better to shuffle the code around a little, so that controller is only set if ret == 0. Can you please do this last bit and send a V3 ? I'd like to pick the patch then. Thanks!

Please see the v3 of this patch

Regards,
Sergey

Marek Vasut

2015-08-18 13:56 UTC #5 lore raw

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

Quoted 54 lines · Sergei Temerkhanov

Quoted 53 lines · Marek Vasut
Quoted 51 lines · Sergei Temerkhanov
Quoted 37 lines · Marek Vasut
Quoted 36 lines · Sergey Temerkhanov
This patch fixes a potential NULL pointer dereference arising on non-present/non-initialized xHCI controllers and adds some error handling to xHCI code
Signed-off-by Sergey Temerkhanov <[email protected]>
Signed-off-by Radha Mohan Chintakuntla <[email protected]>
---
Changes in v2:
- Add return value check with setting hccr and hcor to NULL
 drivers/usb/host/xhci.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/usb/host/xhci.c +1 -1
index 0b09643..f8e2d70 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 +0 -0
@@ -1079,6 +1079,11 @@ int usb_lowlevel_init(int index, enum
 usb_init_type init, void **controller)
 
Quoted 8 lines
drivers/usb/host/xhci.c int usb_lowlevel_init(int index, enum usb_init_type init, void **controller) · line 1084
Quoted 5 lines
drivers/usb/host/xhci.c +3 -0
      *controller = &xhcic[index];
 
+     if (ret) {
+             ctrl->hccr = NULL;
+             ctrl->hcor = NULL;
Controller should be set to NULL too, for the sake of being completely precise, don't you think so ?
Maybe. Though the only place it's actually used at the moment (there is also some USB gadget stuff
which seems to rely on EHCI) passes a pointer to a local variable and checks the return value.

Quoted 5 lines

Quoted 3 lines
I think it might be even better to shuffle the code around a little, so that controller is only set if ret == 0. Can you please do this last bit and send a V3 ? I'd like to pick the patch then. Thanks!
Please see the v3 of this patch

Hi,

I saw it, it's OK and I'll pick it shortly. Thanks!

Best regards,
Marek Vasut