On Tue, Mar 20, 2012 at 5:40 PM, Ming Lei [off-list ref] wrote:
Hi,
On Mon, Mar 19, 2012 at 11:12 PM, Dave Jones [off-list ref] wrote:
quoted
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],
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Ming Lei <tom.leiming@gmail.com> Date: 2012-03-21 01:34:42
On Wed, Mar 21, 2012 at 9:04 AM, Ming Lei [off-list ref] wrote:
quoted hunk
On Tue, Mar 20, 2012 at 5:40 PM, Ming Lei [off-list ref] wrote:
quoted
Hi,
On Mon, Mar 19, 2012 at 11:12 PM, Dave Jones [off-list ref] wrote:
quoted
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],
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,
--
Ming Lei
From: Alan Stern <stern@rowland.harvard.edu> Date: 2012-03-21 14:35:23
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
On Wed, Mar 21, 2012 at 09:04:15AM +0800, Ming Lei wrote:
On Tue, Mar 20, 2012 at 5:40 PM, Ming Lei [off-list ref] wrote:
quoted
Hi,
On Mon, Mar 19, 2012 at 11:12 PM, Dave Jones [off-list ref] wrote:
quoted
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?
quoted hunk
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],
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
From: Ming Lei <tom.leiming@gmail.com> Date: 2012-03-21 15:07:57
On Wed, Mar 21, 2012 at 10:44 PM, Greg KH [off-list ref] wrote:
On Wed, Mar 21, 2012 at 09:04:15AM +0800, Ming Lei wrote:
quoted
On Tue, Mar 20, 2012 at 5:40 PM, Ming Lei [off-list ref] wrote:
quoted
Hi,
On Mon, Mar 19, 2012 at 11:12 PM, Dave Jones [off-list ref] wrote:
quoted
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.
quoted
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],
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,
--
Ming Lei