[PATCH] accel/tcg: Assert mmu_idx in range before use in cputlb

[PATCH] accel/tcg: Assert mmu_idx in range before use in cputlb

[Richard Henderson]

Coverity reports out-of-bound accesses within cputlb.c.
This should be a false positive due to how the index is
decoded from MemOpIdx.  To be fair, nothing is checking
the correct bounds during encoding either.

Assert index in range before use, both to catch user errors
and to pacify static analysis.

Fixes: Coverity CID 1487120, 1487127, 1487170, 1487196, 1487215, 1487238
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/cputlb.c | 40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index dd45e0467b..f90f4312ea 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1761,7 +1761,7 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
                                MemOpIdx oi, int size, int prot,
                                uintptr_t retaddr)
 {
-    size_t mmu_idx = get_mmuidx(oi);
+    uintptr_t mmu_idx = get_mmuidx(oi);
     MemOp mop = get_memop(oi);
     int a_bits = get_alignment_bits(mop);
     uintptr_t index;
@@ -1769,6 +1769,8 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
     target_ulong tlb_addr;
     void *hostaddr;
 
+    tcg_debug_assert(mmu_idx < NB_MMU_MODES);
+
     /* Adjust the given return address.  */
     retaddr -= GETPC_ADJ;
 
@@ -1908,18 +1910,20 @@ load_helper(CPUArchState *env, target_ulong addr, MemOpIdx oi,
             uintptr_t retaddr, MemOp op, bool code_read,
             FullLoadHelper *full_load)
 {
-    uintptr_t mmu_idx = get_mmuidx(oi);
-    uintptr_t index = tlb_index(env, mmu_idx, addr);
-    CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
-    target_ulong tlb_addr = code_read ? entry->addr_code : entry->addr_read;
     const size_t tlb_off = code_read ?
         offsetof(CPUTLBEntry, addr_code) : offsetof(CPUTLBEntry, addr_read);
     const MMUAccessType access_type =
         code_read ? MMU_INST_FETCH : MMU_DATA_LOAD;
-    unsigned a_bits = get_alignment_bits(get_memop(oi));
+    const unsigned a_bits = get_alignment_bits(get_memop(oi));
+    const size_t size = memop_size(op);
+    uintptr_t mmu_idx = get_mmuidx(oi);
+    uintptr_t index;
+    CPUTLBEntry *entry;
+    target_ulong tlb_addr;
     void *haddr;
     uint64_t res;
-    size_t size = memop_size(op);
+
+    tcg_debug_assert(mmu_idx < NB_MMU_MODES);
 
     /* Handle CPU specific unaligned behaviour */
     if (addr & ((1 << a_bits) - 1)) {
@@ -1927,6 +1931,10 @@ load_helper(CPUArchState *env, target_ulong addr, MemOpIdx oi,
                              mmu_idx, retaddr);
     }
 
+    index = tlb_index(env, mmu_idx, addr);
+    entry = tlb_entry(env, mmu_idx, addr);
+    tlb_addr = code_read ? entry->addr_code : entry->addr_read;
+
     /* If the TLB entry is for a different page, reload and try again.  */
     if (!tlb_hit(tlb_addr, addr)) {
         if (!victim_tlb_hit(env, mmu_idx, index, tlb_off,
@@ -2310,14 +2318,16 @@ static inline void QEMU_ALWAYS_INLINE
 store_helper(CPUArchState *env, target_ulong addr, uint64_t val,
              MemOpIdx oi, uintptr_t retaddr, MemOp op)
 {
-    uintptr_t mmu_idx = get_mmuidx(oi);
-    uintptr_t index = tlb_index(env, mmu_idx, addr);
-    CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
-    target_ulong tlb_addr = tlb_addr_write(entry);
     const size_t tlb_off = offsetof(CPUTLBEntry, addr_write);
-    unsigned a_bits = get_alignment_bits(get_memop(oi));
+    const unsigned a_bits = get_alignment_bits(get_memop(oi));
+    const size_t size = memop_size(op);
+    uintptr_t mmu_idx = get_mmuidx(oi);
+    uintptr_t index;
+    CPUTLBEntry *entry;
+    target_ulong tlb_addr;
     void *haddr;
-    size_t size = memop_size(op);
+
+    tcg_debug_assert(mmu_idx < NB_MMU_MODES);
 
     /* Handle CPU specific unaligned behaviour */
     if (addr & ((1 << a_bits) - 1)) {
@@ -2325,6 +2335,10 @@ store_helper(CPUArchState *env, target_ulong addr, uint64_t val,
                              mmu_idx, retaddr);
     }
 
+    index = tlb_index(env, mmu_idx, addr);
+    entry = tlb_entry(env, mmu_idx, addr);
+    tlb_addr = tlb_addr_write(entry);
+
     /* If the TLB entry is for a different page, reload and try again.  */
     if (!tlb_hit(tlb_addr, addr)) {
         if (!victim_tlb_hit(env, mmu_idx, index, tlb_off,
-- 
2.25.1

Re: [PATCH] accel/tcg: Assert mmu_idx in range before use in cputlb

[Richard Henderson]

Ping.

On 4/1/22 10:08, Richard Henderson wrote:
> Coverity reports out-of-bound accesses within cputlb.c.
> This should be a false positive due to how the index is
> decoded from MemOpIdx.  To be fair, nothing is checking
> the correct bounds during encoding either.
> 
> Assert index in range before use, both to catch user errors
> and to pacify static analysis.
> 
> Fixes: Coverity CID 1487120, 1487127, 1487170, 1487196, 1487215, 1487238
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   accel/tcg/cputlb.c | 40 +++++++++++++++++++++++++++-------------
>   1 file changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index dd45e0467b..f90f4312ea 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1761,7 +1761,7 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
>                                  MemOpIdx oi, int size, int prot,
>                                  uintptr_t retaddr)
>   {
> -    size_t mmu_idx = get_mmuidx(oi);
> +    uintptr_t mmu_idx = get_mmuidx(oi);
>       MemOp mop = get_memop(oi);
>       int a_bits = get_alignment_bits(mop);
>       uintptr_t index;
> @@ -1769,6 +1769,8 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
>       target_ulong tlb_addr;
>       void *hostaddr;
>   
> +    tcg_debug_assert(mmu_idx < NB_MMU_MODES);
> +
>       /* Adjust the given return address.  */
>       retaddr -= GETPC_ADJ;
>   
> @@ -1908,18 +1910,20 @@ load_helper(CPUArchState *env, target_ulong addr, MemOpIdx oi,
>               uintptr_t retaddr, MemOp op, bool code_read,
>               FullLoadHelper *full_load)
>   {
> -    uintptr_t mmu_idx = get_mmuidx(oi);
> -    uintptr_t index = tlb_index(env, mmu_idx, addr);
> -    CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
> -    target_ulong tlb_addr = code_read ? entry->addr_code : entry->addr_read;
>       const size_t tlb_off = code_read ?
>           offsetof(CPUTLBEntry, addr_code) : offsetof(CPUTLBEntry, addr_read);
>       const MMUAccessType access_type =
>           code_read ? MMU_INST_FETCH : MMU_DATA_LOAD;
> -    unsigned a_bits = get_alignment_bits(get_memop(oi));
> +    const unsigned a_bits = get_alignment_bits(get_memop(oi));
> +    const size_t size = memop_size(op);
> +    uintptr_t mmu_idx = get_mmuidx(oi);
> +    uintptr_t index;
> +    CPUTLBEntry *entry;
> +    target_ulong tlb_addr;
>       void *haddr;
>       uint64_t res;
> -    size_t size = memop_size(op);
> +
> +    tcg_debug_assert(mmu_idx < NB_MMU_MODES);
>   
>       /* Handle CPU specific unaligned behaviour */
>       if (addr & ((1 << a_bits) - 1)) {
> @@ -1927,6 +1931,10 @@ load_helper(CPUArchState *env, target_ulong addr, MemOpIdx oi,
>                                mmu_idx, retaddr);
>       }
>   
> +    index = tlb_index(env, mmu_idx, addr);
> +    entry = tlb_entry(env, mmu_idx, addr);
> +    tlb_addr = code_read ? entry->addr_code : entry->addr_read;
> +
>       /* If the TLB entry is for a different page, reload and try again.  */
>       if (!tlb_hit(tlb_addr, addr)) {
>           if (!victim_tlb_hit(env, mmu_idx, index, tlb_off,
> @@ -2310,14 +2318,16 @@ static inline void QEMU_ALWAYS_INLINE
>   store_helper(CPUArchState *env, target_ulong addr, uint64_t val,
>                MemOpIdx oi, uintptr_t retaddr, MemOp op)
>   {
> -    uintptr_t mmu_idx = get_mmuidx(oi);
> -    uintptr_t index = tlb_index(env, mmu_idx, addr);
> -    CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
> -    target_ulong tlb_addr = tlb_addr_write(entry);
>       const size_t tlb_off = offsetof(CPUTLBEntry, addr_write);
> -    unsigned a_bits = get_alignment_bits(get_memop(oi));
> +    const unsigned a_bits = get_alignment_bits(get_memop(oi));
> +    const size_t size = memop_size(op);
> +    uintptr_t mmu_idx = get_mmuidx(oi);
> +    uintptr_t index;
> +    CPUTLBEntry *entry;
> +    target_ulong tlb_addr;
>       void *haddr;
> -    size_t size = memop_size(op);
> +
> +    tcg_debug_assert(mmu_idx < NB_MMU_MODES);
>   
>       /* Handle CPU specific unaligned behaviour */
>       if (addr & ((1 << a_bits) - 1)) {
> @@ -2325,6 +2335,10 @@ store_helper(CPUArchState *env, target_ulong addr, uint64_t val,
>                                mmu_idx, retaddr);
>       }
>   
> +    index = tlb_index(env, mmu_idx, addr);
> +    entry = tlb_entry(env, mmu_idx, addr);
> +    tlb_addr = tlb_addr_write(entry);
> +
>       /* If the TLB entry is for a different page, reload and try again.  */
>       if (!tlb_hit(tlb_addr, addr)) {
>           if (!victim_tlb_hit(env, mmu_idx, index, tlb_off,

Re: [PATCH] accel/tcg: Assert mmu_idx in range before use in cputlb

[Peter Maydell]

On Fri, 1 Apr 2022 at 18:08, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Coverity reports out-of-bound accesses within cputlb.c.
> This should be a false positive due to how the index is
> decoded from MemOpIdx.  To be fair, nothing is checking
> the correct bounds during encoding either.
>
> Assert index in range before use, both to catch user errors
> and to pacify static analysis.
>
> Fixes: Coverity CID 1487120, 1487127, 1487170, 1487196, 1487215, 1487238
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  accel/tcg/cputlb.c | 40 +++++++++++++++++++++++++++-------------
>  1 file changed, 27 insertions(+), 13 deletions(-)
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH] accel/tcg: Assert mmu_idx in range before use in cputlb

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Coverity reports out-of-bound accesses within cputlb.c.
> This should be a false positive due to how the index is
> decoded from MemOpIdx.  To be fair, nothing is checking
> the correct bounds during encoding either.
>
> Assert index in range before use, both to catch user errors
> and to pacify static analysis.
>
> Fixes: Coverity CID 1487120, 1487127, 1487170, 1487196, 1487215, 1487238
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH] accel/tcg: Assert mmu_idx in range before use in cputlb

[Richard Henderson]

On 4/1/22 10:08, Richard Henderson wrote:
> Coverity reports out-of-bound accesses within cputlb.c.
> This should be a false positive due to how the index is
> decoded from MemOpIdx.  To be fair, nothing is checking
> the correct bounds during encoding either.
> 
> Assert index in range before use, both to catch user errors
> and to pacify static analysis.
> 
> Fixes: Coverity CID 1487120, 1487127, 1487170, 1487196, 1487215, 1487238
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Queuing to tcg-next.

r~