Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
>> > Felipe:
>> >
>> > On Thu, 29 Jun 2017, kernel test robot wrote:
>> >
>> >> FYI, we noticed the following commit:
>> >> 
>> >> commit: f16443a034c7aa359ddf6f0f9bc40d01ca31faea ("USB: gadgetfs, dummy-hcd, net2280: fix locking for callbacks")
>> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
>> >> 
>> >> in testcase: trinity
>> >> with following parameters:
>> >> 
>> >> 	runtime: 300s
>> >> 
>> >> test-description: Trinity is a linux system call fuzz tester.
>> >> test-url: http://codemonkey.org.uk/projects/trinity/
>> >> 
>> >> 
>> >> on test machine: qemu-system-x86_64 -enable-kvm -m 420M
>> >> 
>> >> caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
>> > ...
>> >
>> > I won't include the entire report.  The gist is that we have a problem 
>> > with lock ordering.  The report is about dummy-hcd, but this could 
>> > affect any UDC driver.
>> >
>> >      1. When a SETUP request arrives, composite_setup() acquires
>> > 	cdev->lock before calling the function driver's callback.
>> > 	When that callback submits a reply, it causes the UDC driver
>> > 	to acquire its private lock.
>> 
>> this only seems to happen before calling set_config():
>> 
>> 	case USB_REQ_SET_CONFIGURATION:
>> 		if (ctrl->bRequestType != 0)
>> 			goto unknown;
>> 		if (gadget_is_otg(gadget)) {
>> 			if (gadget->a_hnp_support)
>> 				DBG(cdev, "HNP available\n");
>> 			else if (gadget->a_alt_hnp_support)
>> 				DBG(cdev, "HNP on another port\n");
>> 			else
>> 				VDBG(cdev, "HNP inactive\n");
>> 		}
>> 		spin_lock(&cdev->lock);
>> 		value = set_config(cdev, ctrl, w_value);
>> 		spin_unlock(&cdev->lock);
>> 		break;
>
> That's true.  Why is the lock held for set_config() but not for any of 
> the other callbacks?

this is really old code from Dave. Your guess is as good as mine :-(

> Doesn't that mean the other callbacks can race with function
> unregistration?

Probably not as UDCs are required to cancel transfers and kill all
endpoints before unregistering. We would probably just giveback a few
requests with -ESHUTDOWN and prevent new ones from being queued to HW,
no?

>> The "reply" here is a usb_ep_queue(gadget->ep0, req) call which should
>> hold UDC driver's private lock and disable interrupts for a short period
>> of time. Here's how it looks on dwc3:
>> 
>> int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
>> 		gfp_t gfp_flags)
>> {
>> 	struct dwc3_request		*req = to_dwc3_request(request);
>> 	struct dwc3_ep			*dep = to_dwc3_ep(ep);
>> 	struct dwc3			*dwc = dep->dwc;
>> 
>> 	unsigned long			flags;
>> 
>> 	int				ret;
>> 
>> 	spin_lock_irqsave(&dwc->lock, flags);
>> 	if (!dep->endpoint.desc) {
>> 		dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
>> 				dep->name);
>> 		ret = -ESHUTDOWN;
>> 		goto out;
>> 	}
>> 
>> 	/* we share one TRB for ep0/1 */
>> 	if (!list_empty(&dep->pending_list)) {
>> 		ret = -EBUSY;
>> 		goto out;
>> 	}
>> 
>> 	ret = __dwc3_gadget_ep0_queue(dep, req);
>> 
>> out:
>> 	spin_unlock_irqrestore(&dwc->lock, flags);
>> 
>> 	return ret;
>> }
>> 
>> >      2. When a bus reset occurs, the UDC's interrupt handler acquires
>> > 	its private lock before calling usb_gadget_udc_reset(), which
>> > 	calls composite_disconnect(), which acquires cdev->lock.
>> 
>> The reset handler will only run after B has been released though, no?
>
> B?  Do you mean the UDC's private lock?

yes :-)

>> Also, UDC should *release* its private lock before calling UDC reset:
>> 
>> static void dwc3_reset_gadget(struct dwc3 *dwc)
>> {
>> 	if (!dwc->gadget_driver)
>> 		return;
>> 
>> 	if (dwc->gadget.speed != USB_SPEED_UNKNOWN) {
>> 		spin_unlock(&dwc->lock);
>> 		usb_gadget_udc_reset(&dwc->gadget, dwc->gadget_driver);
>> 		spin_lock(&dwc->lock);
>> 	}
>> }
>> 
>> Are you sure this isn't a bug in dummy only?
>
> That's the issue.  The commit in $SUBJECT changed dummy-hcd precisely
> to _avoid_ releasing the private lock before calling the reset routine.
>
> The reason is explained in the commit's changelog: If the lock isn't
> held then it is possible for the user to cause a use-after-free BUG by
> closing the gadgetfs control file when a reset is about to occur.  The 
> bug report showed this happening with dummy-hcd, but the same bug ought 
> to affect any UDC driver that isn't careful about races between 
> callbacks and unregistration.

understood. Then seems like this should be handled in a more generic way.

> Please read the follow-up email in this thread, the one sent on June 30.

Will do

-- 
balbi