linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH][next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int
@ 2021-05-13  8:52 Colin King
  2021-05-13  9:10 ` David Laight
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Colin King @ 2021-05-13  8:52 UTC (permalink / raw)
  To: Shubhrajyoti Datta, Srinivas Neeli, Michal Simek, Linus Walleij,
	Bartosz Golaszewski, Andy Shevchenko, linux-gpio,
	linux-arm-kernel
  Cc: kernel-janitors, linux-kernel

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;
 }
 
 static inline int xgpio_regoffset(struct xgpio_instance *chip, int ch)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* RE: [PATCH][next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int
  2021-05-13  8:52 [PATCH][next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int Colin King
@ 2021-05-13  9:10 ` David Laight
  2021-05-17  7:04   ` Andy Shevchenko
  2021-05-14  5:37 ` Dan Carpenter
  2021-05-17  7:03 ` Andy Shevchenko
  2 siblings, 1 reply; 9+ messages in thread
From: David Laight @ 2021-05-13  9:10 UTC (permalink / raw)
  To: 'Colin King',
	Shubhrajyoti Datta, Srinivas Neeli, Michal Simek, Linus Walleij,
	Bartosz Golaszewski, Andy Shevchenko, linux-gpio,
	linux-arm-kernel
  Cc: kernel-janitors, linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int
  2021-05-13  8:52 [PATCH][next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int Colin King
  2021-05-13  9:10 ` David Laight
@ 2021-05-14  5:37 ` Dan Carpenter
  2021-05-17  7:07   ` Andy Shevchenko
  2021-05-17  7:03 ` Andy Shevchenko
  2 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2021-05-14  5:37 UTC (permalink / raw)
  To: Colin King
  Cc: Shubhrajyoti Datta, Srinivas Neeli, Michal Simek, Linus Walleij,
	Bartosz Golaszewski, Andy Shevchenko, linux-gpio,
	linux-arm-kernel, kernel-janitors, linux-kernel

On Thu, May 13, 2021 at 09:52:27AM +0100, Colin King wrote:
> 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;

Doing a shift by BIT(5) is super weird.  It looks like a double shift
bug and should probably trigger a static checker warning.  It's like
when people do BIT(BIT(5)).

It would be more readable to write it as:

	int shift = (bit % BITS_PER_LONG) ? 32 : 0;

regards,
dan carpenter


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int
  2021-05-13  8:52 [PATCH][next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int Colin King
  2021-05-13  9:10 ` David Laight
  2021-05-14  5:37 ` Dan Carpenter
@ 2021-05-17  7:03 ` Andy Shevchenko
  2021-05-17  7:32   ` Andy Shevchenko
  2 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2021-05-17  7:03 UTC (permalink / raw)
  To: Colin King
  Cc: Shubhrajyoti Datta, Srinivas Neeli, Michal Simek, Linus Walleij,
	Bartosz Golaszewski, Andy Shevchenko, open list:GPIO SUBSYSTEM,
	linux-arm Mailing List, kernel-janitors,
	Linux Kernel Mailing List

On Thu, May 13, 2021 at 12:12 PM Colin King <colin.king@canonical.com> wrote:
>
> 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")

No, it is a false positive,

>         const unsigned long offset = (bit % BITS_PER_LONG) & BIT(5);

See above, offset is 0 when BITS_PER_LONG == 32 and 32 when it's equal to 64.

> -       map[index] |= v << offset;
> +       map[index] |= (unsigned long)v << offset;

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int
  2021-05-13  9:10 ` David Laight
@ 2021-05-17  7:04   ` Andy Shevchenko
  0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2021-05-17  7:04 UTC (permalink / raw)
  To: David Laight
  Cc: Colin King, Shubhrajyoti Datta, Srinivas Neeli, Michal Simek,
	Linus Walleij, Bartosz Golaszewski, Andy Shevchenko, linux-gpio,
	linux-arm-kernel, kernel-janitors, linux-kernel

On Thu, May 13, 2021 at 1:04 PM David Laight <David.Laight@aculab.com> wrote:
> > From: Colin Ian King <colin.king@canonical.com>

> > @@ -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.

Can you please be more specific on which code is dubious on 32-bit
arches and why?

> 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[].

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int
  2021-05-14  5:37 ` Dan Carpenter
@ 2021-05-17  7:07   ` Andy Shevchenko
  2021-05-17 13:36     ` Dan Carpenter
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2021-05-17  7:07 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Colin King, Shubhrajyoti Datta, Srinivas Neeli, Michal Simek,
	Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
	open list:GPIO SUBSYSTEM, linux-arm Mailing List,
	kernel-janitors, Linux Kernel Mailing List

On Fri, May 14, 2021 at 12:26 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Thu, May 13, 2021 at 09:52:27AM +0100, Colin King wrote:

...

> >       const unsigned long offset = (bit % BITS_PER_LONG) & BIT(5);
> >
> >       map[index] &= ~(0xFFFFFFFFul << offset);
> > -     map[index] |= v << offset;
> > +     map[index] |= (unsigned long)v << offset;
>
> Doing a shift by BIT(5) is super weird.

Not the first place in the kernel with such a trick.

>  It looks like a double shift
> bug and should probably trigger a static checker warning.  It's like
> when people do BIT(BIT(5)).
>
> It would be more readable to write it as:
>
>         int shift = (bit % BITS_PER_LONG) ? 32 : 0;

Usually this code is in a kinda fast path. Have you checked if the
compiler generates the same or better code when you are using ternary?

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int
  2021-05-17  7:03 ` Andy Shevchenko
@ 2021-05-17  7:32   ` Andy Shevchenko
  0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2021-05-17  7:32 UTC (permalink / raw)
  To: Colin King
  Cc: Shubhrajyoti Datta, Srinivas Neeli, Michal Simek, Linus Walleij,
	Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
	linux-arm Mailing List, kernel-janitors,
	Linux Kernel Mailing List

On Mon, May 17, 2021 at 10:03:15AM +0300, Andy Shevchenko wrote:
> On Thu, May 13, 2021 at 12:12 PM Colin King <colin.king@canonical.com> wrote:
> >
> > 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")
> 
> No, it is a false positive,
> 
> >         const unsigned long offset = (bit % BITS_PER_LONG) & BIT(5);
> 
> See above, offset is 0 when BITS_PER_LONG == 32 and 32 when it's equal to 64.

Should be read as "...and 0 or 32 when..."

> > -       map[index] |= v << offset;
> > +       map[index] |= (unsigned long)v << offset;

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int
  2021-05-17  7:07   ` Andy Shevchenko
@ 2021-05-17 13:36     ` Dan Carpenter
  2021-05-17 13:49       ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2021-05-17 13:36 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Colin King, Shubhrajyoti Datta, Srinivas Neeli, Michal Simek,
	Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
	open list:GPIO SUBSYSTEM, linux-arm Mailing List,
	kernel-janitors, Linux Kernel Mailing List

On Mon, May 17, 2021 at 10:07:20AM +0300, Andy Shevchenko wrote:
> On Fri, May 14, 2021 at 12:26 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Thu, May 13, 2021 at 09:52:27AM +0100, Colin King wrote:
> 
> ...
> 
> > >       const unsigned long offset = (bit % BITS_PER_LONG) & BIT(5);
> > >
> > >       map[index] &= ~(0xFFFFFFFFul << offset);
> > > -     map[index] |= v << offset;
> > > +     map[index] |= (unsigned long)v << offset;
> >
> > Doing a shift by BIT(5) is super weird.
> 
> Not the first place in the kernel with such a trick.
> 
> >  It looks like a double shift
> > bug and should probably trigger a static checker warning.  It's like
> > when people do BIT(BIT(5)).
> >
> > It would be more readable to write it as:
> >
> >         int shift = (bit % BITS_PER_LONG) ? 32 : 0;
> 
> Usually this code is in a kinda fast path. Have you checked if the
> compiler generates the same or better code when you are using ternary?

I wrote a little benchmark to see which was faster and they're the same
as far as I can see.

regards,
dan carpenter

static inline __attribute__((__gnu_inline__)) unsigned long xgpio_set_value_orig(unsigned long *map, int bit, u32 v)
{
        int shift = (bit % 64) & ((((1UL))) << (5));
        return v << shift;
}

static inline __attribute__((__gnu_inline__)) unsigned long xgpio_set_value_new(unsigned long *map, int bit, u32 v)
{
        int shift = (bit % 64) ? 32 : 0;
        return v << shift;
}

int main(void)
{
        int i;

        for (i = 0; i < INT_MAX; i++)
                xgpio_set_value_orig(NULL, i, 0);

//      for (i = 0; i < INT_MAX; i++)
//              xgpio_set_value_new(NULL, i, 0);

        return 0;
}


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int
  2021-05-17 13:36     ` Dan Carpenter
@ 2021-05-17 13:49       ` Andy Shevchenko
  0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2021-05-17 13:49 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Colin King, Shubhrajyoti Datta, Srinivas Neeli, Michal Simek,
	Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
	linux-arm Mailing List, kernel-janitors,
	Linux Kernel Mailing List

On Mon, May 17, 2021 at 04:36:43PM +0300, Dan Carpenter wrote:
> On Mon, May 17, 2021 at 10:07:20AM +0300, Andy Shevchenko wrote:
> > On Fri, May 14, 2021 at 12:26 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > On Thu, May 13, 2021 at 09:52:27AM +0100, Colin King wrote:
> > 
> > ...
> > 
> > > >       const unsigned long offset = (bit % BITS_PER_LONG) & BIT(5);
> > > >
> > > >       map[index] &= ~(0xFFFFFFFFul << offset);
> > > > -     map[index] |= v << offset;
> > > > +     map[index] |= (unsigned long)v << offset;
> > >
> > > Doing a shift by BIT(5) is super weird.
> > 
> > Not the first place in the kernel with such a trick.
> > 
> > >  It looks like a double shift
> > > bug and should probably trigger a static checker warning.  It's like
> > > when people do BIT(BIT(5)).
> > >
> > > It would be more readable to write it as:
> > >
> > >         int shift = (bit % BITS_PER_LONG) ? 32 : 0;
> > 
> > Usually this code is in a kinda fast path. Have you checked if the
> > compiler generates the same or better code when you are using ternary?
> 
> I wrote a little benchmark to see which was faster and they're the same
> as far as I can see.

Thanks for checking.

Besides the fact that offset should be 0 for 32-bit always and if compiler can
proof that...

The test below doesn't take into account the exact trick is used for offset
(i.e. implicit dependency between BITS_PER_LONG, size of unsigned long, and
 using 5th bit out of value). I don't know if compiler can properly optimize
the ternary in this case (but it looks like it should generate the same code).

That said, I would rather to see the diff between assembly of the exact
function before and after your proposal.

> static inline __attribute__((__gnu_inline__)) unsigned long xgpio_set_value_orig(unsigned long *map, int bit, u32 v)
> {
>         int shift = (bit % 64) & ((((1UL))) << (5));
>         return v << shift;
> }
> 
> static inline __attribute__((__gnu_inline__)) unsigned long xgpio_set_value_new(unsigned long *map, int bit, u32 v)
> {
>         int shift = (bit % 64) ? 32 : 0;
>         return v << shift;
> }
> 
> int main(void)
> {
>         int i;
> 
>         for (i = 0; i < INT_MAX; i++)
>                 xgpio_set_value_orig(NULL, i, 0);
> 
> //      for (i = 0; i < INT_MAX; i++)
> //              xgpio_set_value_new(NULL, i, 0);
> 
>         return 0;
> }
> 

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-05-17 13:51 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-13  8:52 [PATCH][next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int Colin King
2021-05-13  9:10 ` David Laight
2021-05-17  7:04   ` Andy Shevchenko
2021-05-14  5:37 ` Dan Carpenter
2021-05-17  7:07   ` Andy Shevchenko
2021-05-17 13:36     ` Dan Carpenter
2021-05-17 13:49       ` Andy Shevchenko
2021-05-17  7:03 ` Andy Shevchenko
2021-05-17  7:32   ` Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).