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: <e6128372-6dea-4d24-a7c7-7c3e90183836@linux.ibm.com>
Date: Wed, 11 Dec 2024 14:46:24 +0530
From: Nilay Shroff <nilay@...ux.ibm.com>
To: Nathan Chancellor <nathan@...nel.org>
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 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