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

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

[Alexander Lobakin]

BTW, I have this in my inbox:

From: Kernel Test Robot <lkp@intel.com>
Date: Tue, 17 Oct 2023 08:14:51 +0800

> tree:   https://github.com/alobakin/linux pfcp
> head:   9183a3eb639912169a3d3e2be4f25556b465919b
> commit: c8a652cdcc0964510f108726b3da0784d1bc0cd2 [11/19] bitmap: make bitmap_{get,set}_value8() use bitmap_{read,write}()

So it happened after I converted bitmap_{get,set}_value8() so that they
use bitmap_{read,write}().

> config: x86_64-randconfig-004-20231017 (https://download.01.org/0day-ci/archive/20231017/202310170708.fJzLlgDM-lkp@intel.com/config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231017/202310170708.fJzLlgDM-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202310170708.fJzLlgDM-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
>    In file included from include/linux/cpumask.h:12,
>                     from arch/x86/include/asm/cpumask.h:5,
>                     from arch/x86/include/asm/msr.h:11,
>                     from arch/x86/include/asm/processor.h:23,
>                     from arch/x86/include/asm/cpufeature.h:5,
>                     from arch/x86/include/asm/thread_info.h:53,
>                     from include/linux/thread_info.h:60,
>                     from arch/x86/include/asm/preempt.h:9,
>                     from include/linux/preempt.h:79,
>                     from include/linux/spinlock.h:56,
>                     from include/linux/mmzone.h:8,
>                     from include/linux/gfp.h:7,
>                     from include/linux/slab.h:16,
>                     from include/linux/resource_ext.h:11,
>                     from include/linux/acpi.h:13,
>                     from drivers/gpio/gpio-pca953x.c:11:
>    drivers/gpio/gpio-pca953x.c: In function 'pca953x_probe':
>>> include/linux/bitmap.h:642:17: warning: array subscript [1, 1024] is outside array bounds of 'long unsigned int[1]' [-Warray-bounds]
>      642 |  map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
>          |                 ^~
>    In file included from include/linux/kasan-checks.h:5,
>                     from include/asm-generic/rwonce.h:26,
>                     from ./arch/x86/include/generated/asm/rwonce.h:1,
>                     from include/linux/compiler.h:246,
>                     from include/linux/build_bug.h:5,
>                     from include/linux/bits.h:21,
>                     from include/linux/ioport.h:13,
>                     from include/linux/acpi.h:12,
>                     from drivers/gpio/gpio-pca953x.c:11:
>    drivers/gpio/gpio-pca953x.c:1032:17: note: while referencing 'val'
>     1032 |  DECLARE_BITMAP(val, MAX_LINE);
>          |                 ^~~
>    include/linux/types.h:11:16: note: in definition of macro 'DECLARE_BITMAP'
>       11 |  unsigned long name[BITS_TO_LONGS(bits)]
>          |                ^~~~
>    In file included from include/linux/cpumask.h:12,
>                     from arch/x86/include/asm/cpumask.h:5,
>                     from arch/x86/include/asm/msr.h:11,
>                     from arch/x86/include/asm/processor.h:23,
>                     from arch/x86/include/asm/cpufeature.h:5,
>                     from arch/x86/include/asm/thread_info.h:53,
>                     from include/linux/thread_info.h:60,
>                     from arch/x86/include/asm/preempt.h:9,
>                     from include/linux/preempt.h:79,
>                     from include/linux/spinlock.h:56,
>                     from include/linux/mmzone.h:8,
>                     from include/linux/gfp.h:7,
>                     from include/linux/slab.h:16,
>                     from include/linux/resource_ext.h:11,
>                     from include/linux/acpi.h:13,
>                     from drivers/gpio/gpio-pca953x.c:11:
>>> include/linux/bitmap.h:642:17: warning: array subscript [1, 1024] is outside array bounds of 'long unsigned int[1]' [-Warray-bounds]
>      642 |  map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
>          |                 ^~
>    In file included from include/linux/kasan-checks.h:5,
>                     from include/asm-generic/rwonce.h:26,
>                     from ./arch/x86/include/generated/asm/rwonce.h:1,
>                     from include/linux/compiler.h:246,
>                     from include/linux/build_bug.h:5,
>                     from include/linux/bits.h:21,
>                     from include/linux/ioport.h:13,
>                     from include/linux/acpi.h:12,
>                     from drivers/gpio/gpio-pca953x.c:11:
>    drivers/gpio/gpio-pca953x.c:1032:17: note: while referencing 'val'
>     1032 |  DECLARE_BITMAP(val, MAX_LINE);
>          |                 ^~~
>    include/linux/types.h:11:16: note: in definition of macro 'DECLARE_BITMAP'
>       11 |  unsigned long name[BITS_TO_LONGS(bits)]
>          |                ^~~~
>    In file included from include/linux/cpumask.h:12,
>                     from arch/x86/include/asm/cpumask.h:5,
>                     from arch/x86/include/asm/msr.h:11,
>                     from arch/x86/include/asm/processor.h:23,
>                     from arch/x86/include/asm/cpufeature.h:5,
>                     from arch/x86/include/asm/thread_info.h:53,
>                     from include/linux/thread_info.h:60,
>                     from arch/x86/include/asm/preempt.h:9,
>                     from include/linux/preempt.h:79,
>                     from include/linux/spinlock.h:56,
>                     from include/linux/mmzone.h:8,
>                     from include/linux/gfp.h:7,
>                     from include/linux/slab.h:16,
>                     from include/linux/resource_ext.h:11,
>                     from include/linux/acpi.h:13,
>                     from drivers/gpio/gpio-pca953x.c:11:
>    include/linux/bitmap.h:643:17: warning: array subscript [1, 1024] is outside array bounds of 'long unsigned int[1]' [-Warray-bounds]
>      643 |  map[index + 1] |= (value >> space);
>          |                 ^~
>    In file included from include/linux/kasan-checks.h:5,
>                     from include/asm-generic/rwonce.h:26,
>                     from ./arch/x86/include/generated/asm/rwonce.h:1,
>                     from include/linux/compiler.h:246,
>                     from include/linux/build_bug.h:5,
>                     from include/linux/bits.h:21,
>                     from include/linux/ioport.h:13,
>                     from include/linux/acpi.h:12,
>                     from drivers/gpio/gpio-pca953x.c:11:
>    drivers/gpio/gpio-pca953x.c:1032:17: note: while referencing 'val'
>     1032 |  DECLARE_BITMAP(val, MAX_LINE);
>          |                 ^~~
>    include/linux/types.h:11:16: note: in definition of macro 'DECLARE_BITMAP'
>       11 |  unsigned long name[BITS_TO_LONGS(bits)]
>          |                ^~~~
>    In file included from include/linux/cpumask.h:12,
>                     from arch/x86/include/asm/cpumask.h:5,
>                     from arch/x86/include/asm/msr.h:11,
>                     from arch/x86/include/asm/processor.h:23,
>                     from arch/x86/include/asm/cpufeature.h:5,
>                     from arch/x86/include/asm/thread_info.h:53,
>                     from include/linux/thread_info.h:60,
>                     from arch/x86/include/asm/preempt.h:9,
>                     from include/linux/preempt.h:79,
>                     from include/linux/spinlock.h:56,
>                     from include/linux/mmzone.h:8,
>                     from include/linux/gfp.h:7,
>                     from include/linux/slab.h:16,
>                     from include/linux/resource_ext.h:11,
>                     from include/linux/acpi.h:13,
>                     from drivers/gpio/gpio-pca953x.c:11:
>    include/linux/bitmap.h:643:17: warning: array subscript [1, 1024] is outside array bounds of 'long unsigned int[1]' [-Warray-bounds]
>      643 |  map[index + 1] |= (value >> space);
>          |                 ^~
>    In file included from include/linux/kasan-checks.h:5,
>                     from include/asm-generic/rwonce.h:26,
>                     from ./arch/x86/include/generated/asm/rwonce.h:1,
>                     from include/linux/compiler.h:246,
>                     from include/linux/build_bug.h:5,
>                     from include/linux/bits.h:21,
>                     from include/linux/ioport.h:13,
>                     from include/linux/acpi.h:12,
>                     from drivers/gpio/gpio-pca953x.c:11:
>    drivers/gpio/gpio-pca953x.c:1032:17: note: while referencing 'val'
>     1032 |  DECLARE_BITMAP(val, MAX_LINE);
>          |                 ^~~
>    include/linux/types.h:11:16: note: in definition of macro 'DECLARE_BITMAP'
>       11 |  unsigned long name[BITS_TO_LONGS(bits)]
>          |                ^~~~
> 
> 
> vim +642 include/linux/bitmap.h
> 
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  603  
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  604  /**
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  605   * bitmap_write - write n-bit value within a memory region
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  606   * @map: address to the bitmap memory region
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  607   * @value: value to write, clamped to nbits
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  608   * @start: bit offset of the n-bit value
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  609   * @nbits: size of value in bits, nonzero, up to BITS_PER_LONG.
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  610   *
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  611   * bitmap_write() behaves as-if implemented as @nbits calls of __assign_bit(),
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  612   * i.e. bits beyond @nbits are ignored:
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  613   *
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  614   *   for (bit = 0; bit < nbits; bit++)
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  615   *           __assign_bit(start + bit, bitmap, val & BIT(bit));
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  616   */
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  617  static inline void bitmap_write(unsigned long *map,
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  618  				unsigned long value,
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  619  				unsigned long start, unsigned long nbits)
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  620  {
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  621  	size_t index;
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  622  	unsigned long offset;
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  623  	unsigned long space;
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  624  	unsigned long mask;
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  625  	bool fit;
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  626  
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  627  	if (unlikely(!nbits))
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  628  		return;
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  629  
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  630  	mask = BITMAP_LAST_WORD_MASK(nbits);
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  631  	value &= mask;
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  632  	offset = start % BITS_PER_LONG;
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  633  	space = BITS_PER_LONG - offset;
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  634  	fit = space >= nbits;

For that particular case, @offset is not a compile-time constant, thus
the compiler can't assume anything. @space and @fit are as well due to that.

> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  635  	index = BIT_WORD(start);
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  636  
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  637  	map[index] &= (fit ? (~(mask << offset)) : ~BITMAP_FIRST_WORD_MASK(start));
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  638  	map[index] |= value << offset;
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  639  	if (fit)
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  640  		return;

...which means the compiler doesn't know whether this condition will
return from the function or not.

> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  641  
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11 @642  	map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  643  	map[index + 1] |= (value >> space);

However, it knows that the bitmap has only 1 long and indexes above 0
are not valid.

Not sure how to approach this :z It was also captured on the version you
sent 2 weeks ago, so this could've been resolved already.

> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  644  }
> c8ac468adba7fe Syed Nayyar Waris 2023-10-11  645  
> 
> :::::: The code at line 642 was first introduced by commit
> :::::: c8ac468adba7fe930ec22f392d5f127b768deccc lib/bitmap: add bitmap_{read,write}()
> 
> :::::: TO: Syed Nayyar Waris <syednwaris@gmail.com>
> :::::: CC: Alexander Lobakin <aleksander.lobakin@intel.com>
> 

Thanks,
Olek

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

[Andy Shevchenko]

On Mon, Nov 06, 2023 at 05:31:34PM +0100, Alexander Lobakin wrote:

> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202310170708.fJzLlgDM-lkp@intel.com/

> Not sure how to approach this :z It was also captured on the version you
> sent 2 weeks ago, so this could've been resolved already.

Is it in the repository already? if so, we should revert it.
Otherwise you have time to think and fix.

-- 
With Best Regards,
Andy Shevchenko

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

[Alexander Lobakin]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: Mon, 6 Nov 2023 20:23:52 +0200

> On Mon, Nov 06, 2023 at 05:31:34PM +0100, Alexander Lobakin wrote:
> 
>>> | Reported-by: kernel test robot <lkp@intel.com>
>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202310170708.fJzLlgDM-lkp@intel.com/
> 
>> Not sure how to approach this :z It was also captured on the version you
>> sent 2 weeks ago, so this could've been resolved already.
> 
> Is it in the repository already? if so, we should revert it.
> Otherwise you have time to think and fix.

Nah, neither Alex' series nor mine. And I'd say this should rather be
resolved in the functions Alex introduce.

Thanks,
Olek

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 Mon, Nov 06, 2023 at 05:31:34PM +0100, Alexander Lobakin wrote:
> BTW, I have this in my inbox:
> 
> From: Kernel Test Robot <lkp@intel.com>
> Date: Tue, 17 Oct 2023 08:14:51 +0800
> 
> > tree:   https://github.com/alobakin/linux pfcp
> > head:   9183a3eb639912169a3d3e2be4f25556b465919b
> > commit: c8a652cdcc0964510f108726b3da0784d1bc0cd2 [11/19] bitmap: make bitmap_{get,set}_value8() use bitmap_{read,write}()
> 
> So it happened after I converted bitmap_{get,set}_value8() so that they
> use bitmap_{read,write}().
> 
> > config: x86_64-randconfig-004-20231017 (https://download.01.org/0day-ci/archive/20231017/202310170708.fJzLlgDM-lkp@intel.com/config)
> > compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231017/202310170708.fJzLlgDM-lkp@intel.com/reproduce)

[...]
 
> >      643 |  map[index + 1] |= (value >> space);
> >          |                 ^~
> >    In file included from include/linux/kasan-checks.h:5,
> >                     from include/asm-generic/rwonce.h:26,
> >                     from ./arch/x86/include/generated/asm/rwonce.h:1,
> >                     from include/linux/compiler.h:246,
> >                     from include/linux/build_bug.h:5,
> >                     from include/linux/bits.h:21,
> >                     from include/linux/ioport.h:13,
> >                     from include/linux/acpi.h:12,
> >                     from drivers/gpio/gpio-pca953x.c:11:
> >    drivers/gpio/gpio-pca953x.c:1032:17: note: while referencing 'val'

It looks like a gcc-9 false-positive. I tried gcc-12 and gcc-13, and
they both looks OK. Below is the fix that works for me.

Can you please test the following patch and add it to your series?

Thanks,
        Yury

From 2a883221ddbd18796ddef0125e9e3022a56edd7b Mon Sep 17 00:00:00 2001
From: Yury Norov <yury.norov@gmail.com>
Date: Tue, 7 Nov 2023 05:05:17 -0800
Subject: [PATCH] bitmap: suppress false-positive -Warray-bounds in
 bitmap_{read,write}

bitmap_{read,write} conditionally accesses map[index + 1], carefully
checking before that it's a safe dereference. But still, gcc-9 emits
-Warray-bounds.

Gcc-12 and gcc-13 are both OK with this code. So fix it for gcc-9 with
OPTIMIZER_HIDE_VAR().

Reported-by: Alexander Lobakin <aleksander.lobakin@intel.com>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202310170708.fJzLlgDM-lkp@intel.com/
Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 include/linux/bitmap.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 945a680816cc..0b1a07ff1080 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -592,6 +592,11 @@ static inline unsigned long bitmap_read(const unsigned long *map,
 	if (unlikely(!nbits || nbits > BITS_PER_LONG))
 		return 0;
 
+#if CONFIG_GCC_VERSION < 100000
+	/* Suppress gcc-9 false-positive -Warray-bounds */
+	OPTIMIZER_HIDE_VAR(map);
+#endif
+
 	if (space >= nbits)
 		return (map[index] >> offset) & BITMAP_LAST_WORD_MASK(nbits);
 
@@ -634,6 +639,11 @@ static inline void bitmap_write(unsigned long *map, unsigned long value,
 	fit = space >= nbits;
 	index = BIT_WORD(start);
 
+#if CONFIG_GCC_VERSION < 100000
+	/* Suppress gcc-9 false-positive -Warray-bounds */
+	OPTIMIZER_HIDE_VAR(map);
+#endif
+
 	map[index] &= (fit ? (~(mask << offset)) : ~BITMAP_FIRST_WORD_MASK(start));
 	map[index] |= value << offset;
 	if (fit)
-- 
2.39.2

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

[Alexander Potapenko]

On Tue, Nov 7, 2023 at 2:23 PM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Date: Mon, 6 Nov 2023 20:23:52 +0200
>
> > On Mon, Nov 06, 2023 at 05:31:34PM +0100, Alexander Lobakin wrote:
> >
> >>> | Reported-by: kernel test robot <lkp@intel.com>
> >>> | Closes: https://lore.kernel.org/oe-kbuild-all/202310170708.fJzLlgDM-lkp@intel.com/
> >
> >> Not sure how to approach this :z It was also captured on the version you
> >> sent 2 weeks ago, so this could've been resolved already.
> >
> > Is it in the repository already? if so, we should revert it.
> > Otherwise you have time to think and fix.
>
> Nah, neither Alex' series nor mine. And I'd say this should rather be
> resolved in the functions Alex introduce.
>
> Thanks,
> Olek

Sorry, I couldn't reproduce the problem using the instructions at
https://download.01.org/0day-ci/archive/20231017/202310170708.fJzLlgDM-lkp@intel.com/reproduce
locally, maybe that's because I only have gcc-11 and higher.

But if I'm understanding correctly what's going on, then GCC will be
reporting the same issue in the following code:

=======================================================
#include <stddef.h>
#include <stdbool.h>

#define BITS_PER_LONG 64
#define unlikely(x) x
#define UL(x) (x##UL)
#define GENMASK(h, l) \
        (((~UL(0)) - (UL(1) << (l)) + 1) & \
         (~UL(0) >> (BITS_PER_LONG - 1 - (h))))

#define BIT_WORD(nr)            ((nr) / BITS_PER_LONG)
#define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
#define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))

inline void bitmap_write(unsigned long *map,
                                unsigned long value,
                                unsigned long start, unsigned long nbits)
{
        size_t index;
        unsigned long offset;
        unsigned long space;
        unsigned long mask;
        bool fit;

        if (unlikely(!nbits))
                return;

        mask = BITMAP_LAST_WORD_MASK(nbits);
        value &= mask;
        offset = start % BITS_PER_LONG;
        space = BITS_PER_LONG - offset;
        fit = space >= nbits;
        index = BIT_WORD(start);

        map[index] &= (fit ? (~(mask << offset)) :
~BITMAP_FIRST_WORD_MASK(start));
        map[index] |= value << offset;
        if (fit)
                return;

        map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
        map[index + 1] |= (value >> space);
}

unsigned long foo(unsigned int n) {
    unsigned long bm[1] = {0};
    bitmap_write(bm, 1, n, 2);
    return bm[0];
}
=======================================================
(see also https://godbolt.org/z/GfGfYje53)

If so, the problem is not specific to GCC 9, trunk GCC also barks on this code:

=======================================================
In function 'bitmap_write',
    inlined from 'bitmap_write' at <source>:15:13,
    inlined from 'foo' at <source>:47:7:
<source>:40:12: warning: array subscript 1 is outside array bounds of
'long unsigned int[1]' [-Warray-bounds=]
   40 |         map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
      |         ~~~^~~~~~~~~~~
=======================================================

If this is true for the code in drivers/gpio/gpio-pca953x.c,
suppressing the report for GCC 9 won't help for other versions.
Given that this report is isolated in a single file, we probably need
to give the compiler some hints about the range of values passed to
bitmap_write() rather than suppressing the optimizations.

-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

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

[Alexander Lobakin]

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:
>>
>> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Date: Mon, 6 Nov 2023 20:23:52 +0200
>>
>>> On Mon, Nov 06, 2023 at 05:31:34PM +0100, Alexander Lobakin wrote:
>>>
>>>>> | Reported-by: kernel test robot <lkp@intel.com>
>>>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202310170708.fJzLlgDM-lkp@intel.com/
>>>
>>>> Not sure how to approach this :z It was also captured on the version you
>>>> sent 2 weeks ago, so this could've been resolved already.
>>>
>>> Is it in the repository already? if so, we should revert it.
>>> Otherwise you have time to think and fix.
>>
>> Nah, neither Alex' series nor mine. And I'd say this should rather be
>> resolved in the functions Alex introduce.
>>
>> Thanks,
>> Olek
> 
> Sorry, I couldn't reproduce the problem using the instructions at
> https://download.01.org/0day-ci/archive/20231017/202310170708.fJzLlgDM-lkp@intel.com/reproduce
> locally, maybe that's because I only have gcc-11 and higher.
> 
> But if I'm understanding correctly what's going on, then GCC will be
> reporting the same issue in the following code:
> 
> =======================================================
> #include <stddef.h>
> #include <stdbool.h>
> 
> #define BITS_PER_LONG 64
> #define unlikely(x) x
> #define UL(x) (x##UL)
> #define GENMASK(h, l) \
>         (((~UL(0)) - (UL(1) << (l)) + 1) & \
>          (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> 
> #define BIT_WORD(nr)            ((nr) / BITS_PER_LONG)
> #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
> #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))
> 
> inline void bitmap_write(unsigned long *map,
>                                 unsigned long value,
>                                 unsigned long start, unsigned long nbits)
> {
>         size_t index;
>         unsigned long offset;
>         unsigned long space;
>         unsigned long mask;
>         bool fit;
> 
>         if (unlikely(!nbits))
>                 return;
> 
>         mask = BITMAP_LAST_WORD_MASK(nbits);
>         value &= mask;
>         offset = start % BITS_PER_LONG;
>         space = BITS_PER_LONG - offset;
>         fit = space >= nbits;
>         index = BIT_WORD(start);
> 
>         map[index] &= (fit ? (~(mask << offset)) :
> ~BITMAP_FIRST_WORD_MASK(start));
>         map[index] |= value << offset;
>         if (fit)
>                 return;
> 
>         map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
>         map[index + 1] |= (value >> space);
> }
> 
> unsigned long foo(unsigned int n) {
>     unsigned long bm[1] = {0};
>     bitmap_write(bm, 1, n, 2);
>     return bm[0];
> }
> =======================================================
> (see also https://godbolt.org/z/GfGfYje53)
> 
> If so, the problem is not specific to GCC 9, trunk GCC also barks on this code:
> 
> =======================================================
> In function 'bitmap_write',
>     inlined from 'bitmap_write' at <source>:15:13,
>     inlined from 'foo' at <source>:47:7:
> <source>:40:12: warning: array subscript 1 is outside array bounds of
> 'long unsigned int[1]' [-Warray-bounds=]
>    40 |         map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
>       |         ~~~^~~~~~~~~~~
> =======================================================
> 
> If this is true for the code in drivers/gpio/gpio-pca953x.c,
> suppressing the report for GCC 9 won't help for other versions.
> Given that this report is isolated in a single file, we probably need

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.

Thanks,
Olek

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

[Alexander Lobakin]

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.

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

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

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

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

[Alexander Lobakin]

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

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

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

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.

Thanks,
Yury

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

[Kees Cook]

On Tue, Nov 07, 2023 at 05:44:00PM +0100, Alexander Lobakin wrote:
> 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:
> >>
> >> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >> Date: Mon, 6 Nov 2023 20:23:52 +0200
> >>
> >>> On Mon, Nov 06, 2023 at 05:31:34PM +0100, Alexander Lobakin wrote:
> >>>
> >>>>> | Reported-by: kernel test robot <lkp@intel.com>
> >>>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202310170708.fJzLlgDM-lkp@intel.com/
> >>>
> >>>> Not sure how to approach this :z It was also captured on the version you
> >>>> sent 2 weeks ago, so this could've been resolved already.
> >>>
> >>> Is it in the repository already? if so, we should revert it.
> >>> Otherwise you have time to think and fix.
> >>
> >> Nah, neither Alex' series nor mine. And I'd say this should rather be
> >> resolved in the functions Alex introduce.
> >>
> >> Thanks,
> >> Olek
> > 
> > Sorry, I couldn't reproduce the problem using the instructions at
> > https://download.01.org/0day-ci/archive/20231017/202310170708.fJzLlgDM-lkp@intel.com/reproduce
> > locally, maybe that's because I only have gcc-11 and higher.
> > 
> > But if I'm understanding correctly what's going on, then GCC will be
> > reporting the same issue in the following code:
> > 
> > =======================================================
> > #include <stddef.h>
> > #include <stdbool.h>
> > 
> > #define BITS_PER_LONG 64
> > #define unlikely(x) x
> > #define UL(x) (x##UL)
> > #define GENMASK(h, l) \
> >         (((~UL(0)) - (UL(1) << (l)) + 1) & \
> >          (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> > 
> > #define BIT_WORD(nr)            ((nr) / BITS_PER_LONG)
> > #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
> > #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))
> > 
> > inline void bitmap_write(unsigned long *map,
> >                                 unsigned long value,
> >                                 unsigned long start, unsigned long nbits)
> > {
> >         size_t index;
> >         unsigned long offset;
> >         unsigned long space;
> >         unsigned long mask;
> >         bool fit;
> > 
> >         if (unlikely(!nbits))
> >                 return;
> > 
> >         mask = BITMAP_LAST_WORD_MASK(nbits);
> >         value &= mask;
> >         offset = start % BITS_PER_LONG;
> >         space = BITS_PER_LONG - offset;
> >         fit = space >= nbits;
> >         index = BIT_WORD(start);
> > 
> >         map[index] &= (fit ? (~(mask << offset)) :
> > ~BITMAP_FIRST_WORD_MASK(start));
> >         map[index] |= value << offset;
> >         if (fit)
> >                 return;
> > 
> >         map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
> >         map[index + 1] |= (value >> space);
> > }
> > 
> > unsigned long foo(unsigned int n) {
> >     unsigned long bm[1] = {0};
> >     bitmap_write(bm, 1, n, 2);
> >     return bm[0];
> > }
> > =======================================================
> > (see also https://godbolt.org/z/GfGfYje53)
> > 
> > If so, the problem is not specific to GCC 9, trunk GCC also barks on this code:
> > 
> > =======================================================
> > In function 'bitmap_write',
> >     inlined from 'bitmap_write' at <source>:15:13,
> >     inlined from 'foo' at <source>:47:7:
> > <source>:40:12: warning: array subscript 1 is outside array bounds of
> > 'long unsigned int[1]' [-Warray-bounds=]
> >    40 |         map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
> >       |         ~~~^~~~~~~~~~~
> > =======================================================
> > 
> > If this is true for the code in drivers/gpio/gpio-pca953x.c,
> > suppressing the report for GCC 9 won't help for other versions.
> > Given that this report is isolated in a single file, we probably need
> 
> 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.

I'm trying to find all the pieces for this code, so I might be working
from the wrong version or something, but I think this is the change:
https://github.com/alobakin/linux/commit/66808fb20fed014a522b868322d54daef14a6bd8

and the induced warning is correctly analyzed in this thread (i.e. GCC
can't convince itself that it'll never reach the out of bounds access).
Does this work?

-	if (fit)
+	if (fit || index + 1 >= __member_size(map))
 		return;


-- 
Kees Cook

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 03:25:01PM -0800, Kees Cook wrote:
> On Tue, Nov 07, 2023 at 05:44:00PM +0100, Alexander Lobakin wrote:
> > 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:
> > >>
> > >> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >> Date: Mon, 6 Nov 2023 20:23:52 +0200
> > >>
> > >>> On Mon, Nov 06, 2023 at 05:31:34PM +0100, Alexander Lobakin wrote:
> > >>>
> > >>>>> | Reported-by: kernel test robot <lkp@intel.com>
> > >>>>> | Closes: https://lore.kernel.org/oe-kbuild-all/202310170708.fJzLlgDM-lkp@intel.com/
> > >>>
> > >>>> Not sure how to approach this :z It was also captured on the version you
> > >>>> sent 2 weeks ago, so this could've been resolved already.
> > >>>
> > >>> Is it in the repository already? if so, we should revert it.
> > >>> Otherwise you have time to think and fix.
> > >>
> > >> Nah, neither Alex' series nor mine. And I'd say this should rather be
> > >> resolved in the functions Alex introduce.
> > >>
> > >> Thanks,
> > >> Olek
> > > 
> > > Sorry, I couldn't reproduce the problem using the instructions at
> > > https://download.01.org/0day-ci/archive/20231017/202310170708.fJzLlgDM-lkp@intel.com/reproduce
> > > locally, maybe that's because I only have gcc-11 and higher.
> > > 
> > > But if I'm understanding correctly what's going on, then GCC will be
> > > reporting the same issue in the following code:
> > > 
> > > =======================================================
> > > #include <stddef.h>
> > > #include <stdbool.h>
> > > 
> > > #define BITS_PER_LONG 64
> > > #define unlikely(x) x
> > > #define UL(x) (x##UL)
> > > #define GENMASK(h, l) \
> > >         (((~UL(0)) - (UL(1) << (l)) + 1) & \
> > >          (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
> > > 
> > > #define BIT_WORD(nr)            ((nr) / BITS_PER_LONG)
> > > #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
> > > #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))
> > > 
> > > inline void bitmap_write(unsigned long *map,
> > >                                 unsigned long value,
> > >                                 unsigned long start, unsigned long nbits)
> > > {
> > >         size_t index;
> > >         unsigned long offset;
> > >         unsigned long space;
> > >         unsigned long mask;
> > >         bool fit;
> > > 
> > >         if (unlikely(!nbits))
> > >                 return;
> > > 
> > >         mask = BITMAP_LAST_WORD_MASK(nbits);
> > >         value &= mask;
> > >         offset = start % BITS_PER_LONG;
> > >         space = BITS_PER_LONG - offset;
> > >         fit = space >= nbits;
> > >         index = BIT_WORD(start);
> > > 
> > >         map[index] &= (fit ? (~(mask << offset)) :
> > > ~BITMAP_FIRST_WORD_MASK(start));
> > >         map[index] |= value << offset;
> > >         if (fit)
> > >                 return;
> > > 
> > >         map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
> > >         map[index + 1] |= (value >> space);
> > > }
> > > 
> > > unsigned long foo(unsigned int n) {
> > >     unsigned long bm[1] = {0};
> > >     bitmap_write(bm, 1, n, 2);
> > >     return bm[0];
> > > }
> > > =======================================================
> > > (see also https://godbolt.org/z/GfGfYje53)
> > > 
> > > If so, the problem is not specific to GCC 9, trunk GCC also barks on this code:
> > > 
> > > =======================================================
> > > In function 'bitmap_write',
> > >     inlined from 'bitmap_write' at <source>:15:13,
> > >     inlined from 'foo' at <source>:47:7:
> > > <source>:40:12: warning: array subscript 1 is outside array bounds of
> > > 'long unsigned int[1]' [-Warray-bounds=]
> > >    40 |         map[index + 1] &= BITMAP_FIRST_WORD_MASK(start + nbits);
> > >       |         ~~~^~~~~~~~~~~
> > > =======================================================
> > > 
> > > If this is true for the code in drivers/gpio/gpio-pca953x.c,
> > > suppressing the report for GCC 9 won't help for other versions.
> > > Given that this report is isolated in a single file, we probably need
> > 
> > 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.
> 
> I'm trying to find all the pieces for this code, so I might be working
> from the wrong version or something, but I think this is the change:
> https://github.com/alobakin/linux/commit/66808fb20fed014a522b868322d54daef14a6bd8

This is the series:

https://www.spinics.net/lists/kernel/msg4985590.html

The relevant part of the function is:

+	offset = start % BITS_PER_LONG;
+	space = BITS_PER_LONG - offset;
+	fit = space >= nbits; // true if offset + nbits <= BITS_PER_LONG
+	index = BIT_WORD(start);
+
+	map[index] &= XXX;
+	map[index] |= value << offset;
+	if (fit)
+		return;
+
+	map[index + 1] = YYY;
 
Some background for you:

'fit' means that the requested part of bitmap fits into a single
machine word. For example, on 64-bit machine:

        DECLARE_BITMAP(map, 64); // unsigned long val[1];
        bitmap_write(map, val, 60, 4) // fit == true
        bitmap_write(map, val, 60, 8) // fit == false

It's possible that user may overrun the array boundary, like in the
2nd case, and compiler may correctly warn about it.

But in this case...

The code in question is:

        #define bitmap_set_value8(map, value, start)           \
                bitmap_write(map, value, start, BITS_PER_BYTE)
        #define BANK_SZ 8

        for (i = 0; i < NBANK(chip); i++)
                bitmap_set_value8(val, value[i], i * BANK_SZ);

Here nbits is always 8, and start is multiple of 8. With that, we're
always writing into a single word (fit == true), and 'idx + 1' path
is never hit. This makes me think that it the warning here is false
positive. Is that correct?

> and the induced warning is correctly analyzed in this thread (i.e. GCC
> can't convince itself that it'll never reach the out of bounds access).
> Does this work?
> 
> -	if (fit)
> +	if (fit || index + 1 >= __member_size(map))
>  		return;

I already commented this in the other email in this thread - this would
silence a true warning where people break the boundary:

        bitmap_write(map, val, 60, 8);

And as far as I can see, __member_size() implies some runtime
overhead, which I'd like to avoid.

Thanks,
Yury

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

[Alexander Potapenko]

On Tue, Nov 7, 2023 at 8:24 PM Yury Norov <yury.norov@gmail.com> wrote:
>
> 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.

Yes, that's my point.
According to Godbolt, GCC versions 9.1 to 13.2 (trunk included) are
reporting the same false positive on the code I posted above.

In 5a41237ad1d4b62008f93163af1d9b1da90729d8 ("gcc: disable
-Warray-bounds for gcc-11 too") Linus says that "Older gcc versions
end up being increasingly less relevant", so I think there won't be
objections against extending this rule to GCC 9 and 10.

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

[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