diff mbox series

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

Message ID 20210513085227.54392-1-colin.king@canonical.com
State New
Headers show
Series [next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int | expand

Commit Message

Colin Ian King May 13, 2021, 8:52 a.m. UTC 
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(-)

Comments

David Laight May 13, 2021, 9:10 a.m. UTC | #1 
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)
Dan Carpenter May 14, 2021, 5:37 a.m. UTC | #2 
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
Andy Shevchenko May 17, 2021, 7:03 a.m. UTC | #3 
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;
Andy Shevchenko May 17, 2021, 7:04 a.m. UTC | #4 
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[].
Andy Shevchenko May 17, 2021, 7:07 a.m. UTC | #5 
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?
Andy Shevchenko May 17, 2021, 7:32 a.m. UTC | #6 
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;
Dan Carpenter May 17, 2021, 1:36 p.m. UTC | #7 
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;
}
Andy Shevchenko May 17, 2021, 1:49 p.m. UTC | #8 
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;
> }
>
diff mbox series

Patch

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)