| Message ID | 20210715031215.1534938-5-liambeguin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: afe: add temperature rescaling support | expand |
On 2021-07-15 05:12, Liam Beguin wrote: > From: Liam Beguin <lvb@xiphos.com> > > 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 when required. > > Signed-off-by: Liam Beguin <lvb@xiphos.com> > --- > drivers/iio/afe/iio-rescale.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c > index 774eb3044edd..4c3cfd4d5181 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,16 @@ 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; > + if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) || > + check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2)) { The white space should be like this, methinks. if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) || check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2)) { > + factor = gcd(tmp, tmp2); And I just realized, gcd() works on unsigned values which is a bit safer for the scale factor. But here, for the actual values, more care is needed. > + do_div(tmp, factor); > + do_div(tmp2, factor); > + } > + *val = tmp; > + *val2 = tmp2; And beside the above points, the whole mechanism seems broken. The returned value in the third argument to check_mul_overflow isn't useful if there is an overflow. Yet, the code continues to use tmp and tmp2 in case of overflow. And why do you first multiply tmp and tmp2 without checks, only to then do the same mul again but with checks? Or have I completely misunderstood how check_mul_overflow works? Cheers, Peter > return ret; > case IIO_VAL_INT: > *val *= rescale->numerator; >
On Thu Jul 15, 2021 at 6:23 AM EDT, Peter Rosin wrote: > On 2021-07-15 05:12, Liam Beguin wrote: > > From: Liam Beguin <lvb@xiphos.com> > > > > 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 when required. > > > > Signed-off-by: Liam Beguin <lvb@xiphos.com> > > --- > > drivers/iio/afe/iio-rescale.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c > > index 774eb3044edd..4c3cfd4d5181 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,16 @@ 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; > > + if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) || > > + check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2)) { Hi Peter, > > The white space should be like this, methinks. > > if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) || > check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2)) > { > Sorry about that... Like I said in the cover letter, I'm working on getting kunit tests running for the iio-rescale. At the moment it still requires copying part of the code over and sure enough I forgot to copy some of it back. My apologies for the noise... This is what I meant to send: case IIO_VAL_FRACTIONAL: if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) || check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2)) { tmp = (s64)*val * rescale->numerator; tmp2 = (s64)*val2 * rescale->denominator; factor = gcd(tmp, tmp2); do_div(tmp, factor); do_div(tmp2, factor); } *val = tmp; *val2 = tmp2; return ret; I'll also move the opening bracket on a new line if you prefer. > > + factor = gcd(tmp, tmp2); > > And I just realized, gcd() works on unsigned values which is a bit safer > for the > scale factor. But here, for the actual values, more care is needed. > I added negative test cases to take this into account. I'll update and resend. I'm going to find a way to get the test cases ready for the next revision. > > + do_div(tmp, factor); > > + do_div(tmp2, factor); > > + } > > + *val = tmp; > > + *val2 = tmp2; > > And beside the above points, the whole mechanism seems broken. The > returned value > in the third argument to check_mul_overflow isn't useful if there is an > overflow. > Yet, the code continues to use tmp and tmp2 in case of overflow. And why > do you > first multiply tmp and tmp2 without checks, only to then do the same mul > again > but with checks? Or have I completely misunderstood how > check_mul_overflow > works? > Again, my apologies for this. It's not what I meant to send. Hopefully the snippet above makes more sense. Thanks for your time, Liam > Cheers, > Peter > > > return ret; > > case IIO_VAL_INT: > > *val *= rescale->numerator; > >
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c index 774eb3044edd..4c3cfd4d5181 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,16 @@ 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; + if (check_mul_overflow(*val, rescale->numerator, (s32 *)&tmp) || + check_mul_overflow(*val2, rescale->denominator, (s32 *)&tmp2)) { + factor = gcd(tmp, tmp2); + do_div(tmp, factor); + do_div(tmp2, factor); + } + *val = tmp; + *val2 = tmp2; return ret; case IIO_VAL_INT: *val *= rescale->numerator;