[PATCH v1 0/5] Fix LoongArch coverity error and cpu name bug

[PATCH v1 0/5] Fix LoongArch coverity error and cpu name bug

[Xiaojuan Yang]

This series fix some errors for LoongArch virt machine
1. Fix coverity errors such as out-of-bounds, integer overflow,
cond_at_most, etc.
2. Fix loongarch_cpu_class_by_name function.

Xiaojuan Yang (5):
  target/loongarch/cpu: Fix cpu_class_by_name function
  hw/intc/loongarch_pch_pic: Fix coverity errors in update irq
  target/loongarch/cpu: Fix coverity errors about excp_names
  target/loongarch/tlb_helper: Fix coverity integer overflow error
  target/loongarch/op_helper: Fix coverity cond_at_most error

 hw/intc/loongarch_pch_pic.c   | 16 ++++++++++------
 target/loongarch/cpu.c        | 11 ++++-------
 target/loongarch/op_helper.c  |  2 +-
 target/loongarch/tlb_helper.c |  4 ++--
 4 files changed, 17 insertions(+), 16 deletions(-)

-- 
2.31.1

[PATCH 1/5] target/loongarch/cpu: Fix cpu_class_by_name function

[Xiaojuan Yang]

In loongarch_cpu_class_by_name(char *cpu_model) function,
the argument cpu_model already has the suffix '-loongarch-cpu',
so we should remove the LOONGARCH_CPU_TYPE_NAME(cpu_model) macro.

Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
---
 target/loongarch/cpu.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index e21715592a..8294b05ee8 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -571,11 +571,8 @@ static void loongarch_cpu_init(Object *obj)
 static ObjectClass *loongarch_cpu_class_by_name(const char *cpu_model)
 {
     ObjectClass *oc;
-    char *typename;
 
-    typename = g_strdup_printf(LOONGARCH_CPU_TYPE_NAME("%s"), cpu_model);
-    oc = object_class_by_name(typename);
-    g_free(typename);
+    oc = object_class_by_name(cpu_model);
     return oc;
 }
 
-- 
2.31.1

[PATCH 2/5] hw/intc/loongarch_pch_pic: Fix coverity errors in update irq

[Xiaojuan Yang]

Fix coverity errors:
1. In find_first_bit function, the 'size' argument need
'unsigned long' type, so we add the suffix 'UL' on 'size'
argument when use the function.
2. In expression 1ULL << irq, left shifting by more than
63 bits has undefined behavior. And out-of-bounds access
error occured when 'irq' >= 64. So we add a condition to
avoid this.

Fix coverity CID: 1489761 1489764 1489765

Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
---
 hw/intc/loongarch_pch_pic.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/hw/intc/loongarch_pch_pic.c b/hw/intc/loongarch_pch_pic.c
index 3c9814a3b4..779a087a03 100644
--- a/hw/intc/loongarch_pch_pic.c
+++ b/hw/intc/loongarch_pch_pic.c
@@ -21,16 +21,20 @@ static void pch_pic_update_irq(LoongArchPCHPIC *s, uint64_t mask, int level)
     if (level) {
         val = mask & s->intirr & ~s->int_mask;
         if (val) {
-            irq = find_first_bit(&val, 64);
-            s->intisr |= 0x1ULL << irq;
-            qemu_set_irq(s->parent_irq[s->htmsi_vector[irq]], 1);
+            irq = find_first_bit(&val, 64UL);
+            if (irq < 64) {
+                s->intisr |= 0x1ULL << irq;
+                qemu_set_irq(s->parent_irq[s->htmsi_vector[irq]], 1);
+            }
         }
     } else {
         val = mask & s->intisr;
         if (val) {
-            irq = find_first_bit(&val, 64);
-            s->intisr &= ~(0x1ULL << irq);
-            qemu_set_irq(s->parent_irq[s->htmsi_vector[irq]], 0);
+            irq = find_first_bit(&val, 64UL);
+            if (irq < 64) {
+                s->intisr &= ~(0x1ULL << irq);
+                qemu_set_irq(s->parent_irq[s->htmsi_vector[irq]], 0);
+            }
         }
     }
 }
-- 
2.31.1

[PATCH 3/5] target/loongarch/cpu: Fix coverity errors about excp_names

[Xiaojuan Yang]

Fix out-of-bounds errors when access excp_names[] array. the valid
boundary size of excp_names should be 0 to ARRAY_SIZE(excp_names)-1.
However, the general code do not consider the max boundary.

