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
