> -----Original Message-----
> From: Pavel Machek <pavel@xxxxxx>
> Sent: Sunday, April 07, 2019 11:46 AM
> To: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
> Cc: jacek.anaszewski@xxxxxxxxx; linux-leds@xxxxxxxxxxxxxxx; Ido Schimmel
> <idosch@xxxxxxxxxxxx>
> Subject: Re: [PATCH v1 led] leds: mlxreg: Fix UBSAN warning
>
> On Sun 2019-04-07 07:59:01, Vadim Pasternak wrote:
> > Fix the following UBSAN warning, reported undefined behavior at call
> > to
> > ror32():
> > [ 11.426543] UBSAN: Undefined behaviour in ./include/linux/bitops.h:93:33
> ...
> > The warning is caused by call to ror32(), if the second parameters of
> > this function "shift" is zero. In such case UBSAN reports the warning
> > for the next expression: (word << (32 - shift).
> > Fix adds validation of this parameter ? in case it?s equal zero, no
> > need to call rotation function, since value should stay the same.
> >
> > Fixes: 386570d76f2f ("leds: add driver for support Mellanox regmap
> > LEDs for BMC and x86 platform")
> > Reported-by: Ido Schimmel <idosch@xxxxxxxxxxxx>
> > Signed-off-by: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
>
> Actually, rotating zero bits seems pretty valid operation. Could we get that fixed,
> instead?
Hi Pavel,
UBSAN warns in case of call ror32(x, 0):
static inline __u32 ror32(__u32 word, __u32 shift)
{
return (word >> shift) | (word << (32 - shift));
}
The ror32() shift parameter zero causes rotation right of 32
for u32 type.
For example UBSAN will produce the warning for the below code:
u32 value = <some value>;
value <<= 32;
Thanks,
Vadim.
>
> Thanks,
> Pavel
>
> > ---
> > drivers/leds/leds-mlxreg.c | 18 ++++++++++++++----
> > 1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/leds/leds-mlxreg.c b/drivers/leds/leds-mlxreg.c
> > index cabe379071a7..ee6dd2a60002 100644
> > --- a/drivers/leds/leds-mlxreg.c
> > +++ b/drivers/leds/leds-mlxreg.c
> > @@ -82,8 +82,13 @@ mlxreg_led_store_hw(struct mlxreg_led_data
> *led_data, u8 vset)
> > if (ret)
> > goto access_error;
> >
> > - nib = (ror32(data->mask, data->bit) == 0xf0) ? rol32(vset, data->bit) :
> > - rol32(vset, data->bit + 4);
> > + if (data->bit)
> > + nib = (ror32(data->mask, data->bit) == 0xf0) ?
> > + rol32(vset, data->bit) : rol32(vset, data->bit + 4);
> > + else
> > + nib = (data->mask == 0xf0) ? rol32(vset, data->bit) :
> > + rol32(vset, data->bit + 4);
> > +
> > regval = (regval & data->mask) | nib;
> >
> > ret = regmap_write(led_pdata->regmap, data->reg, regval); @@ -122,8
> > +127,13 @@ mlxreg_led_get_hw(struct mlxreg_led_data *led_data)
> > }
> >
> > regval = regval & ~data->mask;
> > - regval = (ror32(data->mask, data->bit) == 0xf0) ? ror32(regval,
> > - data->bit) : ror32(regval, data->bit + 4);
> > + if (data->bit)
> > + regval = (ror32(data->mask, data->bit) == 0xf0) ? ror32(regval,
> > + data->bit) : ror32(regval, data->bit + 4);
> > + else
> > + regval = (data->mask == 0xf0) ? regval :
> > + ror32(regval, data->bit + 4);
> > +
> > if (regval >= led_data->base_color &&
> > regval <= (led_data->base_color +
> MLXREG_LED_OFFSET_BLINK_6HZ))
> > return LED_FULL;
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
