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]'

[Yury Norov]

On Tue, Nov 07, 2023 at 06:24:04PM +0100, Alexander Lobakin wrote:
> From: Alexander Lobakin <aleksander.lobakin@xxxxxxxxx>
> Date: Tue, 7 Nov 2023 17:44:00 +0100
> 
> > From: Alexander Potapenko <glider@xxxxxxxxxx>
> > Date: Tue, 7 Nov 2023 17:33:56 +0100
> > 
> >> On Tue, Nov 7, 2023 at 2:23 PM Alexander Lobakin
> >> <aleksander.lobakin@xxxxxxxxx> 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.

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

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

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