| 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 |
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
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
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
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
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
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; }
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
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)