[PATCH for-8.1] target/mips: Avoid shift by negative number in page_table_walk_refill()

[PATCH for-8.1] target/mips: Avoid shift by negative number in page_table_walk_refill()

[Peter Maydell]

Coverity points out that in page_table_walk_refill() we can shift by
a negative number, which is undefined behaviour (CID 1452918,
1452920, 1452922).  We already catch the negative directory_shift and
leaf_shift as being a "bail out early" case, but not until we've
already used them to calculated some offset values.

Move the calculation of the offset values to after we've done the
"return early if directory_shift or leaf_shift are -1" check.

Since walk_directory() re-calculates these shift values, add an
assert() to tell Coverity that the caller has already ensured they
won't be negative.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/mips/tcg/sysemu/tlb_helper.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/target/mips/tcg/sysemu/tlb_helper.c b/target/mips/tcg/sysemu/tlb_helper.c
index e5e1e9dd3ff..c67c2b09026 100644
--- a/target/mips/tcg/sysemu/tlb_helper.c
+++ b/target/mips/tcg/sysemu/tlb_helper.c
@@ -643,6 +643,9 @@ static int walk_directory(CPUMIPSState *env, uint64_t *vaddr,
     uint64_t lsb = 0;
     uint64_t w = 0;
 
+    /* The caller should have checked this */
+    assert(directory_shift > 0 && leaf_shift > 0);
+
     if (get_physical_address(env, &paddr, &prot, *vaddr, MMU_DATA_LOAD,
                              cpu_mmu_index(env, false)) !=
                              TLBRET_MATCH) {
@@ -743,13 +746,8 @@ static bool page_table_walk_refill(CPUMIPSState *env, vaddr address,
             (ptew == 1) ? native_shift + 1 : native_shift;
 
     /* Offsets into tables */
-    int goffset = gindex << directory_shift;
-    int uoffset = uindex << directory_shift;
-    int moffset = mindex << directory_shift;
-    int ptoffset0 = (ptindex >> 1) << (leaf_shift + 1);
-    int ptoffset1 = ptoffset0 | (1 << (leaf_shift));
-
-    uint32_t leafentry_size = 1 << (leaf_shift + 3);
+    int goffset, uoffset, moffset, ptoffset0, ptoffset1;
+    uint32_t leafentry_size;
 
     /* Starting address - Page Table Base */
     uint64_t vaddr = env->CP0_PWBase;
@@ -775,6 +773,14 @@ static bool page_table_walk_refill(CPUMIPSState *env, vaddr address,
         return false;
     }
 
+    goffset = gindex << directory_shift;
+    uoffset = uindex << directory_shift;
+    moffset = mindex << directory_shift;
+    ptoffset0 = (ptindex >> 1) << (leaf_shift + 1);
+    ptoffset1 = ptoffset0 | (1 << (leaf_shift));
+
+    leafentry_size = 1 << (leaf_shift + 3);
+
     /* Global Directory */
     if (gdw > 0) {
         vaddr |= goffset;
-- 
2.34.1

Re: [PATCH for-8.1] target/mips: Avoid shift by negative number in page_table_walk_refill()

[Philippe Mathieu-Daudé]

On 17/7/23 18:29, Peter Maydell wrote:
> Coverity points out that in page_table_walk_refill() we can shift by
> a negative number, which is undefined behaviour (CID 1452918,
> 1452920, 1452922).  We already catch the negative directory_shift and
> leaf_shift as being a "bail out early" case, but not until we've
> already used them to calculated some offset values.
> 
> Move the calculation of the offset values to after we've done the
> "return early if directory_shift or leaf_shift are -1" check.
> 
> Since walk_directory() re-calculates these shift values, add an
> assert() to tell Coverity that the caller has already ensured they
> won't be negative.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/mips/tcg/sysemu/tlb_helper.c | 20 +++++++++++++-------
>   1 file changed, 13 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH for-8.1] target/mips: Avoid shift by negative number in page_table_walk_refill()

[Peter Maydell]

On Mon, 17 Jul 2023 at 17:29, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Coverity points out that in page_table_walk_refill() we can shift by
> a negative number, which is undefined behaviour (CID 1452918,
> 1452920, 1452922).  We already catch the negative directory_shift and
> leaf_shift as being a "bail out early" case, but not until we've
> already used them to calculated some offset values.
>
> Move the calculation of the offset values to after we've done the
> "return early if directory_shift or leaf_shift are -1" check.
>
> Since walk_directory() re-calculates these shift values, add an
> assert() to tell Coverity that the caller has already ensured they
> won't be negative.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/mips/tcg/sysemu/tlb_helper.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/target/mips/tcg/sysemu/tlb_helper.c b/target/mips/tcg/sysemu/tlb_helper.c
> index e5e1e9dd3ff..c67c2b09026 100644
> --- a/target/mips/tcg/sysemu/tlb_helper.c
> +++ b/target/mips/tcg/sysemu/tlb_helper.c
> @@ -643,6 +643,9 @@ static int walk_directory(CPUMIPSState *env, uint64_t *vaddr,
>      uint64_t lsb = 0;
>      uint64_t w = 0;
>
> +    /* The caller should have checked this */
> +    assert(directory_shift > 0 && leaf_shift > 0);

Whoops, this should be >= 0 && ... >= 0.

-- PMM