1. Patchew
  2. linux
  3. View series

[PATCH] drm/repaper: fix integer overflows in repeat functions

Nikita Zhandarovich posted 1 patch 1 year ago 
drivers/gpu/drm/tiny/repaper.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] drm/repaper: fix integer overflows in repeat functions
Posted by Nikita Zhandarovich 1 year ago 
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@vger.kernel.org
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@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);
Re: [PATCH] drm/repaper: fix integer overflows in repeat functions
Posted by Alex Lanzano 11 months, 2 weeks ago 
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@vger.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@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);

Applied! Thanks for the patch.

Best regards,
Alex
Re: [PATCH] drm/repaper: fix integer overflows in repeat functions
Posted by Alex Lanzano 11 months, 3 weeks ago 
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@vger.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>

After discussing with David Laight I cleared up my concerns.

Reviewed-by: Alex Lanzano <lanzano.alex@gmail.com>

> ---
>  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);
Re: [PATCH] drm/repaper: fix integer overflows in repeat functions
Posted by Alex Lanzano 11 months, 4 weeks ago 
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@vger.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@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

Best regards,
Alex
Re: [PATCH] drm/repaper: fix integer overflows in repeat functions
Posted by David Laight 11 months, 4 weeks ago 
On Thu, 13 Feb 2025 20:54:59 -0500
Alex Lanzano <lanzano.alex@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: stable@vger.kernel.org
> > Signed-off-by: Nikita Zhandarovich <n.zhandarovich@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.
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.

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.

	David 

> 
> Best regards,
> Alex
>
Re: [PATCH] drm/repaper: fix integer overflows in repeat functions
Posted by Alex Lanzano 11 months, 4 weeks ago 
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@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: stable@vger.kernel.org
> > > Signed-off-by: Nikita Zhandarovich <n.zhandarovich@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

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
Re: [PATCH] drm/repaper: fix integer overflows in repeat functions
Posted by David Laight 11 months, 3 weeks ago 
On Fri, 14 Feb 2025 20:02:01 -0500
Alex Lanzano <lanzano.alex@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.alex@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: stable@vger.kernel.org
> > > > Signed-off-by: Nikita Zhandarovich <n.zhandarovich@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
Re: [PATCH] drm/repaper: fix integer overflows in repeat functions
Posted by Alex Lanzano 11 months, 3 weeks ago 
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.alex@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.alex@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: stable@vger.kernel.org
> > > > > Signed-off-by: Nikita Zhandarovich <n.zhandarovich@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
>
Re: [PATCH] drm/repaper: fix integer overflows in repeat functions
Posted by Nikita Zhandarovich 11 months, 4 weeks ago 
Gentle ping :)

On 1/16/25 16:48, 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@vger.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@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);
Re: [PATCH] drm/repaper: fix integer overflows in repeat functions
Posted by Noralf Trønnes 11 months, 4 weeks ago 
Adding Alex who is now the driver maintainer.


On 13.02.2025 17:57, Nikita Zhandarovich wrote:
> Gentle ping :)
> 
> On 1/16/25 16:48, 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@vger.kernel.org
>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@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);
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.