[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] target/mips/mxu_translate.c: Fix array overrun for D16MIN/D1
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH] target/mips/mxu_translate.c: Fix array overrun for D16MIN/D16MAX |
Date: |
Tue, 16 Mar 2021 16:38:59 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 |
+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>