linux-kernel - Re: [PATCH] workaround regression in ina2xx introduced by cb47755725da("time: Prevent undefined behaviour in timespec64_to

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] [day] [month] [year] [list]

Message-ID: <20210911142326.4acd95a1@jic23-huawei>
Date:   Sat, 11 Sep 2021 14:23:26 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Iain Hunter <drhunter95@...il.com>
Cc:     lothar.felten@...il.com, iain@...terembedded.co.uk,
        Lars-Peter Clausen <lars@...afoo.de>,
        Alexandru Ardelean <alexandru.ardelean@...log.com>,
        Matt Ranostay <matt.ranostay@...sulko.com>,
        Gwendal Grignou <gwendal@...omium.org>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] workaround regression in ina2xx introduced by
 cb47755725da("time: Prevent undefined behaviour in timespec64_to_ns()")

On Sat, 11 Sep 2021 12:36:23 +0100
Iain Hunter <drhunter95@...il.com> wrote:

> From: Iain Hunter <iain@...terembedded.co.uk>
> 
> That change adds an error check to avoid saturation during multiplication
> to calculate nano seconds in timespec64_to_ns().
> In ina2xx_capture_thread() a timespec64 structure is used to calculate
> the delta time until the next sample time. This delta can be negative if
> the next sample time was in the past. In the -1 case timespec64_to_ns()
> now clamps the -1 second value to KTIME_MAX. This essentially puts ina2xx
> thread to sleep forever.
> Proposed patch is to replace the call to timespec64_to_ns() with the
> contents of that function without the overflow test introduced by the
> commit (ie revert to pre kernel 5.4 behaviour)
> 
> Signed-off-by: Iain Hunter <iain@...terembedded.co.uk>

Needs a fixes tag with the patch you mention above that added the check
so that we can tell how far back this needs to be backported.



> ---
>  drivers/iio/adc/ina2xx-adc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index a4b2ff9e0..ba3e98fde 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -777,6 +777,7 @@ static int ina2xx_capture_thread(void *data)
>  	int ret;
>  	struct timespec64 next, now, delta;
>  	s64 delay_us;
> +	s64 delta_ns;
>  
>  	/*
>  	 * Poll a bit faster than the chip internal Fs, in case
> @@ -818,7 +819,8 @@ static int ina2xx_capture_thread(void *data)
>  		do {
>  			timespec64_add_ns(&next, 1000 * sampling_us);
>  			delta = timespec64_sub(next, now);
> -			delay_us = div_s64(timespec64_to_ns(&delta), 1000);
> +			delta_ns = (((s64)delta.tv_sec) * NSEC_PER_SEC)+delta.tv_nsec;

spaces around the +

> +			delay_us = div_s64(delta_ns, 1000);

Hmm. The negative timestamp is a bit of a mess anyway.  Perhaps we can do something
neater using the standard functions by checking the validity of the timestamp
using timespec64_valid_strict() in the while loop and dropping the div_s64 out
of the loop.

What do you think?  Would need a comment to explain why we the check on
it being valid though.

Jonathan


>  		} while (delay_us <= 0);
>  
>  		usleep_range(delay_us, (delay_us * 3) >> 1);