linux-kernel - Re: [PATCH v5 04/10] iio: afe: rescale: reduce risk of integer overflow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 15 Jul 2021 12:23:14 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     Liam Beguin <liambeguin@...il.com>, jic23@...nel.org,
        lars@...afoo.de, pmeerw@...erw.net
Cc:     linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org, robh+dt@...nel.org
Subject: Re: [PATCH v5 04/10] iio: afe: rescale: reduce risk of integer
 overflow

On 2021-07-15 05:12, Liam Beguin wrote:
> From: Liam Beguin <lvb@...hos.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@...hos.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;
>