From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Fri, 14 Aug 2015 15:44:17 +0200
Subject: [U-Boot] [PATCH] usb: xhci: Fix a potential NULL pointer
	dereference
In-Reply-To: <CAPEA6dZZdRw0KBM8r7Ceoqy2NqD-c1S4vOkyB8rFZ1Xx5NP5zw@mail.gmail.com>
References: <1439554386-9406-1-git-send-email-s.temerkhanov@gmail.com>
 <201508141415.36132.marex@denx.de>
 <CAPEA6dZZdRw0KBM8r7Ceoqy2NqD-c1S4vOkyB8rFZ1Xx5NP5zw@mail.gmail.com>
Message-ID: <201508141544.17866.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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