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]

Date:   Mon, 7 Mar 2022 18:30:53 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>
Cc:     Vincent MAILHOL <mailhol.vincent@...adoo.fr>,
        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 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. 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