| 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
| ||
|
Message-ID: <87r30zcs95.fsf@linux.intel.com>
Date: Tue, 11 Apr 2017 10:34:14 +0300
From: Felipe Balbi <balbi@...nel.org>
To: Alan Stern <stern@...land.harvard.edu>
Cc: Roger Quadros <rogerq@...com>, vivek.gautam@...eaurora.org,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device
Hi,
Alan Stern <stern@...land.harvard.edu> writes:
>> >> >> >> --- a/drivers/usb/gadget/udc/core.c
>> >> >> >> +++ b/drivers/usb/gadget/udc/core.c
>> >> >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>> >> >> >> flush_work(&gadget->work);
>> >> >> >> device_unregister(&udc->dev);
>> >> >> >> device_unregister(&gadget->dev);
>> >> >> >> + memset(&gadget->dev, 0x00, sizeof(gadget->dev));
>> >> >> >> }
>> >> >> >> EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>> >> >> >
>> >> >> > Isn't this dangerous? It's quite possible that the device_unregister()
>> >> >>
>> >> >> not on the gadget API, no.
>> >> >>
>> >> >> > call on the previous line invokes the gadget->dev.release callback,
>> >> >> > which might deallocate gadget. If that happens, your new memset will
>> >> >> > oops.
>> >> >>
>> >> >> that won't happen. struct usb_gadget is a member of the UDC's private
>> >> >> structure, like this:
>> >> >>
>> >> >> struct dwc3 {
>> >> >> [...]
>> >> >> struct usb_gadget gadget;
>> >> >> struct usb_gadget_driver *gadget_driver;
>> >> >> [...]
>> >> >> };
>> >> >
>> >> > Yes. So what? Can't the UDC driver use the refcount inside struct
>> >> > usb_gadget to control the lifetime of its private structure?
>> >>
>> >> nope, not being used. At least not yet.
>> >
>> > I'm not convinced (yet)...
>> >
>> >> > (By the way, can you tell what's going on in net2280.c? I must be
>> >> > missing something; it looks like gadget_release() would quickly run
>> >> > into problems because it calls dev_get_drvdata() for &gadget->dev, but
>> >> > net2280_probe() never calls dev_set_drvdata() for that device.
>> >> > Furthermore, net2280_remove() continues to reference the net2280 struct
>> >> > after calling usb_del_gadget_udc(), and it never does seem to do a
>> >> > final put.)
>> >>
>> >> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> >> {
>> >> struct net2280 *dev;
>> >> unsigned long resource, len;
>> >> void __iomem *base = NULL;
>> >> int retval, i;
>> >>
>> >> /* alloc, and start init */
>> >> dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>> >> if (dev == NULL) {
>> >> retval = -ENOMEM;
>> >> goto done;
>> >> }
>> >>
>> >> pci_set_drvdata(pdev, dev);
>> >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >
>> > That sets the driver data in the struct pci_dev, not in
>> > dev->gadget.dev. As far as I can see, _nothing_ in the driver sets the
>> > driver data in dev->gadget.dev.
>>
>> hmmm, indeed. The same is happening with other callers of
>> usb_add_gadget_udc_release().
>>
>> I guess this should be enough?
>>
>> @@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
>>
>> static void gadget_release(struct device *_dev)
>> {
>> - struct net2280 *dev = dev_get_drvdata(_dev);
>> + struct net2280 *dev = dev_get_drvdata(_dev->parent);
>>
>> kfree(dev);
>> }
>
> Oddly enough, yes. But it doesn't explain why this code doesn't blow
> up every time it gets called, in its current form.
Well, it does :-)
dev_get_drvdata(_dev) -> NULL -> kfree(NULL)
We're just leaking memory. I guess a patch like below would be best:
diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
index 3828c2ec8623..4dc04253da61 100644
--- a/drivers/usb/gadget/udc/net2280.c
+++ b/drivers/usb/gadget/udc/net2280.c
@@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
/*-------------------------------------------------------------------------*/
-static void gadget_release(struct device *_dev)
-{
- struct net2280 *dev = dev_get_drvdata(_dev);
-
- kfree(dev);
-}
-
/* tear down the binding between this driver and the pci device */
static void net2280_remove(struct pci_dev *pdev)
@@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
device_remove_file(&pdev->dev, &dev_attr_registers);
ep_info(dev, "unbind\n");
+
+ kfree(dev);
}
/* wrap this driver around the specified device, but
@@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id)
if (retval)
goto done;
- retval = usb_add_gadget_udc_release(&pdev->dev, &dev->gadget,
- gadget_release);
+ retval = usb_add_gadget_udc(&pdev->dev, &dev->gadget);
if (retval)
goto done;
return 0;
> And it doesn't help with the fact that net2280_remove() continues to
> access the private data structure after calling usb_del_gadget_udc().
> Strictly speaking, that routine should do
>
> get_device(&dev->gadget.dev);
>
> at the start, with a corresponding put_device() at the end.
>
> There's another problem. Suppose a call to
> usb_add_gadget_udc_release() fails. At the end of that routine, the
> error pathway does put_device(&gadget->dev). This will invoke the
> release callback, deallocating the private data structure without
> giving the caller (i.e., the UDC driver) a chance to clean up.
it won't deallocate anything :-) dev_set_drvdata() was never called,
we will endup with kfree(NULL) which is safe and just silently returns.
--
balbi
Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)
Powered by blists - more mailing lists