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
2023-11-07T18:32:13Z

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.

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