On Sat, Feb 15, 2025 at 11:48:00AM +0000, David Laight wrote:
> 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.
Fair enough. I don't want to delay this patch further. Maybe down the
road this can be can be refactored a bit to add units as you said and change
types
if need be to mitigate future issues. I'll add it to my TODO list.
Best regards,
Alex
> >
> > 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
>
