lkml.org 
[lkml]   [2012]   [Jul]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRE: [PATCH v1] usb: host: Fix possible kernel crash
Hi Greg/Venu/Alan and others,

The defect discussed in this thread was found in 2006, and was marked in Coverity Scan as false positive - intentional ( by linux developer or coverity admin that we don't know)...

As a general rule,
1. what was discussed with some of the Linux folks, Focus on NEW defects...
2. Do NOT fix anything that is already marked as Intentional /False Positive


For any defects found by Covierty Scan there could be potential 3 actions
1. Review and Fix the defect
2. Mark it as Intentional in Coverity Scan, and it will not appear as Defect in the future
3. Contact Coverity Scan-admin, if you would like to understand it more why it was flagged as defect.

• As we all know, inherent in the technology foundation, static analysis will report some false positives. We put a lot of effort into our product to drive this rate to a very low, acceptable limit (commonly between 5% and 25%)
• In order to address FPs, the SCAN part of our product offers a triage process that utilizes a persistent database based on defect hashing. This gives developers the option to declare a defect as FP and then never have to look at it again.

For instance, 3 weeks ago, Coverity reported 7 new defects introduced based on recent code changes, and as we can see in the various threads almost everything was fixed and committed by maintainer.
But a week after that, out of 6 new defects reported based on latest code change, 1 was ignored stating False positive, Intentional...

I hope this clarifies the issue that was discussed here.

Thanks

Coverity Scan-admin scan-admin@coverity.com
Dakshesh Vyas | Technical Manager - SCAN
Coverity | 185 Berry Street | Suite 6500, Lobby 3 | San Francisco, CA 94107
Office: 415.935.2957 | dvyas@coverity.com


________________________________________
From: linux-kernel-owner@vger.kernel.org [linux-kernel-owner@vger.kernel.org] On Behalf Of gregkh@linuxfoundation.org [gregkh@linuxfoundation.org]
Sent: Tuesday, July 10, 2012 7:45 AM
To: Venu Byravarasu
Cc: Alan Stern; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] usb: host: Fix possible kernel crash

On Tue, Jul 10, 2012 at 09:56:39AM +0530, Venu Byravarasu wrote:
> 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.

Why are you trying to "pacify" coverity, when the tool is wrong in this
case? Go poke the owners of that tool to get it to stop emitting this
false warning. Don't paper over it in the kernel. Especially for a
tool that none of us can run on our own.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2012-07-10 19:21    [W:0.087 / U:1.320 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site