Message ID | 20241212142716.523980-1-gerben@altlinux.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | target/arm/tcg: fix potential integer overflow in iwmmxt_macuw() | expand |
On 12/12/24 08:27, 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); Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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
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);