Fix coverity CID: 1489758

Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
---
 target/loongarch/cpu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index 8294b05ee8..8e17b61a85 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -140,7 +140,7 @@ static void loongarch_cpu_do_interrupt(CPUState *cs)
 
     if (cs->exception_index != EXCCODE_INT) {
         if (cs->exception_index < 0 ||
-            cs->exception_index > ARRAY_SIZE(excp_names)) {
+            cs->exception_index >= ARRAY_SIZE(excp_names)) {
             name = "unknown";
         } else {
             name = excp_names[cs->exception_index];
@@ -190,8 +190,8 @@ static void loongarch_cpu_do_interrupt(CPUState *cs)
         cause = cs->exception_index;
         break;
     default:
-        qemu_log("Error: exception(%d) '%s' has not been supported\n",
-                 cs->exception_index, excp_names[cs->exception_index]);
+        qemu_log("Error: exception(%d) has not been supported\n",
+                 cs->exception_index);
         abort();
     }
 
-- 
2.31.1

[PATCH 4/5] target/loongarch/tlb_helper: Fix coverity integer overflow error

[Xiaojuan Yang]

Replace '1' with '1UL' to fix unintentional integer overflow errors
in tlb_helper file.

Fix coverity CID: 1489759 1489762

Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
---
 target/loongarch/tlb_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/loongarch/tlb_helper.c b/target/loongarch/tlb_helper.c
index bab19c7e05..b300230588 100644
--- a/target/loongarch/tlb_helper.c
+++ b/target/loongarch/tlb_helper.c
@@ -298,7 +298,7 @@ static void invalidate_tlb_entry(CPULoongArchState *env, int index)
     } else {
         tlb_ps = FIELD_EX64(env->CSR_STLBPS, CSR_STLBPS, PS);
     }
