From: Liam Beguin <[email protected]>
Reduce the risk of integer overflow by doing the scale calculation with
64bit integers and looking for a Greatest Common Divider for both parts
of the fractional value.
Signed-off-by: Liam Beguin <[email protected]>
---
drivers/iio/afe/iio-rescale.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 774eb3044edd..98bcb5d418d6 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -39,7 +39,8 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
int *val, int *val2, long mask)
{
struct rescale *rescale = iio_priv(indio_dev);
- unsigned long long tmp;
+ s64 tmp, tmp2;
+ u32 factor;
int ret;
switch (mask) {
@@ -67,8 +68,11 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
}
switch (ret) {
case IIO_VAL_FRACTIONAL:
- *val *= rescale->numerator;
- *val2 *= rescale->denominator;
+ tmp = (s64)*val * rescale->numerator;
+ tmp2 = (s64)*val2 * rescale->denominator;
+ factor = gcd(tmp, tmp2);
+ *val = tmp / factor;
+ *val2 = tmp2 / factor;
return ret;
case IIO_VAL_INT:
*val *= rescale->numerator;
--
2.30.1.489.g328c10930387
On Wed, 30 Jun 2021 21:00:28 -0400
Liam Beguin <[email protected]> wrote:
> From: Liam Beguin <[email protected]>
>
> Reduce the risk of integer overflow by doing the scale calculation with
> 64bit integers and looking for a Greatest Common Divider for both parts
> of the fractional value.
>
> Signed-off-by: Liam Beguin <[email protected]>
> ---
> drivers/iio/afe/iio-rescale.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 774eb3044edd..98bcb5d418d6 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -39,7 +39,8 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
> int *val, int *val2, long mask)
> {
> struct rescale *rescale = iio_priv(indio_dev);
> - unsigned long long tmp;
> + s64 tmp, tmp2;
> + u32 factor;
> int ret;
>
> switch (mask) {
> @@ -67,8 +68,11 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
> }
> switch (ret) {
> case IIO_VAL_FRACTIONAL:
> - *val *= rescale->numerator;
> - *val2 *= rescale->denominator;
> + tmp = (s64)*val * rescale->numerator;
> + tmp2 = (s64)*val2 * rescale->denominator;
> + factor = gcd(tmp, tmp2);
Hmm. I wonder if there are cases where this doesn't work and we end up
truncating because the gcd is say 1. If all of val, val2, rescale->numerator,
rescale->denominator are primes and the rescale values are moderately large
then that might happen. We probably need a fallback position. Perhaps
check tmp / factor and temp2/factor will fit in an int. If not, shift them until
they do even if we have to dump some precision to do so.
This stuff is getting fiddly enough we might want to figure out some self tests
that exercise the various cases.
> + *val = tmp / factor;
> + *val2 = tmp2 / factor;
This is doing 64 bit numbers divided by 32 bit ones. Doesn't that require
use of do_div() etc on 32 bit platforms?
> return ret;
> case IIO_VAL_INT:
> *val *= rescale->numerator;
On Sun Jul 4, 2021 at 12:36 PM EDT, Jonathan Cameron wrote:
> On Wed, 30 Jun 2021 21:00:28 -0400
> Liam Beguin <[email protected]> wrote:
>
> > From: Liam Beguin <[email protected]>
> >
> > Reduce the risk of integer overflow by doing the scale calculation with
> > 64bit integers and looking for a Greatest Common Divider for both parts
> > of the fractional value.
> >
> > Signed-off-by: Liam Beguin <[email protected]>
> > ---
> > drivers/iio/afe/iio-rescale.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > index 774eb3044edd..98bcb5d418d6 100644
> > --- a/drivers/iio/afe/iio-rescale.c
> > +++ b/drivers/iio/afe/iio-rescale.c
> > @@ -39,7 +39,8 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
> > int *val, int *val2, long mask)
> > {
> > struct rescale *rescale = iio_priv(indio_dev);
> > - unsigned long long tmp;
> > + s64 tmp, tmp2;
> > + u32 factor;
> > int ret;
> >
> > switch (mask) {
> > @@ -67,8 +68,11 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
> > }
> > switch (ret) {
> > case IIO_VAL_FRACTIONAL:
> > - *val *= rescale->numerator;
> > - *val2 *= rescale->denominator;
> > + tmp = (s64)*val * rescale->numerator;
> > + tmp2 = (s64)*val2 * rescale->denominator;
> > + factor = gcd(tmp, tmp2);
>
> Hmm. I wonder if there are cases where this doesn't work and we end up
> truncating because the gcd is say 1. If all of val, val2,
> rescale->numerator,
> rescale->denominator are primes and the rescale values are moderately
> large
> then that might happen. We probably need a fallback position. Perhaps
> check tmp / factor and temp2/factor will fit in an int. If not, shift
> them until
> they do even if we have to dump some precision to do so.
>
I see what you mean. If we want to do that I guess it would also apply
to other areas of the driver.
> This stuff is getting fiddly enough we might want to figure out some
> self tests
> that exercise the various cases.
>
I never implemented kernel self tests before, I guess it should follow
the example of drivers/iio/test/iio-test-format.c?
Would you be okay to add this in a follow up series?
> > + *val = tmp / factor;
> > + *val2 = tmp2 / factor;
>
> This is doing 64 bit numbers divided by 32 bit ones. Doesn't that
> require
> use of do_div() etc on 32 bit platforms?
>
Apologies for that mistake, will fix.
> > return ret;
> > case IIO_VAL_INT:
> > *val *= rescale->numerator;
On Mon, 05 Jul 2021 00:23:59 -0400
"Liam Beguin" <[email protected]> wrote:
> On Sun Jul 4, 2021 at 12:36 PM EDT, Jonathan Cameron wrote:
> > On Wed, 30 Jun 2021 21:00:28 -0400
> > Liam Beguin <[email protected]> wrote:
> >
> > > From: Liam Beguin <[email protected]>
> > >
> > > Reduce the risk of integer overflow by doing the scale calculation with
> > > 64bit integers and looking for a Greatest Common Divider for both parts
> > > of the fractional value.
> > >
> > > Signed-off-by: Liam Beguin <[email protected]>
> > > ---
> > > drivers/iio/afe/iio-rescale.c | 10 +++++++---
> > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > > index 774eb3044edd..98bcb5d418d6 100644
> > > --- a/drivers/iio/afe/iio-rescale.c
> > > +++ b/drivers/iio/afe/iio-rescale.c
> > > @@ -39,7 +39,8 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
> > > int *val, int *val2, long mask)
> > > {
> > > struct rescale *rescale = iio_priv(indio_dev);
> > > - unsigned long long tmp;
> > > + s64 tmp, tmp2;
> > > + u32 factor;
> > > int ret;
> > >
> > > switch (mask) {
> > > @@ -67,8 +68,11 @@ static int rescale_read_raw(struct iio_dev *indio_dev,
> > > }
> > > switch (ret) {
> > > case IIO_VAL_FRACTIONAL:
> > > - *val *= rescale->numerator;
> > > - *val2 *= rescale->denominator;
> > > + tmp = (s64)*val * rescale->numerator;
> > > + tmp2 = (s64)*val2 * rescale->denominator;
> > > + factor = gcd(tmp, tmp2);
> >
> > Hmm. I wonder if there are cases where this doesn't work and we end up
> > truncating because the gcd is say 1. If all of val, val2,
> > rescale->numerator,
> > rescale->denominator are primes and the rescale values are moderately
> > large
> > then that might happen. We probably need a fallback position. Perhaps
> > check tmp / factor and temp2/factor will fit in an int. If not, shift
> > them until
> > they do even if we have to dump some precision to do so.
> >
>
> I see what you mean. If we want to do that I guess it would also apply
> to other areas of the driver.
Certainly possible. It's a bit obscure so may not have occurred to anyone
on previous reviews :(
>
> > This stuff is getting fiddly enough we might want to figure out some
> > self tests
> > that exercise the various cases.
> >
>
> I never implemented kernel self tests before, I guess it should follow
> the example of drivers/iio/test/iio-test-format.c?
>
> Would you be okay to add this in a follow up series?
Yes, that's fine.
>
> > > + *val = tmp / factor;
> > > + *val2 = tmp2 / factor;
> >
> > This is doing 64 bit numbers divided by 32 bit ones. Doesn't that
> > require
> > use of do_div() etc on 32 bit platforms?
> >
>
> Apologies for that mistake, will fix.
>
> > > return ret;
> > > case IIO_VAL_INT:
> > > *val *= rescale->numerator;
>