| Message ID | CACVXFVP9-Ao_UyBoxkxzq5dg8TfkZ9CECqEUFW5bD_JRzfQybw@mail.gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Mar 21, 2012 at 9:04 AM, Ming Lei <tom.leiming@gmail.com> wrote: > On Tue, Mar 20, 2012 at 5:40 PM, Ming Lei <tom.leiming@gmail.com> wrote: >> Hi, >> >> On Mon, Mar 19, 2012 at 11:12 PM, Dave Jones <davej@redhat.com> wrote: >>> We've had two reports of this use after free in Fedora now recently.. >> >> Could you provide output of 'dmesg' and 'lsusb -v' from the reported machine? > > Looks I have figured out why your problem is triggered. > > If the URB being unlinked is freed before usb_put_dev > inside usb_hcd_unlink_urb, the use-after-free will be triggered. > And the below patch[1] should fix the problem. > > Also there is another bug in tx_complete() of usbnet, the line below > > urb->dev = NULL; > > should be removed to avoid possible oops or memory leak in unlink path. > > Please test the patch if you can reproduce the problem. > > [1], > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index 59681f0..4f4e028 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -592,7 +592,9 @@ static int unlink_urbs (struct usbnet *dev, struct > sk_buff_head *q) > spin_unlock_irqrestore(&q->lock, flags); > // during some PM-driven resume scenarios, > // these (async) unlinks complete immediately > + local_bh_disable(); > retval = usb_unlink_urb (urb); > + local_bh_enable(); Looks it is a general issue about usb_hcd_unlink_urb. Alan, how about increasing URB reference count before calling unlink1 inside usb_hcd_unlink_urb to fix this kind of problem? Thanks,
On Wed, 21 Mar 2012, Ming Lei wrote: > Looks it is a general issue about usb_hcd_unlink_urb. > > Alan, how about increasing URB reference count before calling unlink1 > inside usb_hcd_unlink_urb to fix this kind of problem? No, that won't fix the problem. The URB could complete and be deallocated even before usb_hcd_unlink_urb() is called, so nothing that function can do will prevent the error. It is the caller's responsibility to make sure that the URB does not get freed before usb_unlink_urb() or usb_kill_urb() returns. We probably should mention this in the kerneldoc... Alan Stern -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 21, 2012 at 09:04:15AM +0800, Ming Lei wrote: > On Tue, Mar 20, 2012 at 5:40 PM, Ming Lei <tom.leiming@gmail.com> wrote: > > Hi, > > > > On Mon, Mar 19, 2012 at 11:12 PM, Dave Jones <davej@redhat.com> wrote: > >> We've had two reports of this use after free in Fedora now recently.. > > > > Could you provide output of 'dmesg' and 'lsusb -v' from the reported machine? > > Looks I have figured out why your problem is triggered. > > If the URB being unlinked is freed before usb_put_dev > inside usb_hcd_unlink_urb, the use-after-free will be triggered. > And the below patch[1] should fix the problem. With the reference counting we have, how can the urb be freed at this point in time? Is the driver doing wierd things with the urb reference counts? > Also there is another bug in tx_complete() of usbnet, the line below > > urb->dev = NULL; > > should be removed to avoid possible oops or memory leak in unlink path. > > Please test the patch if you can reproduce the problem. > > [1], > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c > index 59681f0..4f4e028 100644 > --- a/drivers/net/usb/usbnet.c > +++ b/drivers/net/usb/usbnet.c > @@ -592,7 +592,9 @@ static int unlink_urbs (struct usbnet *dev, struct > sk_buff_head *q) > spin_unlock_irqrestore(&q->lock, flags); > // during some PM-driven resume scenarios, > // these (async) unlinks complete immediately > + local_bh_disable(); > retval = usb_unlink_urb (urb); > + local_bh_enable(); That doesn't seem right, as you point out in your follow-up message. This shouldn't be needed, unless you are doing some really wierd things with the urb :( greg k-h -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 21, 2012 at 10:35 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Wed, 21 Mar 2012, Ming Lei wrote: > >> Looks it is a general issue about usb_hcd_unlink_urb. >> >> Alan, how about increasing URB reference count before calling unlink1 >> inside usb_hcd_unlink_urb to fix this kind of problem? > > No, that won't fix the problem. The URB could complete and be > deallocated even before usb_hcd_unlink_urb() is called, so nothing that > function can do will prevent the error. IMO, driver should not call usb_hcd_unlink_urb after urb is freed from the driver, but this problem is that URB may be freed during usb_hcd_unlink_urb. In fact, it is allowed that usb_free_urb is called inside .complete handler, at least as said in Documentation/URB.txt: "You may free an urb that you've submitted,..." So looks reasonable to increase the URB reference count before calling unlink1(), just like that done inside usb_hcd_flush_endpoint(). And I think it is a general solution for avoiding this kind of problem. > > It is the caller's responsibility to make sure that the URB does not > get freed before usb_unlink_urb() or usb_kill_urb() returns. We > probably should mention this in the kerneldoc... If so, looks it is a bit contrary with Documentation/URB.txt, also this may add extra constraint(maybe unnecessary) to the driver. Thanks,
On Wed, Mar 21, 2012 at 10:44 PM, Greg KH <greg@kroah.com> wrote: > On Wed, Mar 21, 2012 at 09:04:15AM +0800, Ming Lei wrote: >> On Tue, Mar 20, 2012 at 5:40 PM, Ming Lei <tom.leiming@gmail.com> wrote: >> > Hi, >> > >> > On Mon, Mar 19, 2012 at 11:12 PM, Dave Jones <davej@redhat.com> wrote: >> >> We've had two reports of this use after free in Fedora now recently.. >> > >> > Could you provide output of 'dmesg' and 'lsusb -v' from the reported machine? >> >> Looks I have figured out why your problem is triggered. >> >> If the URB being unlinked is freed before usb_put_dev >> inside usb_hcd_unlink_urb, the use-after-free will be triggered. >> And the below patch[1] should fix the problem. > > With the reference counting we have, how can the urb be freed at this > point in time? Is the driver doing wierd things with the urb reference > counts? The problem is that the .complete may schedule a tasklet to free the completed URB. And the .complete may be run inside unlink path, so the use-after-free will be triggered if the tasklet is excuted before usb_put_dev inside usb_hcd_unlink_urb. > >> Also there is another bug in tx_complete() of usbnet, the line below >> >> urb->dev = NULL; >> >> should be removed to avoid possible oops or memory leak in unlink path. >> >> Please test the patch if you can reproduce the problem. >> >> [1], >> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c >> index 59681f0..4f4e028 100644 >> --- a/drivers/net/usb/usbnet.c >> +++ b/drivers/net/usb/usbnet.c >> @@ -592,7 +592,9 @@ static int unlink_urbs (struct usbnet *dev, struct >> sk_buff_head *q) >> spin_unlock_irqrestore(&q->lock, flags); >> // during some PM-driven resume scenarios, >> // these (async) unlinks complete immediately >> + local_bh_disable(); >> retval = usb_unlink_urb (urb); >> + local_bh_enable(); > > That doesn't seem right, as you point out in your follow-up message. > This shouldn't be needed, unless you are doing some really wierd things > with the urb :( Looks the driver doesn't do any wierd things, as said above. Thanks,
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 59681f0..4f4e028 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -592,7 +592,9 @@ static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q) spin_unlock_irqrestore(&q->lock, flags); // during some PM-driven resume scenarios, // these (async) unlinks complete immediately + local_bh_disable(); retval = usb_unlink_urb (urb); + local_bh_enable(); if (retval != -EINPROGRESS && retval != 0) netdev_dbg(dev->net, "unlink urb err, %d\n", retval);