On Fri, 14 Feb 2025 20:02:01 -0500
Alex Lanzano <lanzano.a...@gmail.com> wrote:
> 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.a...@gmail.com> 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: sta...@vger.kernel.org
> > > > Signed-off-by: Nikita Zhandarovich <n.zhandarov...@fintech.ru>
> > > > ---
> > > > 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
I'm not sure whether the code is converting seconds to us or ms to ns.
But in either case 32bit is plenty for the configured timeout.
Whether that is 'unsigned int' or 'u32' doesn't really matter.
If you change the type to u64 someone is going to decide that the
multiply needs an overflow check.
OTOH use of 'unsigned long' is often an 'accident waiting to happen'.
There are far too many of them used for clock frequencies and similar.
I'm sure 'long' has been used because of worries that 'int' might be 16bit.
Even when Linux was started that was never going to be true.
David
>
> 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