-    pagesize = 1 << tlb_ps;
+    pagesize = 1UL << tlb_ps;
     mask = MAKE_64BIT_MASK(0, tlb_ps + 1);
 
     if (tlb_v0) {
@@ -736,7 +736,7 @@ void helper_ldpte(CPULoongArchState *env, target_ulong base, target_ulong odd,
                 (tmp0 & (~(1 << R_TLBENTRY_G_SHIFT)));
         ps = ptbase + ptwidth - 1;
         if (odd) {
-            tmp0 += (1 << ps);
+            tmp0 += (1UL << ps);
         }
     } else {
         /* 0:64bit, 1:128bit, 2:192bit, 3:256bit */
-- 
2.31.1

[PATCH 5/5] target/loongarch/op_helper: Fix coverity cond_at_most error

[Xiaojuan Yang]

The boundary size of cpucfg array should be 0 to 20. So,
using index bigger than 20 to access cpucfg[] must be forbidden.

Fix coverity CID: 1489760

Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
---
 target/loongarch/op_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/loongarch/op_helper.c b/target/loongarch/op_helper.c
index 4b429b6699..b05a0b7648 100644
--- a/target/loongarch/op_helper.c
+++ b/target/loongarch/op_helper.c
@@ -81,7 +81,7 @@ target_ulong helper_crc32c(target_ulong val, target_ulong m, uint64_t sz)
 
 target_ulong helper_cpucfg(CPULoongArchState *env, target_ulong rj)
 {
-    return rj > 21 ? 0 : env->cpucfg[rj];
+    return rj > 20 ? 0 : env->cpucfg[rj];
 }
 
 uint64_t helper_rdtime_d(CPULoongArchState *env)
-- 
2.31.1

Re: [PATCH 3/5] target/loongarch/cpu: Fix coverity errors about excp_names

[Richard Henderson]

On 7/12/22 13:31, Xiaojuan Yang wrote:
> Fix out-of-bounds errors when access excp_names[] array. the valid
> boundary size of excp_names should be 0 to ARRAY_SIZE(excp_names)-1.
> However, the general code do not consider the max boundary.
> 
> Fix coverity CID: 1489758
> 
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> ---
>   target/loongarch/cpu.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

> 
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index 8294b05ee8..8e17b61a85 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -140,7 +140,7 @@ static void loongarch_cpu_do_interrupt(CPUState *cs)
>   
>       if (cs->exception_index != EXCCODE_INT) {
>           if (cs->exception_index < 0 ||
> -            cs->exception_index > ARRAY_SIZE(excp_names)) {
> +            cs->exception_index >= ARRAY_SIZE(excp_names)) {
>               name = "unknown";
>           } else {
>               name = excp_names[cs->exception_index];
> @@ -190,8 +190,8 @@ static void loongarch_cpu_do_interrupt(CPUState *cs)
>           cause = cs->exception_index;
>           break;
>       default:
> -        qemu_log("Error: exception(%d) '%s' has not been supported\n",
> -                 cs->exception_index, excp_names[cs->exception_index]);
> +        qemu_log("Error: exception(%d) has not been supported\n",
> +                 cs->exception_index);
>           abort();
>       }
>   

Re: [PATCH 4/5] target/loongarch/tlb_helper: Fix coverity integer overflow error

[Richard Henderson]

On 7/12/22 13:31, Xiaojuan Yang wrote:
> Replace '1' with '1UL' to fix unintentional integer overflow errors
> in tlb_helper file.
> 
> Fix coverity CID: 1489759 1489762
> 
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> ---
>   target/loongarch/tlb_helper.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/loongarch/tlb_helper.c b/target/loongarch/tlb_helper.c
> index bab19c7e05..b300230588 100644
> --- a/target/loongarch/tlb_helper.c
> +++ b/target/loongarch/tlb_helper.c
> @@ -298,7 +298,7 @@ static void invalidate_tlb_entry(CPULoongArchState *env, int index)
>       } else {
>           tlb_ps = FIELD_EX64(env->CSR_STLBPS, CSR_STLBPS, PS);
>       }
> -    pagesize = 1 << tlb_ps;
> +    pagesize = 1UL << tlb_ps;
>       mask = MAKE_64BIT_MASK(0, tlb_ps + 1);

This is incorrect, because 1ul is still 32 bits on some hosts, including windows64.
More generally, 'ul' is *always* incorrect for qemu.

Much better here to use

     pagesize = MAKE_64BIT_MASK(tlb_ps, 1);

> -            tmp0 += (1 << ps);
> +            tmp0 += (1UL << ps);

Likewise.


r~

Re: [PATCH 5/5] target/loongarch/op_helper: Fix coverity cond_at_most error

[Richard Henderson]

On 7/12/22 13:31, Xiaojuan Yang wrote:
> The boundary size of cpucfg array should be 0 to 20. So,
> using index bigger than 20 to access cpucfg[] must be forbidden.
> 
> Fix coverity CID: 1489760
> 
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> ---
>   target/loongarch/op_helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/loongarch/op_helper.c b/target/loongarch/op_helper.c
> index 4b429b6699..b05a0b7648 100644
> --- a/target/loongarch/op_helper.c
> +++ b/target/loongarch/op_helper.c
> @@ -81,7 +81,7 @@ target_ulong helper_crc32c(target_ulong val, target_ulong m, uint64_t sz)
>   
>   target_ulong helper_cpucfg(CPULoongArchState *env, target_ulong rj)
>   {
> -    return rj > 21 ? 0 : env->cpucfg[rj];
> +    return rj > 20 ? 0 : env->cpucfg[rj];

Better using ARRAY_SIZE(env->cpucfg).


r~

Re: [PATCH 2/5] hw/intc/loongarch_pch_pic: Fix coverity errors in update irq

[Richard Henderson]

On 7/12/22 13:31, Xiaojuan Yang wrote:
> Fix coverity errors:
> 1. In find_first_bit function, the 'size' argument need
> 'unsigned long' type, so we add the suffix 'UL' on 'size'
> argument when use the function.
> 2. In expression 1ULL << irq, left shifting by more than
> 63 bits has undefined behavior. And out-of-bounds access
> error occured when 'irq' >= 64. So we add a condition to
> avoid this.
> 
> Fix coverity CID: 1489761 1489764 1489765
> 
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> ---
>   hw/intc/loongarch_pch_pic.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/intc/loongarch_pch_pic.c b/hw/intc/loongarch_pch_pic.c
> index 3c9814a3b4..779a087a03 100644
> --- a/hw/intc/loongarch_pch_pic.c
> +++ b/hw/intc/loongarch_pch_pic.c
> @@ -21,16 +21,20 @@ static void pch_pic_update_irq(LoongArchPCHPIC *s, uint64_t mask, int level)
>       if (level) {
>           val = mask & s->intirr & ~s->int_mask;
>           if (val) {
> -            irq = find_first_bit(&val, 64);
> -            s->intisr |= 0x1ULL << irq;
> -            qemu_set_irq(s->parent_irq[s->htmsi_vector[irq]], 1);
> +            irq = find_first_bit(&val, 64UL);
> +            if (irq < 64) {
> +                s->intisr |= 0x1ULL << irq;
> +                qemu_set_irq(s->parent_irq[s->htmsi_vector[irq]], 1);
> +            }
>           }
>       } else {
>           val = mask & s->intisr;
>           if (val) {
> -            irq = find_first_bit(&val, 64);
> -            s->intisr &= ~(0x1ULL << irq);
> -            qemu_set_irq(s->parent_irq[s->htmsi_vector[irq]], 0);
> +            irq = find_first_bit(&val, 64UL);
> +            if (irq < 64) {
> +                s->intisr &= ~(0x1ULL << irq);
> +                qemu_set_irq(s->parent_irq[s->htmsi_vector[irq]], 0);
> +            }

This needs to be rewritten.  I mentioned this multiple times during review, but this 
instance seems to have slipped through anyway.

You absolutely cannot use find_first_bit etc on raw 'unsigned long' -- bitmap.h functions 
may only be used on DECLARE_BITMAP objects.

That said, there is no point in using any of the bitmap.h functions here.  You should 
simply use uint64_t val, and ctz64() instead of find_first_bit().


r~

Re: [PATCH 1/5] target/loongarch/cpu: Fix cpu_class_by_name function

[Richard Henderson]

On 7/12/22 13:31, Xiaojuan Yang wrote:
> In loongarch_cpu_class_by_name(char *cpu_model) function,
> the argument cpu_model already has the suffix '-loongarch-cpu',
> so we should remove the LOONGARCH_CPU_TYPE_NAME(cpu_model) macro.
> 
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> ---
>   target/loongarch/cpu.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index e21715592a..8294b05ee8 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -571,11 +571,8 @@ static void loongarch_cpu_init(Object *obj)
>   static ObjectClass *loongarch_cpu_class_by_name(const char *cpu_model)
>   {
>       ObjectClass *oc;
> -    char *typename;
>   
> -    typename = g_strdup_printf(LOONGARCH_CPU_TYPE_NAME("%s"), cpu_model);
> -    oc = object_class_by_name(typename);
> -    g_free(typename);
> +    oc = object_class_by_name(cpu_model);
>       return oc;
>   }
>   

Looks like we're missing the assertion that 'cpu_model' resolves to a class of the 
appropriate type.  From e.g riscv/cpu.c,

     if (!oc || !object_class_dynamic_cast(oc, TYPE_RISCV_CPU) ||

         object_class_is_abstract(oc)) {

         return NULL;

     }



r~

Re: [PATCH 3/5] target/loongarch/cpu: Fix coverity errors about excp_names

[Richard Henderson]

On 7/13/22 15:20, Xiaojuan Yang wrote:
> Fix out-of-bounds errors when access excp_names[] array. the valid
> boundary size of excp_names should be 0 to ARRAY_SIZE(excp_names)-1.
> However, the general code do not consider the max boundary.
> 
> Fix coverity CID: 1489758
> 
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> ---
>   target/loongarch/cpu.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

I gave you a reviewed-by for this patch in v1.
You must copy those into v2 so that I don't have to do it again.


r~

> 
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index ed26f9beed..89ea971cde 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -140,7 +140,7 @@ static void loongarch_cpu_do_interrupt(CPUState *cs)
>   
>       if (cs->exception_index != EXCCODE_INT) {
>           if (cs->exception_index < 0 ||
> -            cs->exception_index > ARRAY_SIZE(excp_names)) {
> +            cs->exception_index >= ARRAY_SIZE(excp_names)) {
>               name = "unknown";
>           } else {
>               name = excp_names[cs->exception_index];
> @@ -190,8 +190,8 @@ static void loongarch_cpu_do_interrupt(CPUState *cs)
>           cause = cs->exception_index;
>           break;
>       default:
> -        qemu_log("Error: exception(%d) '%s' has not been supported\n",
> -                 cs->exception_index, excp_names[cs->exception_index]);
> +        qemu_log("Error: exception(%d) has not been supported\n",
> +                 cs->exception_index);
>           abort();
>       }
>   

[PATCH 3/5] target/loongarch/cpu: Fix coverity errors about excp_names

[Xiaojuan Yang]

Fix out-of-bounds errors when access excp_names[] array. the valid
boundary size of excp_names should be 0 to ARRAY_SIZE(excp_names)-1.
However, the general code do not consider the max boundary.

Fix coverity CID: 1489758

Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
---
 target/loongarch/cpu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index ed26f9beed..89ea971cde 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -140,7 +140,7 @@ static void loongarch_cpu_do_interrupt(CPUState *cs)
 
     if (cs->exception_index != EXCCODE_INT) {
         if (cs->exception_index < 0 ||
-            cs->exception_index > ARRAY_SIZE(excp_names)) {
+            cs->exception_index >= ARRAY_SIZE(excp_names)) {
             name = "unknown";
         } else {
             name = excp_names[cs->exception_index];
@@ -190,8 +190,8 @@ static void loongarch_cpu_do_interrupt(CPUState *cs)
         cause = cs->exception_index;
         break;
     default:
-        qemu_log("Error: exception(%d) '%s' has not been supported\n",
-                 cs->exception_index, excp_names[cs->exception_index]);
+        qemu_log("Error: exception(%d) has not been supported\n",
+                 cs->exception_index);
         abort();
     }
 
-- 
2.31.1