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