[PATCH] target-i386: mmu: fix handling of noncanonical virtual addresses

[PATCH] target-i386: mmu: fix handling of noncanonical virtual addresses

[Paolo Bonzini]

mmu_translate is supposed to return an error code for page faults; it is
not able to handle other exceptions.  The #GP case for noncanonical
virtual addresses is not handled correctly, and incorrectly raised as
a page fault with error code 1.  Since it cannot happen for nested
page tables, move it directly to handle_mmu_fault, even before the
invocation of mmu_translate.

Fixes: 661ff4879e ("target/i386: extract mmu_translate", 2021-05-11)
Cc: qemu-stable@nongnu.org
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Fixes: #676
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/sysemu/excp_helper.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
index 492b777de9..5ba739fbed 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -94,15 +94,6 @@ static int mmu_translate(CPUState *cs, hwaddr addr, MMUTranslateFunc get_hphys_f
             bool la57 = pg_mode & PG_MODE_LA57;
             uint64_t pml5e_addr, pml5e;
             uint64_t pml4e_addr, pml4e;
-            int32_t sext;
-
-            /* test virtual address sign extension */
-            sext = la57 ? (int64_t)addr >> 56 : (int64_t)addr >> 47;
-            if (get_hphys_func && sext != 0 && sext != -1) {
-                env->error_code = 0;
-                cs->exception_index = EXCP0D_GPF;
-                return 1;
-            }
 
             if (la57) {
                 pml5e_addr = ((cr3 & ~0xfff) +
@@ -423,6 +414,18 @@ static int handle_mmu_fault(CPUState *cs, vaddr addr, int size,
         page_size = 4096;
     } else {
         pg_mode = get_pg_mode(env);
+        if (pg_mode & PG_MODE_LMA) {
+            int32_t sext;
+
+            /* test virtual address sign extension */
+            sext = (int64_t)addr >> (pg_mode & PG_MODE_LA57 ? 56 : 47);
+            if (sext != 0 && sext != -1) {
+                env->error_code = 0;
+                cs->exception_index = EXCP0D_GPF;
+                return 1;
+            }
+        }
+
         error_code = mmu_translate(cs, addr, get_hphys, env->cr[3], is_write1,
                                    mmu_idx, pg_mode,
                                    &paddr, &page_size, &prot);
-- 
2.33.1

Re: [PATCH] target-i386: mmu: fix handling of noncanonical virtual addresses

[Mark Cave-Ayland]

On 04/11/2021 14:09, Paolo Bonzini wrote:

> mmu_translate is supposed to return an error code for page faults; it is
> not able to handle other exceptions.  The #GP case for noncanonical
> virtual addresses is not handled correctly, and incorrectly raised as
> a page fault with error code 1.  Since it cannot happen for nested
> page tables, move it directly to handle_mmu_fault, even before the
> invocation of mmu_translate.
> 
> Fixes: 661ff4879e ("target/i386: extract mmu_translate", 2021-05-11)
> Cc: qemu-stable@nongnu.org
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: #676
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/tcg/sysemu/excp_helper.c | 21 ++++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
> index 492b777de9..5ba739fbed 100644
> --- a/target/i386/tcg/sysemu/excp_helper.c
> +++ b/target/i386/tcg/sysemu/excp_helper.c
> @@ -94,15 +94,6 @@ static int mmu_translate(CPUState *cs, hwaddr addr, MMUTranslateFunc get_hphys_f
>               bool la57 = pg_mode & PG_MODE_LA57;
>               uint64_t pml5e_addr, pml5e;
>               uint64_t pml4e_addr, pml4e;
> -            int32_t sext;
> -
> -            /* test virtual address sign extension */
> -            sext = la57 ? (int64_t)addr >> 56 : (int64_t)addr >> 47;
> -            if (get_hphys_func && sext != 0 && sext != -1) {
> -                env->error_code = 0;
> -                cs->exception_index = EXCP0D_GPF;
> -                return 1;
> -            }
>   
>               if (la57) {
>                   pml5e_addr = ((cr3 & ~0xfff) +
> @@ -423,6 +414,18 @@ static int handle_mmu_fault(CPUState *cs, vaddr addr, int size,
>           page_size = 4096;
>       } else {
>           pg_mode = get_pg_mode(env);
> +        if (pg_mode & PG_MODE_LMA) {
> +            int32_t sext;
> +
> +            /* test virtual address sign extension */
> +            sext = (int64_t)addr >> (pg_mode & PG_MODE_LA57 ? 56 : 47);
> +            if (sext != 0 && sext != -1) {
> +                env->error_code = 0;
> +                cs->exception_index = EXCP0D_GPF;
> +                return 1;
> +            }
> +        }
> +
>           error_code = mmu_translate(cs, addr, get_hphys, env->cr[3], is_write1,
>                                      mmu_idx, pg_mode,
>                                      &paddr, &page_size, &prot);

I just gave this patch a quick test with my Windows 7 image and it now boots without 
issue. Thanks Paolo!

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> (Windows 7)


ATB,

Mark.