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

[William Breathitt Gray <vilhelm.gray@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6544 bytes --]

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 <syednwaris@gmail.com> wrote:
> >
> > 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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]