[PATCH] comedi: drivers: ni_tio: Fix arithmetic expression overflow

[PATCH] comedi: drivers: ni_tio: Fix arithmetic expression overflow

[Denis Arefev]

The value of an arithmetic expression period_ns * 1000 is subject
to overflow due to a failure to cast operands to a larger data
type before performing arithmetic

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Denis Arefev <arefev@swemel.ru>
---
 drivers/comedi/drivers/ni_tio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/comedi/drivers/ni_tio.c b/drivers/comedi/drivers/ni_tio.c
index da6826d77e60..acc914903c70 100644
--- a/drivers/comedi/drivers/ni_tio.c
+++ b/drivers/comedi/drivers/ni_tio.c
@@ -800,7 +800,7 @@ static int ni_tio_set_clock_src(struct ni_gpct *counter,
 				GI_PRESCALE_X2(counter_dev->variant) |
 				GI_PRESCALE_X8(counter_dev->variant), bits);
 	}
-	counter->clock_period_ps = period_ns * 1000;
+	counter->clock_period_ps = period_ns * 1000UL;
 	ni_tio_set_sync_mode(counter);
 	return 0;
 }
-- 
2.25.1

Re: [PATCH] comedi: drivers: ni_tio: Fix arithmetic expression overflow

[Ian Abbott]

On 17/01/2024 12:42, Denis Arefev wrote:
> The value of an arithmetic expression period_ns * 1000 is subject
> to overflow due to a failure to cast operands to a larger data
> type before performing arithmetic
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Denis Arefev <arefev@swemel.ru>
> ---
>   drivers/comedi/drivers/ni_tio.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/comedi/drivers/ni_tio.c b/drivers/comedi/drivers/ni_tio.c
> index da6826d77e60..acc914903c70 100644
> --- a/drivers/comedi/drivers/ni_tio.c
> +++ b/drivers/comedi/drivers/ni_tio.c
> @@ -800,7 +800,7 @@ static int ni_tio_set_clock_src(struct ni_gpct *counter,
>   				GI_PRESCALE_X2(counter_dev->variant) |
>   				GI_PRESCALE_X8(counter_dev->variant), bits);
>   	}
> -	counter->clock_period_ps = period_ns * 1000;
> +	counter->clock_period_ps = period_ns * 1000UL;
>   	ni_tio_set_sync_mode(counter);
>   	return 0;
>   }

Looks good, thanks!

Reviewed-by: Ian Abbott <abbotti@mev.co.uk>

Could you resend it and Cc Greg Kroah-Hartman 
<gregkh@linuxfoundation.org> ?  It's more likely to get applied that way 
because I don't maintain my own kernel tree for comedi.  Feel free to 
add my Reviewed-by line.

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

[PATCH] comedi: drivers: ni_tio: Fix arithmetic expression overflow

[Denis Arefev]

The value of an arithmetic expression period_ns * 1000 is subject
to overflow due to a failure to cast operands to a larger data
type before performing arithmetic

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
Signed-off-by: Denis Arefev <arefev@swemel.ru>
---
 drivers/comedi/drivers/ni_tio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/comedi/drivers/ni_tio.c b/drivers/comedi/drivers/ni_tio.c
index da6826d77e60..acc914903c70 100644
--- a/drivers/comedi/drivers/ni_tio.c
+++ b/drivers/comedi/drivers/ni_tio.c
@@ -800,7 +800,7 @@ static int ni_tio_set_clock_src(struct ni_gpct *counter,
 				GI_PRESCALE_X2(counter_dev->variant) |
 				GI_PRESCALE_X8(counter_dev->variant), bits);
 	}
-	counter->clock_period_ps = period_ns * 1000;
+	counter->clock_period_ps = period_ns * 1000UL;
 	ni_tio_set_sync_mode(counter);
 	return 0;
 }
-- 
2.25.1

Re: [PATCH] comedi: drivers: ni_tio: Fix arithmetic expression overflow

[Greg Kroah-Hartman]

