Messages in this thread | | | Date | Wed, 8 Nov 2023 13:28:52 +0100 | 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]' | From | Alexander Lobakin <> |
| |
From: Yury Norov <yury.norov@gmail.com> Date: Tue, 7 Nov 2023 11:24:49 -0800
> On Tue, Nov 07, 2023 at 07:52:19PM +0100, Alexander Lobakin wrote: >> 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). > > OK, guys, that's even worse. The 12 and 13 don't fire the warning > because Warray-bounds is explicitly disabled for gcc-11+. Check > 0da6e5fd6c372 ("gcc: disable '-Warray-bounds' for gcc-13 too"). I'll > test how gcc-10 builds it, and if it's broken too, it's worth to shift > the threshold in init/Kconfig.
OPTIMIZER_HIDE_VAR() silences GCC 11 on my side (`-Warray-bounds` is enabled via KCFLAGS), but now I'm wondering if it could possibly hide really incorrect cases like you mentioned before (reading 8 bits starting at 60 from a 64-bit bitmap).
> > Let me check it later today. > > [...] > >> 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 > > I'll not take the code unless there are real kernel users for it. Your > compressor is still under development AFAIK, so I'm going to pull > bitmap_read/write with ip_tunnel series, if it comes first. > >> tree, maybe Alex could incorporate your patch into it and resubmit? > > Yes, that's what I asked him to do. But let's put it on hold while I'm > testing different compilers.
So now I feel like it's a matter of extending the Kconfig threshold like you said. The code is clearly valid.
Alternatively, we could trigger a build bug manually when offset, width and map size are compile-time constants and suppress it otherwise. But sounds pretty hacky.
> > Thanks, > Yury
Thanks, Olek
| |