Re: [alobakin:pfcp 11/19] include/linux/bitmap.h:642:17: warning: array subscript [1, 1024] is outside array bounds of 'long unsigned int[1]'

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Yury Norov <yury.norov@gmail.com>
Date: Tue, 7 Nov 2023 11:24:49 -0800

> On Tue, Nov 07, 2023 at 07:52:19PM +0100, Alexander Lobakin wrote:
>> From: Yury Norov <yury.norov@gmail.com>
>> Date: Tue, 7 Nov 2023 10:32:06 -0800
>>
>>> On Tue, Nov 07, 2023 at 06:24:04PM +0100, Alexander Lobakin wrote:
>>>> From: Alexander Lobakin <aleksander.lobakin@intel.com>
>>>> Date: Tue, 7 Nov 2023 17:44:00 +0100
>>>>
>>>>> From: Alexander Potapenko <glider@google.com>
>>>>> Date: Tue, 7 Nov 2023 17:33:56 +0100
>>>>>
>>>>>> On Tue, Nov 7, 2023 at 2:23 PM Alexander Lobakin
>>>>>> <aleksander.lobakin@intel.com> wrote:
>>>>
>>>> [...]
>>>>
>>>>> I tested it on GCC 9 using modified make.cross from lkp and it triggers
>>>>> on one more file:
>>>>>
>>>>> drivers/thermal/intel/intel_soc_dts_iosf.c: In function 'sys_get_curr_temp':
>>>>> ./include/linux/bitmap.h:601:18: error: array subscript [1,
>>>>> 288230376151711744] is outside array bounds of 'long unsigned int[1]'
>>>>> [-Werror=array-bounds]
>>>>>
>>>>>> to give the compiler some hints about the range of values passed to
>>>>>> bitmap_write() rather than suppressing the optimizations.
>>>>>
>>>>> OPTIMIZER_HIDE_VAR() doesn't disable optimizations if I get it
>>>>> correctly, rather shuts up the compiler in cases like this one.
>>>>>
>>>>> I've been thinking of using __member_size() from fortify-string.h, we
>>>>> could probably optimize the object code even a bit more while silencing
>>>>> this warning.
>>>>> Adding Kees, maybe he'd like to participate in sorting this out as well.
>>>>
>>>> This one seems to work. At least previously mad GCC 9.3.0 now sits
>>>> quietly, as if I added OPTIMIZER_HIDE_VAR() as Yury suggested.
>>>  
>>> What's wrong with OPTIMIZER_HIDE_VAR()? The problem is clearly on GCC
>>> side, namely - it doesn't realize that the map[index+1] fetch is
>>> conditional.
>>
>> It's totally fine for me to use it, this one is just an alternative
>> (well, a bit broken as per below).
> 
> OK, guys, that's even worse. The 12 and 13 don't fire the warning
> because Warray-bounds is explicitly disabled for gcc-11+. Check
> 0da6e5fd6c372 ("gcc: disable '-Warray-bounds' for gcc-13 too"). I'll
> test how gcc-10 builds it, and if it's broken too, it's worth to shift
> the threshold in init/Kconfig.

OPTIMIZER_HIDE_VAR() silences GCC 11 on my side (`-Warray-bounds` is
enabled via KCFLAGS), but now I'm wondering if it could possibly hide
really incorrect cases like you mentioned before (reading 8 bits
starting at 60 from a 64-bit bitmap).

> 
> Let me check it later today.
> 
> [...]
> 
>> Oh you're right, I didn't think about this. Your approach seems optimal
>> unless hardening folks have anything else.
>>
>> I don't see bitmap_{read,write}() mini-series applied anywhere in your
> 
> I'll not take the code unless there are real kernel users for it. Your
> compressor is still under development AFAIK, so I'm going to pull
> bitmap_read/write with ip_tunnel series, if it comes first. 
> 
>> tree, maybe Alex could incorporate your patch into it and resubmit?
> 
> Yes, that's what I asked him to do. But let's put it on hold while I'm
> testing different compilers.

So now I feel like it's a matter of extending the Kconfig threshold like
you said. The code is clearly valid.

Alternatively, we could trigger a build bug manually when offset, width
and map size are compile-time constants and suppress it otherwise. But
sounds pretty hacky.

> 
> Thanks,
> Yury

Thanks,
Olek