Messages in this thread | | | Date | Tue, 10 Jul 2012 09:56:39 +0530 | From | Venu Byravarasu <> | Subject | Re: [PATCH v1] usb: host: Fix possible kernel crash |
| |
Thanks Alan for your comments.
On Monday 09 July 2012 08:04 PM, Alan Stern wrote: > On Mon, 9 Jul 2012, Venu Byravarasu wrote: > >> In functions itd_complete & sitd_complete, a pointer >> by name stream may get dereferenced after freeing it, when >> iso_stream_put is called with stream->refcount = 2. > I don't understand the problem. Did you actually see this happen or is > it only theoretical?
Yes it is a theoretical problem, as complained by Coverity.
> /* for each uframe with a packet */ > for (uframe = 0; uframe < 8; uframe++) { > @@ -1783,7 +1784,8 @@ itd_complete ( > dev->devpath, stream->bEndpointAddress & 0x0f, > (stream->bEndpointAddress & USB_DIR_IN) ? "in" : "out"); > } > - iso_stream_put (ehci, stream); > + stream_ref_count = stream->refcount; > + iso_stream_put(ehci, stream); > This iso_stream_put removes the reference held by the URB. Before it > is called, stream->refcount must be >= 3: > > refcount is set to 1 when the stream is created; > > each active URB holds a reference; > > each itd holds a reference. > > So after the call, the refcount value must be >= 2 and the stream could > not have been deallocated. > >> done: >> itd->urb = NULL; >> @@ -1797,7 +1799,7 @@ done: >> * Move it to a safe place until a new frame starts. >> */ >> list_move(&itd->itd_list, &ehci->cached_itd_list); >> - if (stream->refcount == 2) { >> + if (stream_ref_count == 3) { > Therefore this seems unnecessary.
As per the logic you explained above, this change is not needed. However coverity was complaining as below:
/kernel/drivers/usb/host/ehci-sched.c 1777 USE_AFTER_FREE Dereferencing freed pointer "stream"
Hence to pacify coverity, this change is done. Please let me know if you see any other better way to handle it.
>> /* If iso_stream_put() were called here, stream >> * would be freed. Instead, just prevent reuse. >> */ > Alan Stern >
Thanks, Venu
| |