On 12/10/24 21:44, Nathan Chancellor wrote:
> 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.
Yeah ok, I would remove __diag_GCC_14.
>
>> #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.
> */
Okay I would update comment.
>
>> + */
>> + __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.
Agreed.
>
>> +
>> 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
Thanks,
--Nilay
