linux-kernel - Re: [PATCH] nospec: Move array_index_nospec parameter checking into separate macro

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, 19 Feb 2018 12:47:08 +0100
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Will Deacon <will.deacon@....com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        Ingo Molnar <mingo@...hat.com>, Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH] nospec: Move array_index_nospec parameter checking into
 separate macro

Hi Will,

On Mon, Feb 5, 2018 at 3:16 PM, Will Deacon <will.deacon@....com> wrote:
> For architectures providing their own implementation of
> array_index_mask_nospec in asm/barrier.h, attempting to use WARN_ONCE to
> complain about out-of-range parameters using WARN_ON results in a mess
> of mutually-dependent include files.
>
> Rather than unpick the dependencies, simply have the core code in nospec.h
> perform the checking for us.
>
> Cc: Dan Williams <dan.j.williams@...el.com>
> Cc: Ingo Molnar <mingo@...hat.com>
> Signed-off-by: Will Deacon <will.deacon@....com>
> ---
>  include/linux/nospec.h | 36 +++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/nospec.h b/include/linux/nospec.h
> index b99bced39ac2..fbc98e2c8228 100644
> --- a/include/linux/nospec.h
> +++ b/include/linux/nospec.h
> @@ -20,20 +20,6 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
>                                                     unsigned long size)
>  {
>         /*
> -        * Warn developers about inappropriate array_index_nospec() usage.
> -        *
> -        * Even if the CPU speculates past the WARN_ONCE branch, the
> -        * sign bit of @index is taken into account when generating the
> -        * mask.
> -        *
> -        * This warning is compiled out when the compiler can infer that
> -        * @index and @size are less than LONG_MAX.
> -        */
> -       if (WARN_ONCE(index > LONG_MAX || size > LONG_MAX,
> -                       "array_index_nospec() limited to range of [0, LONG_MAX]\n"))
> -               return 0;
> -
> -       /*
>          * Always calculate and emit the mask even if the compiler
>          * thinks the mask is not needed. The compiler does not take
>          * into account the value of @index under speculation.
> @@ -44,6 +30,26 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
>  #endif
>
>  /*
> + * Warn developers about inappropriate array_index_nospec() usage.
> + *
> + * Even if the CPU speculates past the WARN_ONCE branch, the
> + * sign bit of @index is taken into account when generating the
> + * mask.
> + *
> + * This warning is compiled out when the compiler can infer that
> + * @index and @size are less than LONG_MAX.
> + */
> +#define array_index_mask_nospec_check(index, size)                             \
> +({                                                                             \
> +       if (WARN_ONCE(index > LONG_MAX || size > LONG_MAX,                      \
> +           "array_index_nospec() limited to range of [0, LONG_MAX]\n"))        \
> +               _mask = 0;                                                      \
> +       else                                                                    \
> +               _mask = array_index_mask_nospec(index, size);                   \
> +       _mask;                                                                  \
> +})
> +
> +/*
>   * array_index_nospec - sanitize an array index after a bounds check
>   *
>   * For a code sequence like:
> @@ -61,7 +67,7 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
>  ({                                                                     \
>         typeof(index) _i = (index);                                     \
>         typeof(size) _s = (size);                                       \
> -       unsigned long _mask = array_index_mask_nospec(_i, _s);          \
> +       unsigned long _mask = array_index_mask_nospec_check(_i, _s);    \
>                                                                         \
>         BUILD_BUG_ON(sizeof(_i) > sizeof(long));                        \
>         BUILD_BUG_ON(sizeof(_s) > sizeof(long));                        \

This change is commit 8fa80c503b484ddc ("nospec: Move array_index_nospec()
parameter checking into separate macro") in v4.16-rc2, and triggers the
following warning with gcc-4.1.2:

    net/wireless/nl80211.c: In function ‘parse_txq_params’:
    net/wireless/nl80211.c:2099: warning: comparison is always false
due to limited range of data type

Reverting the commit gets rid of the warning.

As kisskb hasn't completed the full build of  v4.16-rc2 yet, I don't know if
other compiler versions are affected.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds