Message ID | 1439554386-9406-1-git-send-email-s.temerkhanov@gmail.com |
---|---|
State | Superseded |
Delegated to: | Marek Vasut |
Headers | show |
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
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 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
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; }