From: Yury Norov <[email protected]> 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 <[email protected]> >> Date: Tue, 7 Nov 2023 17:44:00 +0100 >> >>> From: Alexander Potapenko <[email protected]> >>> Date: Tue, 7 Nov 2023 17:33:56 +0100 >>> >>>> On Tue, Nov 7, 2023 at 2:23 PM Alexander Lobakin >>>> <[email protected]> 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
