[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZUqCpmDgKFbscRgW@yury-ThinkPad>
Date: Tue, 7 Nov 2023 10:32:06 -0800
From: Yury Norov <yury.norov@...il.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: Alexander Potapenko <glider@...gle.com>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Syed Nayyar Waris <syednwaris@...il.com>,
Kees Cook <keescook@...omium.org>,
kernel test robot <lkp@...el.com>,
oe-kbuild-all@...ts.linux.dev, linux-hardening@...r.kernel.org,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: 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]'
On Tue, Nov 07, 2023 at 06:24:04PM +0100, Alexander Lobakin wrote:
> From: Alexander Lobakin <aleksander.lobakin@...el.com>
> Date: Tue, 7 Nov 2023 17:44:00 +0100
>
> > From: Alexander Potapenko <glider@...gle.com>
> > Date: Tue, 7 Nov 2023 17:33:56 +0100
> >
> >> On Tue, Nov 7, 2023 at 2:23 PM Alexander Lobakin
> >> <aleksander.lobakin@...el.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);
Powered by blists - more mailing lists