[PATCH 0/3] target/arm: Coverity tweaks

[PATCH 0/3] target/arm: Coverity tweaks

[Richard Henderson]

Only the first of these appears to be a real bug.
The other two are adjustments to help satisfy Coverity.


r~


Richard Henderson (3):
  target/arm: Rearrange disabled check for watchpoints
  target/arm: Assert immh != 0 in disas_simd_shift_imm
  target/arm: Move computation of index in handle_simd_dupe

 target/arm/helper.c        | 11 ++++++-----
 target/arm/translate-a64.c |  6 +++++-
 2 files changed, 11 insertions(+), 6 deletions(-)

-- 
2.20.1

[PATCH 1/3] target/arm: Rearrange disabled check for watchpoints

[Richard Henderson]

Coverity rightly notes that ctz32(bas) on 0 will return 32,
which makes the len calculation a BAD_SHIFT.

A value of 0 in DBGWCR<n>_EL1.BAS is reserved.  Simply move
the existing check we have for this case.

Reported-by: Coverity (CID 1421964)
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index d2ec2c5351..b7b6887241 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6340,17 +6340,18 @@ void hw_watchpoint_update(ARMCPU *cpu, int n)
         int bas = extract64(wcr, 5, 8);
         int basstart;
 
-        if (bas == 0) {
-            /* This must act as if the watchpoint is disabled */
-            return;
-        }
-
         if (extract64(wvr, 2, 1)) {
             /* Deprecated case of an only 4-aligned address. BAS[7:4] are
              * ignored, and BAS[3:0] define which bytes to watch.
              */
             bas &= 0xf;
         }
+
+        if (bas == 0) {
+            /* This must act as if the watchpoint is disabled */
+            return;
+        }
+
         /* The BAS bits are supposed to be programmed to indicate a contiguous
          * range of bytes. Otherwise it is CONSTRAINED UNPREDICTABLE whether
          * we fire for each byte in the word/doubleword addressed by the WVR.
-- 
2.20.1

[PATCH 2/3] target/arm: Assert immh != 0 in disas_simd_shift_imm

[Richard Henderson]

Coverity raised a shed-load of errors cascading from inferring
that clz32(immh) might yield 32, from immh might be 0.

While immh cannot be 0 from encoding, it is not obvious even to
a human how we've checked that: via the filtering provided by
data_proc_simd[].

Reported-by: Coverity (CID 1421923, and more)
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate-a64.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 8fffb52203..032478614c 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -10405,6 +10405,9 @@ static void disas_simd_shift_imm(DisasContext *s, uint32_t insn)
     bool is_u = extract32(insn, 29, 1);
     bool is_q = extract32(insn, 30, 1);
 
+    /* data_proc_simd[] has sent immh == 0 to disas_simd_mod_imm. */
+    assert(immh != 0);
+
     switch (opcode) {
     case 0x08: /* SRI */
         if (!is_u) {
-- 
2.20.1

[PATCH 3/3] target/arm: Move computation of index in handle_simd_dupe

[Richard Henderson]

Coverity reports a BAD_SHIFT with ctz32(imm5), with imm5 == 0.
This is an invalid encoding, but we diagnose that just below
by rejecting size > 3.  Avoid the warning by sinking the
computation of index below the check.

Reported-by: Coverity (CID 1421965)
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate-a64.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 032478614c..7580e46367 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -7422,7 +7422,7 @@ static void handle_simd_dupe(DisasContext *s, int is_q, int rd, int rn,
                              int imm5)
 {
     int size = ctz32(imm5);
-    int index = imm5 >> (size + 1);
+    int index;
 
     if (size > 3 || (size == 3 && !is_q)) {
         unallocated_encoding(s);
@@ -7433,6 +7433,7 @@ static void handle_simd_dupe(DisasContext *s, int is_q, int rd, int rn,
         return;
     }
 
+    index = imm5 >> (size + 1);
     tcg_gen_gvec_dup_mem(size, vec_full_reg_offset(s, rd),
                          vec_reg_offset(s, rn, index, size),
                          is_q ? 16 : 8, vec_full_reg_size(s));
-- 
2.20.1

Re: [PATCH 0/3] target/arm: Coverity tweaks

[Philippe Mathieu-Daudé]

On 3/20/20 5:06 PM, Richard Henderson wrote:
> Only the first of these appears to be a real bug.
> The other two are adjustments to help satisfy Coverity.
> 
> 
> r~
> 
> 
> Richard Henderson (3):
>    target/arm: Rearrange disabled check for watchpoints
>    target/arm: Assert immh != 0 in disas_simd_shift_imm
>    target/arm: Move computation of index in handle_simd_dupe
> 
>   target/arm/helper.c        | 11 ++++++-----
>   target/arm/translate-a64.c |  6 +++++-
>   2 files changed, 11 insertions(+), 6 deletions(-)
> 

Series:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 0/3] target/arm: Coverity tweaks

[Peter Maydell]

On Fri, 20 Mar 2020 at 16:06, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Only the first of these appears to be a real bug.
> The other two are adjustments to help satisfy Coverity.
>
>
> r~
>
>
> Richard Henderson (3):
>   target/arm: Rearrange disabled check for watchpoints
>   target/arm: Assert immh != 0 in disas_simd_shift_imm
>   target/arm: Move computation of index in handle_simd_dupe



Applied to target-arm.next, thanks.

-- PMM