linux-kernel - Re: [PATCH] staging: wilc1000: Remove unnecessary pointer check

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: <20180921053911.GA5170@flashbox>
Date:   Thu, 20 Sep 2018 22:39:11 -0700
From:   Nathan Chancellor <natechancellor@...il.com>
To:     valdis.kletnieks@...edu
Cc:     Aditya Shankar <aditya.shankar@...rochip.com>,
        Ganesh Krishna <ganesh.krishna@...rochip.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-wireless@...r.kernel.org, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: wilc1000: Remove unnecessary pointer check

On Fri, Sep 21, 2018 at 01:25:32AM -0400, valdis.kletnieks@...edu wrote:
> On Thu, 20 Sep 2018 14:26:49 -0700, Nathan Chancellor said:
> > Clang warns that the address of a pointer will always evaluated as true
> > in a boolean context:
> >
> > drivers/staging/wilc1000/linux_wlan.c:267:20: warning: address of
> > 'vif->ndev->dev' will always evaluate to 'true'
> > [-Wpointer-bool-conversion]
> >         if (!(&vif->ndev->dev))
> >             ~  ~~~~~~~~~~~^~~
> > 1 warning generated.
> >
> > Since this statement always evaluates to false due to the logical not,
> > remove it.
> 
> Often, "just nuke it because it's now dead code" isn't the best answer...
> 
> At one time, that was likely intended to be checking whether ->dev was a null
> pointer, to make sure we don't pass request_firmware() a null pointer and oops
> the kernel, or other things that go pear-shaped....
> 
> So the question becomes:   Is it safe to just remove it, or was it intended to
> test for something that could  legitimately be null if we've hit an error along
> the way (which means we should fix the condition to be proper and acceptable
> to both gcc and clang)?
> 
> 

I certainly considered whether or not removing the check versus fixing
it was the correct answer. Given that this check can be traced back to
the initial check in of the driver in 2015, I figured it was safe to
remove it (since a null pointer dereference would most likely have been
noticed by now).

Most patches addressing this warning just remove the check given that it's
not actually changing the code, such as commit a7dc662c6a7b ("ASoC: codecs:
PCM1789: unconditionally flush work"). However, if the driver authors and/or
maintainers think that this check should be something else (maybe checking
that the contents of dev is not null versus the address, I'm perfectly
happy to submit a v2 with this change.

Thank you for the response and review!
Nathan