linux-kernel - RE: [PATCH] usb: cdnsp: Fix lack of spin_lock_irqsave/spin_lock

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] [day] [month] [year] [list]

Message-ID: <BYAPR07MB53811E2C7CB05B9DEACF7A8CDD749@BYAPR07MB5381.namprd07.prod.outlook.com>
Date:   Mon, 13 Dec 2021 14:24:02 +0000
From:   Pawel Laszczak <pawell@...ence.com>
To:     Peter Chen <peter.chen@...nel.org>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jianhe@...arella.com" <jianhe@...arella.com>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH] usb: cdnsp: Fix lack of
 spin_lock_irqsave/spin_lock_restore

>
>On 21-12-13 13:20:01, Pawel Laszczak wrote:
>> From: Pawel Laszczak <pawell@...ence.com>
>>
>> Patch puts content of cdnsp_gadget_pullup function inside
>> spin_lock_irqsave and spin_lock_restore section.
>> This construction is required here to keep the data consistency,
>> otherwise some data can be changed e.g. from interrupt context.
>>
>> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
>> Reported-by: Ken (Jian) He <jianhe@...arella.com>
>> cc: <stable@...r.kernel.org>
>> Signed-off-by: Pawel Laszczak <pawell@...ence.com>
>> ---
>>  drivers/usb/cdns3/cdnsp-gadget.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c b/drivers/usb/cdns3/cdnsp-gadget.c
>> index f6d231760a6a..d0c040556984 100644
>> --- a/drivers/usb/cdns3/cdnsp-gadget.c
>> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
>> @@ -1544,8 +1544,10 @@ static int cdnsp_gadget_pullup(struct usb_gadget *gadget, int is_on)
>>  {
>>  	struct cdnsp_device *pdev = gadget_to_cdnsp(gadget);
>>  	struct cdns *cdns = dev_get_drvdata(pdev->dev);
>> +	unsigned long flags;
>>
>>  	trace_cdnsp_pullup(is_on);
>> +	spin_lock_irqsave(&pdev->lock, flags);
>
>If the interrupt bottom half is pending, the consistent issue may still
>exist, you may let the bottom half has finished first, eg: calling
>disable_irq before spin_lock_irqsave.
>
>Peter
>>

But bottom half procedure is also protected by spin lock, so it will be waiting for completion
cdnsp_gadget_pullup and vice versa.

I think you means the case when driver in bottom half function release the spin lock before calling some API function.
and in this moment the pullup function starts to be handled. 
I didn't detect such issue, but theoretically it is possible.

Let me test option with disable_irq before spin_lock_irqsave.

>>  	if (!is_on) {
>>  		cdnsp_reset_device(pdev);
>> @@ -1553,6 +1555,9 @@ static int cdnsp_gadget_pullup(struct usb_gadget *gadget, int is_on)
>>  	} else {
>>  		cdns_set_vbus(cdns);
>>  	}
>> +
>> +	spin_unlock_irqrestore(&pdev->lock, flags);
>> +
>>  	return 0;
>>  }
>>
>> --
>> 2.25.1
>>
>

--

Thanks,
Pawel Laszczak