On Thu, Jan 18, 2024 at 10:36:19AM +0300, Denis Arefev wrote:
> The value of an arithmetic expression period_ns * 1000 is subject
> to overflow due to a failure to cast operands to a larger data
> type before performing arithmetic
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
> Signed-off-by: Denis Arefev <arefev@swemel.ru>
> ---
>  drivers/comedi/drivers/ni_tio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/comedi/drivers/ni_tio.c b/drivers/comedi/drivers/ni_tio.c
> index da6826d77e60..acc914903c70 100644
> --- a/drivers/comedi/drivers/ni_tio.c
> +++ b/drivers/comedi/drivers/ni_tio.c
> @@ -800,7 +800,7 @@ static int ni_tio_set_clock_src(struct ni_gpct *counter,
>  				GI_PRESCALE_X2(counter_dev->variant) |
>  				GI_PRESCALE_X8(counter_dev->variant), bits);
>  	}
> -	counter->clock_period_ps = period_ns * 1000;
> +	counter->clock_period_ps = period_ns * 1000UL;

Can period_ns ever be "too big" to cause such an overflow?

And what commit id does this fix?

thanks,

greg k-h

Re: [PATCH] comedi: drivers: ni_tio: Fix arithmetic expression overflow

[Ian Abbott]

On 18/01/2024 09:32, Greg Kroah-Hartman wrote:
> On Thu, Jan 18, 2024 at 10:36:19AM +0300, Denis Arefev wrote:
>> The value of an arithmetic expression period_ns * 1000 is subject
>> to overflow due to a failure to cast operands to a larger data
>> type before performing arithmetic
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
>> Signed-off-by: Denis Arefev <arefev@swemel.ru>
>> ---
>>   drivers/comedi/drivers/ni_tio.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/comedi/drivers/ni_tio.c b/drivers/comedi/drivers/ni_tio.c
>> index da6826d77e60..acc914903c70 100644
>> --- a/drivers/comedi/drivers/ni_tio.c
>> +++ b/drivers/comedi/drivers/ni_tio.c
>> @@ -800,7 +800,7 @@ static int ni_tio_set_clock_src(struct ni_gpct *counter,
>>   				GI_PRESCALE_X2(counter_dev->variant) |
>>   				GI_PRESCALE_X8(counter_dev->variant), bits);
>>   	}
>> -	counter->clock_period_ps = period_ns * 1000;
>> +	counter->clock_period_ps = period_ns * 1000UL;
> 
> Can period_ns ever be "too big" to cause such an overflow?

The period_ns value comes from user-space and could be any unsigned int 
value.  It's used to let the driver know the period of an external clock 
source.

> And what commit id does this fix?

Fixes: 3e90b1c7ebe9 ("staging: comedi: ni_tio: tidy up 
ni_tio_set_clock_src() and helpers")
Cc: <stable@vger.kernel.org> # v5.15+

The first affected kernel version is v3.17.  This patch should apply 
cleanly to v5.13 onwards (when comedi was moved out of drivers/staging), 
but earlier stable/longterm kernels would require a backported patch.

@Denis, if you could add the above Fixes: and Cc: lines to a v2 patch, 
that would be great!

> 
> thanks,
> 
> greg k-h

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

[PATCH] comedi: drivers: ni_tio: Fix arithmetic expression overflow

[Denis Arefev]

The value of an arithmetic expression period_ns * 1000 is subject
to overflow due to a failure to cast operands to a larger data
type before performing arithmetic

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 3e90b1c7ebe9 ("staging: comedi: ni_tio: tidy up ni_tio_set_clock_src() and helpers")
Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
Signed-off-by: Denis Arefev <arefev@swemel.ru>
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/comedi/drivers/ni_tio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/comedi/drivers/ni_tio.c b/drivers/comedi/drivers/ni_tio.c
index da6826d77e60..acc914903c70 100644
--- a/drivers/comedi/drivers/ni_tio.c
+++ b/drivers/comedi/drivers/ni_tio.c
@@ -800,7 +800,7 @@ static int ni_tio_set_clock_src(struct ni_gpct *counter,
 				GI_PRESCALE_X2(counter_dev->variant) |
 				GI_PRESCALE_X8(counter_dev->variant), bits);
 	}
