linux-kernel - Re: [PATCHv3] gcc: disable '-Wstrignop-overread' universally for gcc-13+ and FORTIFY

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: <20241210161458.GA1305110@ax162>
Date: Tue, 10 Dec 2024 09:14:58 -0700
From: Nathan Chancellor <nathan@...nel.org>
To: Nilay Shroff <nilay@...ux.ibm.com>
Cc: Yury Norov <yury.norov@...il.com>, linux-kernel@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	briannorris@...omium.org, kees@...nel.org, gustavoars@...nel.org,
	steffen.klassert@...unet.com, daniel.m.jordan@...cle.com,
	gjoyce@....com, linux-crypto@...r.kernel.org, linux@...ssschuh.net
Subject: Re: [PATCHv3] gcc: disable '-Wstrignop-overread' universally for
 gcc-13+ and FORTIFY_SOURCE

On Tue, Dec 10, 2024 at 01:58:00PM +0530, Nilay Shroff wrote:
> Okay so I think you (and Greg) were suggesting instead of disabling 
> -Wstringop-overread globally or tuning it off for a particular source
> file, lets disable it on gcc-13+ while we invoke bitmap_copy() as shown
> below: 

I cannot speak for Greg but yes, this is generally what I had in mind, I
have a few comments below.

> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index d0ed9583743f..e61b9f3ff6a7 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -139,6 +139,18 @@
>  #define __diag_GCC_8(s)
>  #endif
>  
> +#if GCC_VERSION >= 130000
> +#define __diag_GCC_13(s)       __diag(s)
> +#else
> +#define __diag_GCC_13(s)
> +#endif
> +
> +#if GCC_VERSION >= 140000
> +#define __diag_GCC_14(s)       __diag(s)
> +#else
> +#define __diag_GCC_14(s)
> +#endif

You do not need to add __diag_GCC_14 because __diag_GCC_13 covers
GCC 13 and newer.

>  #define __diag_ignore_all(option, comment) \
>         __diag(__diag_GCC_ignore option)
>  
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 9278a50d514f..6885856e38b0 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -836,7 +836,23 @@ void cpumask_shift_left(struct cpumask *dstp, const struct cpumask *srcp, int n)
>  static __always_inline
>  void cpumask_copy(struct cpumask *dstp, const struct cpumask *srcp)
>  {
> +       /*
> +        * Silence -Wstringop-overead warning generated while copying cpumask
> +        * bits on gcc-13+ and CONFIG_FORTIFY_SOURCE=y. The gcc-13+ emits
> +        * warning suggesting "we're trying to copy nbits which potentially
> +        * exceeds NR_CPUS. Apparently, this seems false positive and might be
> +        * a gcc bug as we know that large_cpumask_bits should never exceed
> +        * NR_CPUS.

I think the last sentence needs to be either dropped entirely or needs
to have more assertive language. While this might be a false positive, I
think it is entirely unreasonable to expect GCC to know that
large_cpumask_bits when it is nr_cpu_ids is bounded by NR_CPUS because
it does not have the definition of nr_cpu_ids visible at this point and
even if it did, it is still a global variable, so it has to assume that
value could be anything in lieu of an explicit bounds check.

Maybe something like this for the full comment?

/*
 * Silence instances of -Wstringop-overread that come from the memcpy() in
 * bitmap_copy() that may appear with GCC 13+, CONFIG_FORTIFY_SOURCE=y, and
 * and CONFIG_NR_CPUS > 256, as the length of the memcpy() in bitmap_copy() will
 * not a compile time constant. Without an explicit bounds check on the length
 * of the copy in this path, GCC will assume the length could be 0 to UINT_MAX,
 * which would trigger an overread of the source if it were to happen. As
 * nr_cpu_ids is known to be bounded by NR_CPUS, this copy will always be in
 * bounds.
 */

> +        */
> +       __diag_push();
> +       __diag_ignore(GCC, 13, "-Wstringop-overread",
> +               "Ignore string overflow warning while copying cpumask bits");
> +       __diag_ignore(GCC, 14, "-Wstringop-overread",
> +               "Ignore string overflow warning while copying cpumask bits");

This __diag_ignore() can be dropped as well.

> +
>         bitmap_copy(cpumask_bits(dstp), cpumask_bits(srcp), large_cpumask_bits);
> +
> +       __diag_pop();
>  }
> 
> Does the above change look good to everyone?

I think this seems reasonable to me, but it might be good to get some
feedback from the hardening folks.

Cheers,
Nathan