linux-kernel - Re: [PATCH] staging: rtl8188eu: os_dep: usb_intf.c: Fix for possible null pointer dereference

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFo99gZ6TUdJm7Vw7=XusOujTuNLC_CkNZHU3hgxG13GVUqgRw@mail.gmail.com>
Date:	Wed, 21 May 2014 23:51:06 +0200
From:	Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	Josh Triplett <josh@...htriplett.org>,
	Larry Finger <Larry.Finger@...inger.net>,
	devel@...verdev.osuosl.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Manu Gupta <manugupt1@...il.com>
Subject: Re: [PATCH] staging: rtl8188eu: os_dep: usb_intf.c: Fix for possible
 null pointer dereference

Hi

Exactly as you say Dan, cppcheck has found that there is a null check
at some point while the others in this case are missed. So I'm not for
unnecessary checks.
But we can instead agree that I make a patch that will remove all null
checks padapter?

Dan: I'll check on the scripts/checkpatch.pl

Best regards
Rickard Strandqvist


2014-05-21 9:24 GMT+02:00 Dan Carpenter <dan.carpenter@...cle.com>:
> On Tue, May 20, 2014 at 04:57:56PM -0700, josh@...htriplett.org wrote:
>> On Tue, May 20, 2014 at 06:26:51PM -0500, Larry Finger wrote:
>> > On 05/20/2014 04:31 PM, Rickard Strandqvist wrote:
>> > >There is otherwise a risk of a possible null pointer dereference.
>> > >
>> > >Was largely found by using a static code analysis program called cppcheck.
>> > >
>> > >Signed-off-by: Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
>> > >---
>> > >  drivers/staging/rtl8188eu/os_dep/usb_intf.c |  127 ++++++++++++++-------------
>> > >  1 file changed, 66 insertions(+), 61 deletions(-)
>> >
>> > Although cppcheck's analysis is correct, pointer padapter can never
>> > be null in any of these routines. In routine rtw_drv_init(),
>> > rtw_usb_if1_init() is called to allocate memory for struct adapter
>> > for the driver. If that fails, none of these other routines will
>> > ever be called as the driver will not be loaded.
>> >
>> > If it is deemed better to fill the code with needless tests because
>> > some static checker points out a place like this, that is OK with
>> > me, but I do not see the point.
>>
>> If it's an invariant of the code that padapter cannot ever be NULL, then
>> that seems perfectly fine to rely on, and the tool just needs fixing or
>> silencing.  At most, it might be helpful to add annotations like GCC's
>> "nonnull", if that helps the checker and the compiler generate more
>> useful warnings.
>
> The tool is complaining that the existing checks for NULL don't make
> sense.  Rickard changed them to cover all the dereferences but really we
> should just remove the checks.
>
> Btw, speaking of GCC, it now silently removes inconsistent NULL
> checking.  It takes the opposite of the normall kernel programmer
> approach of adding checks everywhere.  :P  The glibc people recently
> annotated qsort() to say that the function pointer can't be NULL even
> though this "worked" in the past.  Hillarity!
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61236
>
> regards,
> dan carpenter
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/