Re: [PATCH v12 4/4] gpio: xilinx: Utilize generic bitmap_get_value and _set_value

On Thu, Oct 29, 2020 at 11:44:47PM +0100, Arnd Bergmann wrote:

On Sun, Oct 18, 2020 at 11:44 PM Syed Nayyar Waris [off-list ref] wrote:

quoted

This patch reimplements the xgpio_set_multiple() function in
drivers/gpio/gpio-xilinx.c to use the new generic functions:
bitmap_get_value() and bitmap_set_value(). The code is now simpler
to read and understand. Moreover, instead of looping for each bit
in xgpio_set_multiple() function, now we can check each channel at
a time and save cycles.

This now causes -Wtype-limits warnings in linux-next with gcc-10:

Hi Arnd,

What version of gcc-10 are you running? I'm having trouble generating
these warnings so I suspect I'm using a different version than you.

Regardless I can see your concern about the code, and I think I have a
solution.

quoted

+       u32 *const state = chip->gpio_state;
+       unsigned int *const width = chip->gpio_width;
+
+       DECLARE_BITMAP(old, 64);
+       DECLARE_BITMAP(new, 64);
+       DECLARE_BITMAP(changed, 64);
+
+       spin_lock_irqsave(&chip->gpio_lock[0], flags);
+       spin_lock(&chip->gpio_lock[1]);
+
+       bitmap_set_value(old, state[0], 0, width[0]);
+       bitmap_set_value(old, state[1], width[0], width[1]);

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:22,
                 from ../arch/x86/include/asm/timex.h:5,
                 from ../include/linux/timex.h:65,
                 from ../include/linux/time32.h:13,
                 from ../include/linux/time.h:73,
                 from ../include/linux/stat.h:19,
                 from ../include/linux/module.h:13,
                 from ../drivers/gpio/gpio-xilinx.c:11:
../include/linux/bitmap.h:639:18: warning: array subscript [1,
67108864] is outside array bounds of 'long unsigned int[1]'
[-Warray-bounds]
  639 |   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:22,
                 from ../include/linux/bitops.h:6,
                 from ../drivers/gpio/gpio-xilinx.c:8:
../drivers/gpio/gpio-xilinx.c:144:17: note: while referencing 'old'
  144 |  DECLARE_BITMAP(old, 64);
      |                 ^~~
../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:22,
                 from ../arch/x86/include/asm/timex.h:5,
                 from ../include/linux/timex.h:65,
                 from ../include/linux/time32.h:13,
                 from ../include/linux/time.h:73,
                 from ../include/linux/stat.h:19,
                 from ../include/linux/module.h:13,
                 from ../drivers/gpio/gpio-xilinx.c:11:

The compiler clearly tries to do range-checking here and notices
that the index into the fixed-length array on the stack is not correctly
bounded. It seems this would happen whenever width[0] + width[1]
is larger than 64.

I have just submitted patches for all other -Wtype-limits warnings
and would like to enable this option by default. Can you try to find
a way to make this code safer? I would expect that you need a
variant of bitmap_set_value() that takes an explicit ceiling here,
and checks the stand and nbits values against that.

       Arnd

Let me first verify that I understand the problem correctly. The issue
is the possibility of a stack smash in bitmap_set_value() when the value
of start + nbits is larger than the length of the map bitmap memory
region. This is because index (or index + 1) could be outside the range
of the bitmap memory region passed in as map. Is my understanding
correct here?

In xgpio_set_multiple(), the variables width[0] and width[1] serve as
possible start and nbits values for the bitmap_set_value() calls.
Because width[0] and width[1] are unsigned int variables, GCC considers
the possibility that the value of width[0]/width[1] might exceed the
length of the bitmap memory region named old and thus result in a stack
smash.

I don't know if invalid width values are actually possible for the
Xilinx gpio device, but let's err on the side of safety and assume this
is actually a possibility. We should verify that the combined value of
gpio_width[0] + gpio_width[1] does not exceed 64 bits; we can add a
check for this in xgpio_probe() when we grab the gpio_width values.

However, we're still left with the GCC warnings because GCC is not smart
enough to know that we've already checked the boundary and width[0] and
width[1] are valid values. I suspect we can avoid this warning is we
refactor bitmap_set_value() to increment map seperately and then set it:

static inline void bitmap_set_value(unsigned long *map,
				    unsigned long value,
				    unsigned long start, unsigned long nbits)
{
	const unsigned long offset = start % BITS_PER_LONG;
	const unsigned long ceiling = round_up(start + 1, BITS_PER_LONG);
	const unsigned long space = ceiling - start;

	map += BIT_WORD(start);
	value &= GENMASK(nbits - 1, 0);

	if (space >= nbits) {
		*map &= ~(GENMASK(nbits - 1, 0) << offset);
		*map |= value << offset;
	} else {
		*map &= ~BITMAP_FIRST_WORD_MASK(start);
		*map |= value << offset;
		map++;
		*map &= ~BITMAP_LAST_WORD_MASK(start + nbits);
		*map |= value >> space;
	}
}

This avoids adding a costly conditional check inside bitmap_set_value()
when almost all bitmap_set_value() calls will have static arguments with
well-defined and obvious boundaries.

Do you think this would be an acceptable solution to resolve your GCC
warnings?

Sincerely,

William Breathitt Gray