linux-kernel - Re: [PATCH] pinctrl: renesas: remove checker warnings: x

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: <869dd27f-7046-486a-83a0-acd489565083@kadam.mountain>
Date:   Tue, 13 Jun 2023 11:37:00 +0300
From:   Dan Carpenter <dan.carpenter@...aro.org>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     "GONG, Ruiqi" <gongruiqi@...weicloud.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-renesas-soc@...r.kernel.org, linux-gpio@...r.kernel.org,
        linux-kernel@...r.kernel.org, gongruiqi1@...wei.com,
        linux-sparse@...r.kernel.org
Subject: Re: [PATCH] pinctrl: renesas: remove checker warnings: x | !y

On Tue, Jun 13, 2023 at 09:38:20AM +0200, Geert Uytterhoeven wrote:
> Hi Gong,
> 
> On Tue, Jun 13, 2023 at 4:13 AM GONG, Ruiqi <gongruiqi@...weicloud.com> wrote:
> > Eliminate the following Sparse reports when building with C=1:
> >
> > drivers/pinctrl/renesas/pinctrl-rzn1.c:187:52: warning: dubious: x | !y
> > drivers/pinctrl/renesas/pinctrl-rzn1.c:193:52: warning: dubious: x | !y
> >
> > Signed-off-by: GONG, Ruiqi <gongruiqi@...weicloud.com>
> 
> Thanks for your patch!
> 
> Looks like sparse needs to be taught the "|" is not used in a boolean
> context here?
> 

I've spent some time exploring how these bugs look like but it was years
ago so I have forgotten the details.  I think the main issue is when the
! is on the left.

Bug:	if (!x & 0xf) {
Fixed:	if (!(x & 0xf)) {

Or less commonly:

Bug:	if (!x > y) {
Fixed:	if (x <= y) {

Originally Sparse used to only warn about !x & y...  I feel like Josh
maybe got a bit over enthusiastic in changing it to warn about
everything.  But that was in 2008 and we're only noticing now so maybe
it's fine.

The other bug that we see is mixing up logical and bitwise negation
but those bugs are harder to separate from good code.

> > --- a/drivers/pinctrl/renesas/pinctrl-rzn1.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzn1.c
> > @@ -184,13 +184,15 @@ static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 lock, u8 value)
> >          * address | 1.
> >          */
> >         if (lock & LOCK_LEVEL1) {
> > -               u32 val = ipctl->lev1_protect_phys | !(value & LOCK_LEVEL1);
> > +               u32 val = ipctl->lev1_protect_phys |
> > +                       (value & LOCK_LEVEL1 ? 0 : 1);

To me this code is more confusing than the original code because I
struggle to remember if & has higher precedence that ?: (It does.  The
code is fine).  But Cppcheck also thinks the code is confusing and will
print a warning:

style: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]

You would think adding another set of parentheses would silence the
sparse warning but it doesn't:

		u32 val = ipctl->lev1_protect_phys | (!(value & LOCK_LEVEL1));

regards,
dan carpenter