Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <cover.1603055402.git.syednwaris@gmail.com> <15a044d3ba23f00c31fd09437bdd3e5924bb91cd.1603055402.git.syednwaris@gmail.com>
In-Reply-To: <15a044d3ba23f00c31fd09437bdd3e5924bb91cd.1603055402.git.syednwaris@gmail.com>
From:   Arnd Bergmann <arnd@kernel.org>
Date:   Thu, 29 Oct 2020 23:44:47 +0100
Message-ID: <CAK8P3a3f=fuq24QwNee3QgoMcSK5rcvLRpdTOWBZ9NJ4d-4bvA@mail.gmail.com>
Subject: Re: [PATCH v12 4/4] gpio: xilinx: Utilize generic bitmap_get_value
 and _set_value
To:     Syed Nayyar Waris <syednwaris@gmail.com>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        William Breathitt Gray <vilhelm.gray@gmail.com>,
        Michal Simek <michal.simek@xilinx.com>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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:

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