RE: [PATCH][next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int

From: David Laight <David.Laight@ACULAB.COM>
To: 'Colin King' <colin.king@canonical.com>,
	Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>,
	Srinivas Neeli <srinivas.neeli@xilinx.com>,
	Michal Simek <michal.simek@xilinx.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Cc: "kernel-janitors@vger.kernel.org"
	<kernel-janitors@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH][next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int
Date: Thu, 13 May 2021 09:10:07 +0000	[thread overview]
Message-ID: <ca5918c4d9a240bc80bad1ad16d929f9@AcuMS.aculab.com> (raw)
In-Reply-To: <20210513085227.54392-1-colin.king@canonical.com>

From: Colin King
> Sent: 13 May 2021 09:52
> 
> From: Colin Ian King <colin.king@canonical.com>
> 
> The left shift of the u32 integer v is evaluated using 32 bit
> arithmetic and then assigned to a u64 integer. There are cases
> where v will currently overflow on the shift. Avoid this by
> casting it to unsigned long (same type as map[]) before shifting
> it.
> 
> Addresses-Coverity: ("Unintentional integer overflow")
> Fixes: 02b3f84d9080 ("gpio: xilinx: Switch to use bitmap APIs")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/gpio/gpio-xilinx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
> index 109b32104867..164a3a5a9393 100644
> --- a/drivers/gpio/gpio-xilinx.c
> +++ b/drivers/gpio/gpio-xilinx.c
> @@ -99,7 +99,7 @@ static inline void xgpio_set_value32(unsigned long *map, int bit, u32 v)
>  	const unsigned long offset = (bit % BITS_PER_LONG) & BIT(5);
> 
>  	map[index] &= ~(0xFFFFFFFFul << offset);
> -	map[index] |= v << offset;
> +	map[index] |= (unsigned long)v << offset;
>  }

That code looks dubious on 32bit architectures.

I don't have 02b3f84d9080 in any of my source trees.
But that patch may itself be very dubious.

Since the hardware requires explicit bits be set, relying
on the bitmap functions seems pointless and possibly wrong.
Clearly they cause additional problems because they use long[]
and here the code needs u32[].

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel