[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
Related show

Commit Message

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

Dan Carpenter May 14, 2021, 5:37 a.m. | #1
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. | #2
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
Andy Shevchenko May 17, 2021, 7:04 a.m. | #3
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
Andy Shevchenko May 17, 2021, 7:07 a.m. | #4
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
andriy.shevchenko@linux.intel.com May 17, 2021, 7:32 a.m. | #5
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
Dan Carpenter May 17, 2021, 1:36 p.m. | #6
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. | #7
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

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)