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@xxxxxxxxx> 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:
signature.asc
Description: PGP signature
