Re: [PATCH] target/arm/tcg: fix potential integer overflow in iwmmxt_macuw()

[Peter Maydell]

On Thu, 12 Dec 2024 at 14:28, <gerben@altlinux.org> wrote:
>
> From: Denis Rastyogin <gerben@altlinux.org>
>
> The function iwmmxt_macuw() could potentially cause an integer
> overflow when summing up four 32-bit multiplications.
> This occurs because the intermediate results may exceed the 32-bit
> range before being cast to uint64_t. The fix ensures each
> multiplication is explicitly cast to uint64_t prior to summation,
> preventing potential issues and ensuring correctness.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Signed-off-by: Denis Sergeev <zeff@altlinux.org>
> Signed-off-by: Denis Rastyogin <gerben@altlinux.org>
> ---
>  target/arm/tcg/iwmmxt_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/tcg/iwmmxt_helper.c b/target/arm/tcg/iwmmxt_helper.c
> index 610b1b2103..19c709655e 100644
> --- a/target/arm/tcg/iwmmxt_helper.c
> +++ b/target/arm/tcg/iwmmxt_helper.c
> @@ -140,7 +140,7 @@ uint64_t HELPER(iwmmxt_macsw)(uint64_t a, uint64_t b)
>
>  uint64_t HELPER(iwmmxt_macuw)(uint64_t a, uint64_t b)
>  {
> -#define MACU(SHR) ( \
> +#define MACU(SHR) (uint64_t)( \
>          (uint32_t) ((a >> SHR) & 0xffff) * \
>          (uint32_t) ((b >> SHR) & 0xffff))
>      return MACU(0) + MACU(16) + MACU(32) + MACU(48);

This makes the unsigned version of iwMMXt WMAC behave differently
from the signed version (which is still doing the addition at
32 bits and then sign extending to 64 bits). The description in
the Intel Wireless MMX Technology Developer Guide is not
super clear (it says "The input arguments are (Ax,Bx) 16-bits,
the intermediate values (Px) are 32-bits, and the result is
64-bits" where the Px are the results of the multiplies;
there's just a diagram of a sum being done on the Px and the
64-bit wRd input into the 64-bit wRd output, so although it's
clear that the Px should be 32-bits it's not totally clear
that the accumulation of these different sized inputs should
all be done at 64-bit width) -- but it seems pretty plausible
that this is supposed to be done with 64-bit addition.

But it definitely doesn't seem like the signed and unsigned
versions of the insn should be doing this differently, so
changing the iwmmxt_macuw helper and not iwmmxt_macsw doesn't
look right.

More generally, I am super reluctant to apply changes to the
iwMMXt decoder unless they come attached to descriptions of
testing that's been done against some known-good implementation
of iwMMXt (ideally hardware), because we have basically no
testing capability here. This is moribund code for a dead
subset of the architecture, and I am inclined to say
"don't touch it unless we know for a fact that it's
broken"...

thanks
-- PMM