Re: [PATCH] target/mips/mxu_translate.c: Fix array overrun for D16MIN/D16MAX

[Philippe Mathieu-Daudé]

+Craig

On 3/16/21 2:13 PM, Peter Maydell wrote:
> Coverity reported (CID 1450831) an array overrun in
> gen_mxu_D16MAX_D16MIN():
> 
>   1103     } else if (unlikely((XRb == 0) || (XRa == 0))) {
>   ....
>   1112         if (opc == OPC_MXU_D16MAX) {
>   1113             tcg_gen_smax_i32(mxu_gpr[XRa - 1], t0, t1);
>   1114         } else {
>   1115             tcg_gen_smin_i32(mxu_gpr[XRa - 1], t0, t1);
>   1116         }
> 
>>>> Overrunning array "mxu_gpr" of 15 8-byte elements at element
>     index 4294967295 (byte offset 34359738367) using index "XRa - 1U"
>     (which evaluates to 4294967295).
> 
> This happens because the code is confused about which of XRa, XRb and
> XRc is the output, and which are the inputs.  XRa is the output, but
> most of the conditions separating out different special cases are
> written as if XRc is the output, with the result that we can end up
> in the code path that assumes XRa is non-0 even when it is zero.
> 
> Fix the erroneous code, bringing it in to line with the structure
> used in functions like gen_mxu_S32MAX_S32MIN() and
> gen_mxu_Q8MAX_Q8MIN().
> 
> Fixes: CID 1450831
> Fixes: bb84cbf38505bd1d8
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> NB: tested with 'make check' and 'make check-acceptance' only, which
> almost certainly don't exercise this code path.

Not it isn't indeed.

I'm adding testing where users show interest.

For Loongson SIMD I have 2 images, Fedora and Debian, and watch
a video stream with ffmpeg. I have in my TODO to add a tcg plugin
to check the instruction coverage. Maybe it already exists, I haven't
checked.
Why I'd love to get this integration testing in mainstream, I gave up
because of the artifact storage problem (it doesn't make sense to
rebuild a full OS to test a single package to test an ISA).

I don't have anything for XBurst. Craig was running some
testing 2 years ago, but I have no idea what it was.

>  target/mips/mxu_translate.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/mips/mxu_translate.c b/target/mips/mxu_translate.c
> index afc008eeeef..fb0a811af6c 100644
> --- a/target/mips/mxu_translate.c
> +++ b/target/mips/mxu_translate.c
> @@ -1095,12 +1095,12 @@ static void gen_mxu_D16MAX_D16MIN(DisasContext *ctx)
>  
>      if (unlikely(pad != 0)) {
>          /* opcode padding incorrect -> do nothing */
> -    } else if (unlikely(XRc == 0)) {
> +    } else if (unlikely(XRa == 0)) {
>          /* destination is zero register -> do nothing */
> -    } else if (unlikely((XRb == 0) && (XRa == 0))) {
> +    } else if (unlikely((XRb == 0) && (XRc == 0))) {
>          /* both operands zero registers -> just set destination to zero */
> -        tcg_gen_movi_i32(mxu_gpr[XRc - 1], 0);
> -    } else if (unlikely((XRb == 0) || (XRa == 0))) {
> +        tcg_gen_movi_i32(mxu_gpr[XRa - 1], 0);
> +    } else if (unlikely((XRb == 0) || (XRc == 0))) {
>          /* exactly one operand is zero register - find which one is not...*/
>          uint32_t XRx = XRb ? XRb : XRc;
>          /* ...and do half-word-wise max/min with one operand 0 */
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>