-	counter->clock_period_ps = period_ns * 1000;
+	counter->clock_period_ps = period_ns * 1000UL;
 	ni_tio_set_sync_mode(counter);
 	return 0;
 }
-- 
2.25.1

[PATCH] comedi: drivers: ni_tio: Fix arithmetic expression overflow

[Denis Arefev]

The value of an arithmetic expression period_ns * 1000 is subject
to overflow due to a failure to cast operands to a larger data
type before performing arithmetic

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 3e90b1c7ebe9 ("staging: comedi: ni_tio: tidy up ni_tio_set_clock_src() and helpers")
Cc: <stable@vger.kernel.org> # v5.15+ 
Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
Signed-off-by: Denis Arefev <arefev@swemel.ru>
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/comedi/drivers/ni_tio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/comedi/drivers/ni_tio.c b/drivers/comedi/drivers/ni_tio.c
index da6826d77e60..acc914903c70 100644
--- a/drivers/comedi/drivers/ni_tio.c
+++ b/drivers/comedi/drivers/ni_tio.c
@@ -800,7 +800,7 @@ static int ni_tio_set_clock_src(struct ni_gpct *counter,
 				GI_PRESCALE_X2(counter_dev->variant) |
 				GI_PRESCALE_X8(counter_dev->variant), bits);
 	}
-	counter->clock_period_ps = period_ns * 1000;
+	counter->clock_period_ps = period_ns * 1000UL;
 	ni_tio_set_sync_mode(counter);
 	return 0;
 }
-- 
2.25.1

Re: [PATCH] comedi: drivers: ni_tio: Fix arithmetic expression overflow

[Greg Kroah-Hartman]

On Thu, Jan 18, 2024 at 03:37:47PM +0300, Denis Arefev wrote:
> The value of an arithmetic expression period_ns * 1000 is subject
> to overflow due to a failure to cast operands to a larger data
> type before performing arithmetic
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 3e90b1c7ebe9 ("staging: comedi: ni_tio: tidy up ni_tio_set_clock_src() and helpers")
> Cc: <stable@vger.kernel.org> # v5.15+ 
> Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
> Signed-off-by: Denis Arefev <arefev@swemel.ru>
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
>  drivers/comedi/drivers/ni_tio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/comedi/drivers/ni_tio.c b/drivers/comedi/drivers/ni_tio.c
> index da6826d77e60..acc914903c70 100644
> --- a/drivers/comedi/drivers/ni_tio.c
> +++ b/drivers/comedi/drivers/ni_tio.c
> @@ -800,7 +800,7 @@ static int ni_tio_set_clock_src(struct ni_gpct *counter,
>  				GI_PRESCALE_X2(counter_dev->variant) |
>  				GI_PRESCALE_X8(counter_dev->variant), bits);
>  	}
> -	counter->clock_period_ps = period_ns * 1000;
> +	counter->clock_period_ps = period_ns * 1000UL;
>  	ni_tio_set_sync_mode(counter);
>  	return 0;
>  }
> -- 
> 2.25.1
> 
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/process/submitting-patches.rst for what
  needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

RE: [PATCH] comedi: drivers: ni_tio: Fix arithmetic expression overflow

[David Laight]

> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: 18 January 2024 14:14
> To: Denis Arefev <arefev@swemel.ru>
> Cc: Ian Abbott <abbotti@mev.co.uk>; H Hartley Sweeten <hsweeten@visionengravers.com>; linux-
> kernel@vger.kernel.org; lvc-project@linuxtesting.org; stable@vger.kernel.org
> Subject: Re: [PATCH] comedi: drivers: ni_tio: Fix arithmetic expression overflow
> 
> On Thu, Jan 18, 2024 at 03:37:47PM +0300, Denis Arefev wrote:
> > The value of an arithmetic expression period_ns * 1000 is subject
> > to overflow due to a failure to cast operands to a larger data
> > type before performing arithmetic
> >
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> >
> > Fixes: 3e90b1c7ebe9 ("staging: comedi: ni_tio: tidy up ni_tio_set_clock_src() and helpers")
> > Cc: <stable@vger.kernel.org> # v5.15+
> > Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
> > Signed-off-by: Denis Arefev <arefev@swemel.ru>
> > Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> > ---
> >  drivers/comedi/drivers/ni_tio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/comedi/drivers/ni_tio.c b/drivers/comedi/drivers/ni_tio.c
> > index da6826d77e60..acc914903c70 100644
> > --- a/drivers/comedi/drivers/ni_tio.c
> > +++ b/drivers/comedi/drivers/ni_tio.c
> > @@ -800,7 +800,7 @@ static int ni_tio_set_clock_src(struct ni_gpct *counter,
> >  				GI_PRESCALE_X2(counter_dev->variant) |
> >  				GI_PRESCALE_X8(counter_dev->variant), bits);
> >  	}
> > -	counter->clock_period_ps = period_ns * 1000;
> > +	counter->clock_period_ps = period_ns * 1000UL;

