| 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
| ||
|
Date: Mon, 15 Dec 2014 17:01:14 +0100
From: Robert Baldyga <r.baldyga@...sung.com>
To: Paul Zimmerman <Paul.Zimmerman@...opsys.com>
Cc: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"balbi@...com" <balbi@...com>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"m.szyprowski@...sung.com" <m.szyprowski@...sung.com>,
"k.opasiak@...sung.com" <k.opasiak@...sung.com>
Subject: Re: [PATCH] usb: dwc2: gadget: kill requests with 'force' in
s3c_hsotg_udc_stop()
On 12/11/2014 08:34 PM, Paul Zimmerman wrote:
>> From: Robert Baldyga [mailto:r.baldyga@...sung.com]
>> Sent: Tuesday, December 09, 2014 5:42 AM
>>
>> This makes us sure that all requests are completed before we unbind
>> gadget. There are assumptions in gadget API that all requests have to
>> be completed and leak of complete can break some usb function drivers.
>>
>> For example unbind of ECM function can cause NULL pointer dereference:
>>
>> [ 26.396595] configfs-gadget gadget: unbind function
>> 'cdc_ethernet'/e79c4c00
>> [ 26.414999] Unable to handle kernel NULL pointer dereference at
>> virtual address 00000000
>> (...)
>> [ 26.452223] PC is at ecm_unbind+0x6c/0x9c
>> [ 26.456209] LR is at ecm_unbind+0x68/0x9c
>> (...)
>> [ 26.603696] [<c033fdb4>] (ecm_unbind) from [<c033661c>]
>> (purge_configs_funcs+0x94/0xd8)
>> [ 26.611674] [<c033661c>] (purge_configs_funcs) from [<c0336674>]
>> (configfs_composite_unbind+0x14/0x34)
>> [ 26.620961] [<c0336674>] (configfs_composite_unbind) from
>> [<c0337124>] (usb_gadget_remove_driver+0x68/0x9c)
>> [ 26.630683] [<c0337124>] (usb_gadget_remove_driver) from [<c03376c8>]
>> (usb_gadget_unregister_driver+0x64/0x94)
>> [ 26.640664] [<c03376c8>] (usb_gadget_unregister_driver) from
>> [<c0336be8>] (unregister_gadget+0x20/0x3c)
>> [ 26.650038] [<c0336be8>] (unregister_gadget) from [<c0336c84>]
>> (gadget_dev_desc_UDC_store+0x80/0xb8)
>> [ 26.659152] [<c0336c84>] (gadget_dev_desc_UDC_store) from
>> [<c0335120>] (gadget_info_attr_store+0x1c/0x28)
>> [ 26.668703] [<c0335120>] (gadget_info_attr_store) from [<c012135c>]
>> (configfs_write_file+0xe8/0x148)
>> [ 26.677818] [<c012135c>] (configfs_write_file) from [<c00c8dd4>]
>> (vfs_write+0xb0/0x1a0)
>> [ 26.685801] [<c00c8dd4>] (vfs_write) from [<c00c91b8>]
>> (SyS_write+0x44/0x84)
>> [ 26.692834] [<c00c91b8>] (SyS_write) from [<c000e560>]
>> (ret_fast_syscall+0x0/0x30)
>> [ 26.700381] Code: e30409f8 e34c0069 eb07b88d e59430a8 (e5930000)
>> [ 26.706485] ---[ end trace f62a082b323838a2 ]---
>>
>> It's because in some cases request is still running on endpoint during
>> unbind and kill_all_requests() called from s3c_hsotg_udc_stop() function
>> doesn't cause call of complete() of request. Missing complete() call
>> causes ecm->notify_req equals NULL in ecm_unbind() function, and this
>> is reason of this bug.
>>
>> Similar breaks can be observed in another usb function drivers.
>>
>> This patch fixes this bug forcing usb request completion in when
>> s3c_hsotg_ep_disable() is called from s3c_hsotg_udc_stop().
>>
>> Signed-off-by: Robert Baldyga <r.baldyga@...sung.com>
>> ---
>> drivers/usb/dwc2/gadget.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 8b5c079..cb4c925 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -2590,7 +2590,7 @@ error:
>> * s3c_hsotg_ep_disable - disable given endpoint
>> * @ep: The endpoint to disable.
>> */
>> -static int s3c_hsotg_ep_disable(struct usb_ep *ep)
>> +static int s3c_hsotg_ep_disable_force(struct usb_ep *ep, bool force)
>> {
>> struct s3c_hsotg_ep *hs_ep = our_ep(ep);
>> struct s3c_hsotg *hsotg = hs_ep->parent;
>> @@ -2611,7 +2611,7 @@ static int s3c_hsotg_ep_disable(struct usb_ep *ep)
>>
>> spin_lock_irqsave(&hsotg->lock, flags);
>> /* terminate all requests with shutdown */
>> - kill_all_requests(hsotg, hs_ep, -ESHUTDOWN, false);
>> + kill_all_requests(hsotg, hs_ep, -ESHUTDOWN, force);
>>
>> hsotg->fifo_map &= ~(1<<hs_ep->fifo_index);
>> hs_ep->fifo_index = 0;
>> @@ -2632,6 +2632,10 @@ static int s3c_hsotg_ep_disable(struct usb_ep *ep)
>> return 0;
>> }
>>
>> +static int s3c_hsotg_ep_disable(struct usb_ep *ep)
>> +{
>> + return s3c_hsotg_ep_disable_force(ep, false);
>> +}
>> /**
>> * on_list - check request is on the given endpoint
>> * @ep: The endpoint to check.
>> @@ -2933,7 +2937,7 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
>>
>> /* all endpoints should be shutdown */
>> for (ep = 1; ep < hsotg->num_of_eps; ep++)
>> - s3c_hsotg_ep_disable(&hsotg->eps[ep].ep);
>> + s3c_hsotg_ep_disable_force(&hsotg->eps[ep].ep, true);
>>
>> spin_lock_irqsave(&hsotg->lock, flags);
>>
>
> Acked-by: Paul Zimmerman <paulz@...opsys.com>
>
One more thing which I have forgot to write: This patch makes sense only
with my patch to udc-core [1] which modifies order of calls in
usb_gadget_remove_driver() function. Hence this patch shouldn't be
applied before [1] will be accepted.
[1] https://lkml.org/lkml/2014/12/12/360
Best regards,
Robert Baldyga
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists