[U-Boot] [PATCH v2] usb: xhci: Fix a potential NULL pointer dereference

[Sergei Temerkhanov]

On Fri, Aug 14, 2015 at 11:46 PM, Marek Vasut <marex at denx.de> wrote:
> On Friday, August 14, 2015 at 05:14:09 PM, Sergey Temerkhanov wrote:
>> 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 <s.temerkhanov at gmail.com>
>> Signed-off-by: Radha Mohan Chintakuntla <rchintakuntla at cavium.com>
>>
>> ---
>>
>> 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(-)
>>
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 0b09643..f8e2d70 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);
>> @@ -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 ?

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.
>
>> +     }
>> +
>>       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;
>>  }
>
> Good job, thanks :)
>
> Best regards,
> Marek Vasut