What about 32bit systems...

> >  	ni_tio_set_sync_mode(counter);
> >  	return 0;
> >  }
> > --
> > 2.25.1
> >
> >
> 
> Hi,
> 
> This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
> a patch that has triggered this response.  He used to manually respond
> to these common problems, but in order to save his sanity (he kept
> writing the same thing over and over, yet to different people), I was
> created.  Hopefully you will not take offence and will fix the problem
> in your patch and resubmit it so that it can be accepted into the Linux
> kernel tree.
> 
> You are receiving this message because of the following common error(s)
> as indicated below:
> 
> - This looks like a new version of a previously submitted patch, but you
>   did not list below the --- line any changes from the previous version.
>   Please read the section entitled "The canonical patch format" in the
>   kernel file, Documentation/process/submitting-patches.rst for what
>   needs to be done here to properly describe this.
> 
> If you wish to discuss this problem further, or you have questions about
> how to resolve this issue, please feel free to respond to this email and
> Greg will reply once he has dug out from the pending patches received
> from other developers.
> 
> thanks,
> 
> greg k-h's patch email bot

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] comedi: drivers: ni_tio: Fix arithmetic expression overflow

[Ian Abbott]

On 2024-01-18 14:37, David Laight wrote:
>> -----Original Message-----
>> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Sent: 18 January 2024 14:14
>> To: Denis Arefev <arefev@swemel.ru>
>> Cc: Ian Abbott <abbotti@mev.co.uk>; H Hartley Sweeten <hsweeten@visionengravers.com>; linux-
>> kernel@vger.kernel.org; lvc-project@linuxtesting.org; stable@vger.kernel.org
>> Subject: Re: [PATCH] comedi: drivers: ni_tio: Fix arithmetic expression overflow
>>
>> On Thu, Jan 18, 2024 at 03:37:47PM +0300, Denis Arefev wrote:
>>> The value of an arithmetic expression period_ns * 1000 is subject
>>> to overflow due to a failure to cast operands to a larger data
>>> type before performing arithmetic
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>>
>>> Fixes: 3e90b1c7ebe9 ("staging: comedi: ni_tio: tidy up ni_tio_set_clock_src() and helpers")
>>> Cc: <stable@vger.kernel.org> # v5.15+
>>> Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
>>> Signed-off-by: Denis Arefev <arefev@swemel.ru>
>>> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
>>> ---
>>>   drivers/comedi/drivers/ni_tio.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/comedi/drivers/ni_tio.c b/drivers/comedi/drivers/ni_tio.c
>>> index da6826d77e60..acc914903c70 100644
>>> --- a/drivers/comedi/drivers/ni_tio.c
>>> +++ b/drivers/comedi/drivers/ni_tio.c
>>> @@ -800,7 +800,7 @@ static int ni_tio_set_clock_src(struct ni_gpct *counter,
>>>   				GI_PRESCALE_X2(counter_dev->variant) |
>>>   				GI_PRESCALE_X8(counter_dev->variant), bits);
>>>   	}
>>> -	counter->clock_period_ps = period_ns * 1000;
>>> +	counter->clock_period_ps = period_ns * 1000UL;
> 
> What about 32bit systems...

Oh, good point.  It should be 1000ULL.

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-