linux-kernel - Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT

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: <CAMZ6RqKPicgiU15g3xkFD15-TVsSYTyxNqkCEYVn2EpzrmJcNw@mail.gmail.com>
Date:   Tue, 8 Mar 2022 21:20:32 +0900
From:   Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        Arnd Bergmann <arnd@...db.de>,
        Rikard Falkeborn <rikard.falkeborn@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK()

On Tue. 8 Mar 2022 à 01:30, Andy Shevchenko <andy.shevchenko@...il.com> wrote:
> On Mon, Mar 7, 2022 at 5:10 PM Alexander Lobakin
> <alexandr.lobakin@...el.com> wrote:
> > From: Vincent MAILHOL <mailhol.vincent@...adoo.fr>
> > Date: Mon, 7 Mar 2022 22:50:56 +0900
>
> ...
>
> > For example, people tend to make the following mistake:
> >
> >         unsigned int i;
> >
> >         for (i = 0; i ...) {
> >                 ret = setup_something(array[i]);
> >                 if (ret)
> >                         goto unroll;
> >         }
> >
> > unroll:
> >         while (--i)
> >                 unroll_something(array[i]);
> >
> > The loop will never end as `i` was declared as unsigned.
> > -Wtype-limits catches this.
>
> This looks like a wrapping value issue, not sure if the type limits
> makes logical sense. What I'm saying is that the waning is
> controversial. It may help or it may make noise.
>
> > Not speaking of checking unsigned variables on < 0:
> >
> >         unsigned int num;
> >
> >         /* calculate_something() returns the number of something
> >          * or -ERRNO in case of an error
> >          */
> >         num = calculate_something();
> >         if (num < 0)
> >                 ...
>
> Depends on the context. Here is a mistake, but there are plenty of
> cases when it's okay to do so.

I am curious to see which case you are thinking of.

Personally, I see two cases, both with macro:

  1/ Cases similar to GENMASK_INPUT_CHECK() in which the macro is
  made for generic purpose and in which there was no way to know
  in advance that one parameter would be unsigned and the other
  zero.

  2/ Comparaison again a macro which might or might not be
  zero. e.g.:

#ifdef FOO
#define BAR 42
#else
#define BAR 0
#endif

void baz(void)
{
        unsigned int i;

        if (i > BAR) /* yields -Wtype-limits if FOO is not defined. */
}

And because of those two false positives, moving it to W=2 was a
wise choice.

But I am not aware of any use cases outside of macro where doing
an:

unsigned int num;
/* ... */
if (num < 0)

would be okay. At best it is dead code, at worse, it is a bug as
pointed out by Alexander.

I am not sure what I am missing here.

> And in the above the variable name is
> misleading with its semantics, The proper code should be
>
>   unsigned int num;
>   int ret;
>
>   ret = ...
>   if (ret < 0)
>     ...
>   num = ret;
>
> Again, the warning is controversial in my opinion.
>
> --
> With Best Regards,
> Andy Shevchenko