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 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).

> 
> And moreover, it's fixed in later stable builds. I tested 12 and 13,
> and both are silent.

Yeah, this happens only on GCC 9 on my side. I thought on older GCCs
`-Warray-bounds` is not enabled due to false-positives.

> 
>> Note that ideally @map should be marked as `POS` in both cases to help
>> Clang, but `POS` gets undefined at the end of fortify-string.h, so I
>> decided to not do that within this draft.
>>
>> Thanks,
>> Olek
>> ---
>> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
>> index e8031a157db5..efa0a0287d7c 100644
>> --- a/include/linux/bitmap.h
>> +++ b/include/linux/bitmap.h
>> @@ -589,12 +589,14 @@ static inline unsigned long bitmap_read(const
>> unsigned long *map,
>>  	size_t index = BIT_WORD(start);
>>  	unsigned long offset = start % BITS_PER_LONG;
>>  	unsigned long space = BITS_PER_LONG - offset;
>> +	const size_t map_size = __member_size(map);
>>  	unsigned long value_low, value_high;
>>
>>  	if (unlikely(!nbits || nbits > BITS_PER_LONG))
>>  		return 0;
>>
>> -	if (space >= nbits)
>> +	if ((__builtin_constant_p(map_size) && map_size != SIZE_MAX &&
>> +	     index + 1 >= map_size / sizeof(long)) || space >= nbits)
>>  		return (map[index] >> offset) & BITMAP_LAST_WORD_MASK(nbits);
> 
> This silences the compiler, but breaks the code logic and hides potential bugs.
> After the fix, the following code will become legit:
> 
>         DECLARE_BITMAP(bitmap, 64);
> 
>         bitmap_fill(bitmap, 64)
>         char ret = bitmap_read(bitmap, 60, 8); // OK, return 0b00001111
> 
> Before this change, the return value would be undef: 0xXXXX1111, and
> it would (should) trigger Warray-bounds on compile time, because it's
> a compile-time boundary violation.

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
tree, maybe Alex could incorporate your patch into it and resubmit?

> 
> On runtime KASAN, UBSAN and whatever *SAN would most likely be silenced
> too with your fix. So no, this one doesn't seem to work.
> 
>>  	value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
>> @@ -620,6 +622,7 @@ static inline unsigned long bitmap_read(const
>> unsigned long *map,
>>  static inline void bitmap_write(unsigned long *map, unsigned long value,
>>  				unsigned long start, unsigned long nbits)
>>  {
>> +	const size_t map_size = __member_size(map);
>>  	size_t index;
>>  	unsigned long offset;
>>  	unsigned long space;
>> @@ -638,7 +641,9 @@ static inline void bitmap_write(unsigned long *map,
>> unsigned long value,
>>
>>  	map[index] &= (fit ? (~(mask << offset)) :
>> ~BITMAP_FIRST_WORD_MASK(start));
>>  	map[index] |= value << offset;
>> -	if (fit)
>> +
>> +	if ((__builtin_constant_p(map_size) && map_size != SIZE_MAX &&
>> +	     index + 1 >= map_size / sizeof(long)) || fit)
>>  		return;
>>
>>  	map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);

Thanks,
Olek