Re: [PATCH] hw/timer: fix int underflow

[Peter Maydell]

On Wed, 6 Nov 2024 at 10:41, Dmitry Frolov <frolov@swemel.ru> wrote:
>
> Both timeout and return value of imx_gpt_update_count() are unsigned.
> Thus "limit" can not be negative, but obviously it was implied.

For changes to device models, you need to look at the
data sheet for the device to determine the correct
behaviour so you can confirm that the change you're
making is right. This commit message doesn't include
any of that reasoning, which means reviewers have to
do it all for themselves.

> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> ---
>  hw/timer/imx_gpt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/timer/imx_gpt.c b/hw/timer/imx_gpt.c
> index 23b3d79bdb..06e4837fed 100644
> --- a/hw/timer/imx_gpt.c
> +++ b/hw/timer/imx_gpt.c
> @@ -238,7 +238,7 @@ static void imx_gpt_compute_next_timeout(IMXGPTState *s, 
> bool event)
>      }
>
>      /* the new range to count down from */
> -    limit = timeout - imx_gpt_update_count(s);
> +    limit = (long long)timeout - imx_gpt_update_count(s);

We really shouldn't be using "long long" here in the
first place. If we need a 64-bit signed integer to
give correct behaviour then that's int64_t. Almost
all the handful of uses of 'long long' in hw/ are
doing it just as a lazy way to avoid PRIx64 in a
printf format string.

The GPT has a 32-bit up-counter. If timeout is, for
instance, 0x9000_0000 and the current count is
0x2000_0000 then we want the new "limit" value to
be 0x7000_0000 because we still have 0x7000_0000
cycles to go til we hit the next point where we want
to generate a compare event. So I think we do indeed
want to cast to a 64-bit signed value here.

(More broadly I think the problem with this device is
that it's trying to use ptimer (which is a down-counter)
to model an up-counter. This doesn't work very well,
and it's better to not do that. Arguably we should have
some nicer abstractions over raw qtimers for the
up-counter case and/or get ptimer to handle that
too, but that's a lot of work...)

thanks
-- PMM