On Fri, Feb 14, 2025 at 01:29:10PM +0000, David Laight wrote:
> On Thu, 13 Feb 2025 20:54:59 -0500
> Alex Lanzano <lanzano.alex@xxxxxxxxx> wrote:
>
> > On Thu, Jan 16, 2025 at 05:48:01AM -0800, Nikita Zhandarovich wrote:
> > > There are conditions, albeit somewhat unlikely, under which right hand
> > > expressions, calculating the end of time period in functions like
> > > repaper_frame_fixed_repeat(), may overflow.
> > >
> > > For instance, if 'factor10x' in repaper_get_temperature() is high
> > > enough (170), as is 'epd->stage_time' in repaper_probe(), then the
> > > resulting value of 'end' will not fit in unsigned int expression.
> > >
> > > Mitigate this by casting 'epd->factored_stage_time' to wider type before
> > > any multiplication is done.
> > >
> > > Found by Linux Verification Center (linuxtesting.org) with static
> > > analysis tool SVACE.
> > >
> > > Fixes: 3589211e9b03 ("drm/tinydrm: Add RePaper e-ink driver")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/tiny/repaper.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
> > > index 77944eb17b3c..d76c0e8e05f5 100644
> > > --- a/drivers/gpu/drm/tiny/repaper.c
> > > +++ b/drivers/gpu/drm/tiny/repaper.c
> > > @@ -456,7 +456,7 @@ static void repaper_frame_fixed_repeat(struct repaper_epd *epd, u8 fixed_value,
> > > enum repaper_stage stage)
> > > {
> > > u64 start = local_clock();
> > > - u64 end = start + (epd->factored_stage_time * 1000 * 1000);
> > > + u64 end = start + ((u64)epd->factored_stage_time * 1000 * 1000);
> > >
> > > do {
> > > repaper_frame_fixed(epd, fixed_value, stage);
> > > @@ -467,7 +467,7 @@ static void repaper_frame_data_repeat(struct repaper_epd *epd, const u8 *image,
> > > const u8 *mask, enum repaper_stage stage)
> > > {
> > > u64 start = local_clock();
> > > - u64 end = start + (epd->factored_stage_time * 1000 * 1000);
> > > + u64 end = start + ((u64)epd->factored_stage_time * 1000 * 1000);
> > >
> > > do {
> > > repaper_frame_data(epd, image, mask, stage);
> >
> > It might be best to change the underlying type in the struct instead of
> > type casting
>
> That'll just make people think there is a different overflow.
The commit message should describe which overflow this applies to regardless.
> It'd also force the compiler to use a wider multiply.
>
> A more subtle approach is to change the type of the first 1000 to 1000ull.
>
My reasoning for favoring the type change route is as follows:
1. I'm not a big fan of using the standard C integer types especially
mixing them with the fixed sized kernel integer types for these kinds of
overflow scenarios
2. It would remove the chances of this field causing the same overflow
issues in future development
> Personally I like to see the units on variables containing times (x_s, _ms, _ns)
> since it makes off-by-1000 errors less likely and you can more easily tell
> whether overflow if likely.
Agreed but this is out of scope of this patch
Best regards,
Alex
