linux-kernel - Re: [PATCH] usb: dwc3: qcom: fix NULL pointer dereference on dwc3_qcom_read_usb2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <48651728-a4bb-4bef-81d7-6100a6c2a1fe@quicinc.com>
Date: Wed, 14 Aug 2024 16:42:12 +0530
From: Faisal Hassan <quic_faisalh@...cinc.com>
To: Prashanth K <quic_prashk@...cinc.com>,
        Thinh Nguyen
	<Thinh.Nguyen@...opsys.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH] usb: dwc3: qcom: fix NULL pointer dereference on
 dwc3_qcom_read_usb2_speed



On 8/14/2024 11:05 AM, Prashanth K wrote:
> 
> 
> On 14-08-24 05:47 am, Thinh Nguyen wrote:
>> On Tue, Aug 13, 2024, Faisal Hassan wrote:
>>> Null pointer dereference occurs when accessing 'hcd' to detect speed
>>> from dwc3_qcom_suspend after the xhci-hcd is unbound.
>>> To avoid this issue, ensure to check for NULL in
>>> dwc3_qcom_read_usb2_speed.
>>>
>>> echo xhci-hcd.0.auto > /sys/bus/platform/drivers/xhci-hcd/unbind
>>>    xhci_plat_remove() -> usb_put_hcd() -> hcd_release() -> kfree(hcd)
>>>
>>>    Unable to handle kernel NULL pointer dereference at virtual address
>>>    0000000000000060
>>>    Call trace:
>>>     dwc3_qcom_suspend.part.0+0x17c/0x2d0 [dwc3_qcom]
>>>     dwc3_qcom_runtime_suspend+0x2c/0x40 [dwc3_qcom]
>>>     pm_generic_runtime_suspend+0x30/0x44
>>>     __rpm_callback+0x4c/0x190
>>>     rpm_callback+0x6c/0x80
>>>     rpm_suspend+0x10c/0x620
>>>     pm_runtime_work+0xc8/0xe0
>>>     process_one_work+0x1e4/0x4f4
>>>     worker_thread+0x64/0x43c
>>>     kthread+0xec/0x100
>>>     ret_from_fork+0x10/0x20
>>>
>>> Fixes: c5f14abeb52b ("usb: dwc3: qcom: fix peripheral and OTG suspend")
>>> Cc: stable@...r.kernel.org
>>> Signed-off-by: Faisal Hassan <quic_faisalh@...cinc.com>
>>> ---
>>>   drivers/usb/dwc3/dwc3-qcom.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>>> index 88fb6706a18d..0c7846478655 100644
>>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>>> @@ -319,13 +319,15 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom
>>> *qcom)
>>>   static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct
>>> dwc3_qcom *qcom, int port_index)
>>>   {
>>>       struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>>
>> What if dwc is not available?
> 
> Thats unlikely, dwc3_qcom_suspend() -> dwc3_qcom_is_host() checks for
> dwc, calls dwc3_qcom_read_usb2_speed() only if dwc is valid. But adding
> an extra check shouldn't cause harm.

Thanks Thinh and Prashanth for reviewing the patch.
Since the caller is validating 'dwc', I think there is no need to recheck.

>>
>>> -    struct usb_device *udev;
>>> +    struct usb_device __maybe_unused *udev;
>>
>> This is odd.... Is there a scenario where you don't want to set
>> CONFIG_USB if dwc3_qcom is in use?
>>
> AFAIK this function is used to get the speeds of each ports, so that
> wakeup interrupts (dp/dm/ss irqs) can be configured accordingly before
> going to suspend, which is done during host mode only. So there
> shouldn't be any scenarios where CONFIG_USB isnt set when this is called.

>From history I see CONFIG_USB was added to fix build issues for gadget
only configuration. So configuration without CONFIG_USB also exists.

>>>       struct usb_hcd __maybe_unused *hcd;
>>>         /*
>>>        * FIXME: Fix this layering violation.
>>>        */
>>>       hcd = platform_get_drvdata(dwc->xhci);
>>> +    if (!hcd)
>>> +        return USB_SPEED_UNKNOWN;
>>>     #ifdef CONFIG_USB
>>
>> Perhaps this #ifdef shouldn't only be checking this. But that's for
>> another patch >>       udev = usb_hub_find_child(hcd->self.root_hub,
>> port_index + 1);
>>> -- 
>>> 2.17.1
>>>
>>
>> BR,
>> Thinh
> Thanks,
> Prashanth K
Thanks,
Faisal