linux-kernel - Re: [PATCH v12 4/4] gpio: xilinx: Utilize generic bitmap_get_value and _set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK8P3a3f=fuq24QwNee3QgoMcSK5rcvLRpdTOWBZ9NJ4d-4bvA@mail.gmail.com>
Date:   Thu, 29 Oct 2020 23:44:47 +0100
From:   Arnd Bergmann <arnd@...nel.org>
To:     Syed Nayyar Waris <syednwaris@...il.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        William Breathitt Gray <vilhelm.gray@...il.com>,
        Michal Simek <michal.simek@...inx.com>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v12 4/4] gpio: xilinx: Utilize generic bitmap_get_value
 and _set_value

On Sun, Oct 18, 2020 at 11:44 PM Syed Nayyar Waris <syednwaris@...il.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:

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