| Message ID | 20200721201558.20069-1-novikov@ispras.ru (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 2468c877da428ebfd701142c4cdfefcfb7d4c00e |
| Headers | show |
| Series | usb: gadget: net2280: fix memory leak on probe error handling paths | expand |
On Tue, Jul 21, 2020 at 11:15:58PM +0300, Evgeny Novikov wrote: > Driver does not release memory for device on error handling paths in > net2280_probe() when gadget_release() is not registered yet. > > The patch fixes the bug like in other similar drivers. > > Found by Linux Driver Verification project (linuxtesting.org). > > Signed-off-by: Evgeny Novikov <novikov@ispras.ru> > --- > drivers/usb/gadget/udc/net2280.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c > index 5eff85eeaa5a..d5fe071b2db2 100644 > --- a/drivers/usb/gadget/udc/net2280.c > +++ b/drivers/usb/gadget/udc/net2280.c > @@ -3781,8 +3781,10 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id) > return 0; > > done: > - if (dev) > + if (dev) { > net2280_remove(pdev); > + kfree(dev); > + } > return retval; > } This patch seems to be the tip of an iceberg. Following through its implications led to a couple of discoveries. usb_del_gadget_udc() calls device_unregister(&gadget->dev). Once this call returns, gadget has to be regarded as a stale pointer. But the very next line of code does: memset(&gadget->dev, 0x00, sizeof(gadget->dev)); for no apparent reason. I'm amazed this hasn't caused problems already. Is there any justification for keeping this memset? It's hard to imagine that it does any good. Similarly, net2280_remove() calls usb_del_gadget_udc(&dev->gadget) at its start, and so dev must be a stale pointer for the entire remainder of the routine. But it gets used repeatedly. Surely we ought to have a device_get() and device_put() in there. Alan Stern
Hi Alan, I have neither an appropriate hardware nor an experience to deal with issues that you mentioned. Our framework does not allow to detect them as well at the moment. At last, it seems that rather many drivers can suffer from these issues. So, it would be much better if somebody else will suggest necessary fixes and test them carefully. BTW, you have already discussed the race within net2280_remove() with my colleague about 3 years ago. But you did not achieve a consensus at that time and no fixes were made after all. Anyway, one can consider both issues independently on the one fixed by the patch. -- Evgeny Novikov Linux Verification Center, ISP RAS http://linuxtesting.org 22.07.2020, 17:17, "Alan Stern" <stern@rowland.harvard.edu>: > On Tue, Jul 21, 2020 at 11:15:58PM +0300, Evgeny Novikov wrote: >> Driver does not release memory for device on error handling paths in >> net2280_probe() when gadget_release() is not registered yet. >> >> The patch fixes the bug like in other similar drivers. >> >> Found by Linux Driver Verification project (linuxtesting.org). >> >> Signed-off-by: Evgeny Novikov <novikov@ispras.ru> >> --- >> drivers/usb/gadget/udc/net2280.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c >> index 5eff85eeaa5a..d5fe071b2db2 100644 >> --- a/drivers/usb/gadget/udc/net2280.c >> +++ b/drivers/usb/gadget/udc/net2280.c >> @@ -3781,8 +3781,10 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> return 0; >> >> done: >> - if (dev) >> + if (dev) { >> net2280_remove(pdev); >> + kfree(dev); >> + } >> return retval; >> } > > This patch seems to be the tip of an iceberg. Following through its > implications led to a couple of discoveries. > > usb_del_gadget_udc() calls device_unregister(&gadget->dev). Once this > call returns, gadget has to be regarded as a stale pointer. But the > very next line of code does: > > memset(&gadget->dev, 0x00, sizeof(gadget->dev)); > > for no apparent reason. I'm amazed this hasn't caused problems already. > Is there any justification for keeping this memset? It's hard to > imagine that it does any good. > > Similarly, net2280_remove() calls usb_del_gadget_udc(&dev->gadget) at > its start, and so dev must be a stale pointer for the entire remainder > of the routine. But it gets used repeatedly. Surely we ought to have > a device_get() and device_put() in there. > > Alan Stern
On Wed, 2020-07-22 at 22:56 +0300, Evgeny Novikov wrote: > Hi Alan, > > I have neither an appropriate hardware nor an experience to deal with > issues that you mentioned. Our framework does not allow to detect > them as well at the moment. At last, it seems that rather many > drivers can suffer from these issues. So, it would be much better if > somebody else will suggest necessary fixes and test them carefully. > > BTW, you have already discussed the race within net2280_remove() with > my colleague about 3 years ago. But you did not achieve a consensus > at that time and no fixes were made after all. > > Anyway, one can consider both issues independently on the one fixed > by the patch. FYI. It looks like I'm likely to resume my work on that driver in the next few weeks in which case I could probably look into these Alan. Cheers, Ben. > -- > Evgeny Novikov > Linux Verification Center, ISP RAS > http://linuxtesting.org > > 22.07.2020, 17:17, "Alan Stern" <stern@rowland.harvard.edu>: > > On Tue, Jul 21, 2020 at 11:15:58PM +0300, Evgeny Novikov wrote: > > > Driver does not release memory for device on error handling > > > paths in > > > net2280_probe() when gadget_release() is not registered yet. > > > > > > The patch fixes the bug like in other similar drivers. > > > > > > Found by Linux Driver Verification project (linuxtesting.org). > > > > > > Signed-off-by: Evgeny Novikov <novikov@ispras.ru> > > > --- > > > drivers/usb/gadget/udc/net2280.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/gadget/udc/net2280.c > > > b/drivers/usb/gadget/udc/net2280.c > > > index 5eff85eeaa5a..d5fe071b2db2 100644 > > > --- a/drivers/usb/gadget/udc/net2280.c > > > +++ b/drivers/usb/gadget/udc/net2280.c > > > @@ -3781,8 +3781,10 @@ static int net2280_probe(struct pci_dev > > > *pdev, const struct pci_device_id *id) > > > return 0; > > > > > > done: > > > - if (dev) > > > + if (dev) { > > > net2280_remove(pdev); > > > + kfree(dev); > > > + } > > > return retval; > > > } > > > > This patch seems to be the tip of an iceberg. Following through its > > implications led to a couple of discoveries. > > > > usb_del_gadget_udc() calls device_unregister(&gadget->dev). Once > > this > > call returns, gadget has to be regarded as a stale pointer. But the > > very next line of code does: > > > > memset(&gadget->dev, 0x00, sizeof(gadget->dev)); > > > > for no apparent reason. I'm amazed this hasn't caused problems > > already. > > Is there any justification for keeping this memset? It's hard to > > imagine that it does any good. > > > > Similarly, net2280_remove() calls usb_del_gadget_udc(&dev->gadget) > > at > > its start, and so dev must be a stale pointer for the entire > > remainder > > of the routine. But it gets used repeatedly. Surely we ought to > > have > > a device_get() and device_put() in there. > > > > Alan Stern
On Wed, Jul 22, 2020 at 10:56:09PM +0300, Evgeny Novikov wrote: > Hi Alan, > > I have neither an appropriate hardware nor an experience to deal with > issues that you mentioned. Our framework does not allow to detect them > as well at the moment. At last, it seems that rather many drivers can > suffer from these issues. So, it would be much better if somebody else > will suggest necessary fixes and test them carefully. Heh... Working from home, I no longer have access to the appropriate hardware either. But at least I do have the necessary experience. :-) > BTW, you have already discussed the race within net2280_remove() with > my colleague about 3 years ago. But you did not achieve a consensus at > that time and no fixes were made after all. I don't recall that. Do you have a pointer to the email thread in the archives? > Anyway, one can consider both issues independently on the one fixed by > the patch. Yes. I'll write and submit a series of patches. Alan Stern
On Thu, Jul 23, 2020 at 10:59:06AM +1000, Benjamin Herrenschmidt wrote: > On Wed, 2020-07-22 at 22:56 +0300, Evgeny Novikov wrote: > > Hi Alan, > > > > I have neither an appropriate hardware nor an experience to deal with > > issues that you mentioned. Our framework does not allow to detect > > them as well at the moment. At last, it seems that rather many > > drivers can suffer from these issues. So, it would be much better if > > somebody else will suggest necessary fixes and test them carefully. > > > > BTW, you have already discussed the race within net2280_remove() with > > my colleague about 3 years ago. But you did not achieve a consensus > > at that time and no fixes were made after all. > > > > Anyway, one can consider both issues independently on the one fixed > > by the patch. > > FYI. It looks like I'm likely to resume my work on that driver in the > next few weeks in which case I could probably look into these Alan. That probably won't be needed, although thanks for the offer. I'll CC you on any patches when they are submitted. Alan Stern
23.07.2020, 04:00, "Alan Stern" <stern@rowland.harvard.edu>: > On Wed, Jul 22, 2020 at 10:56:09PM +0300, Evgeny Novikov wrote: >> Hi Alan, >> >> I have neither an appropriate hardware nor an experience to deal with >> issues that you mentioned. Our framework does not allow to detect them >> as well at the moment. At last, it seems that rather many drivers can >> suffer from these issues. So, it would be much better if somebody else >> will suggest necessary fixes and test them carefully. > > Heh... Working from home, I no longer have access to the appropriate > hardware either. But at least I do have the necessary experience. :-) > >> BTW, you have already discussed the race within net2280_remove() with >> my colleague about 3 years ago. But you did not achieve a consensus at >> that time and no fixes were made after all. > > I don't recall that. Do you have a pointer to the email thread in the > archives? https://lkml.org/lkml/2017/8/16/345 - this is the top message of that thread. Evgeny >> Anyway, one can consider both issues independently on the one fixed by >> the patch. > > Yes. I'll write and submit a series of patches. > > Alan Stern
diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c index 5eff85eeaa5a..d5fe071b2db2 100644 --- a/drivers/usb/gadget/udc/net2280.c +++ b/drivers/usb/gadget/udc/net2280.c @@ -3781,8 +3781,10 @@ static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id *id) return 0; done: - if (dev) + if (dev) { net2280_remove(pdev); + kfree(dev); + } return retval; }
Driver does not release memory for device on error handling paths in net2280_probe() when gadget_release() is not registered yet. The patch fixes the bug like in other similar drivers. Found by Linux Driver Verification project (linuxtesting.org). Signed-off-by: Evgeny Novikov <novikov@ispras.ru> --- drivers/usb/gadget/udc/net2280.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)