In get_physical_address, the canonical address check failed to
set TranslateFault.stage2, which resulted in an uninitialized
read from the struct when reporting the fault in x86_cpu_tlb_fill.

Adjust all error paths to use structure assignment so that the
entire struct is always initialized.

Reported-by: Daniel Hoffman <dhoff749@gmail.com>
Fixes: 9bbcf372193a ("target/i386: Reorg GET_HPHYS")
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/tcg/sysemu/excp_helper.c | 34 ++++++++++++++++------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
index 405a5d414a..55bd1194d3 100644
--- a/target/i386/tcg/sysemu/excp_helper.c
+++ b/target/i386/tcg/sysemu/excp_helper.c
@@ -71,10 +71,11 @@ static bool ptw_translate(PTETranslate *inout, hwaddr addr)
         TranslateFault *err = inout->err;
 
         assert(inout->ptw_idx == MMU_NESTED_IDX);
-        err->exception_index = 0; /* unused */
-        err->error_code = inout->env->error_code;
-        err->cr2 = addr;
-        err->stage2 = S2_GPT;
+        *err = (TranslateFault){
+            .error_code = inout->env->error_code,
+            .cr2 = addr,
+            .stage2 = S2_GPT,
+        };
         return false;
     }
     return true;
@@ -431,10 +432,11 @@ do_check_protect_pse36:
                                   MMU_NESTED_IDX, true,
                                   &pte_trans.haddr, &full, 0);
         if (unlikely(flags & TLB_INVALID_MASK)) {
-            err->exception_index = 0; /* unused */
-            err->error_code = env->error_code;
-            err->cr2 = paddr;
-            err->stage2 = S2_GPA;
+            *err = (TranslateFault){
+                .error_code = env->error_code,
+                .cr2 = paddr,
+                .stage2 = S2_GPA,
+            };
             return false;
         }
 
@@ -494,10 +496,11 @@ do_check_protect_pse36:
         }
         break;
     }
-    err->exception_index = EXCP0E_PAGE;
-    err->error_code = error_code;
-    err->cr2 = addr;
-    err->stage2 = S2_NONE;
+    *err = (TranslateFault){
+        .exception_index = EXCP0E_PAGE,
+        .error_code = error_code,
+        .cr2 = addr,
+    };
     return false;
 }
 
@@ -564,9 +567,10 @@ static bool get_physical_address(CPUX86State *env, vaddr addr,
                 int shift = in.pg_mode & PG_MODE_LA57 ? 56 : 47;
                 int64_t sext = (int64_t)addr >> shift;
                 if (sext != 0 && sext != -1) {
-                    err->exception_index = EXCP0D_GPF;
-                    err->error_code = 0;
-                    err->cr2 = addr;
+                    *err = (TranslateFault){
+                        .exception_index = EXCP0D_GPF,
+                        .cr2 = addr,
+                    };
                     return false;
                 }
             }
-- 
2.34.1

Queued, thanks.

Paolo

On 11/30/22 23:45, Richard Henderson wrote:
> In get_physical_address, the canonical address check failed to
> set TranslateFault.stage2, which resulted in an uninitialized
> read from the struct when reporting the fault in x86_cpu_tlb_fill.
> 
> Adjust all error paths to use structure assignment so that the
> entire struct is always initialized.
> 
> Reported-by: Daniel Hoffman <dhoff749@gmail.com>
> Fixes: 9bbcf372193a ("target/i386: Reorg GET_HPHYS")
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/i386/tcg/sysemu/excp_helper.c | 34 ++++++++++++++++------------
>   1 file changed, 19 insertions(+), 15 deletions(-)

Ah hah, I just found the issue filed for this:

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1324


r~

> 
> diff --git a/target/i386/tcg/sysemu/excp_helper.c b/target/i386/tcg/sysemu/excp_helper.c
> index 405a5d414a..55bd1194d3 100644
> --- a/target/i386/tcg/sysemu/excp_helper.c
> +++ b/target/i386/tcg/sysemu/excp_helper.c
> @@ -71,10 +71,11 @@ static bool ptw_translate(PTETranslate *inout, hwaddr addr)
>           TranslateFault *err = inout->err;
>   
>           assert(inout->ptw_idx == MMU_NESTED_IDX);
> -        err->exception_index = 0; /* unused */
> -        err->error_code = inout->env->error_code;
> -        err->cr2 = addr;
> -        err->stage2 = S2_GPT;
> +        *err = (TranslateFault){
> +            .error_code = inout->env->error_code,
> +            .cr2 = addr,
> +            .stage2 = S2_GPT,
> +        };
>           return false;
>       }
>       return true;
> @@ -431,10 +432,11 @@ do_check_protect_pse36:
>                                     MMU_NESTED_IDX, true,
>                                     &pte_trans.haddr, &full, 0);
>           if (unlikely(flags & TLB_INVALID_MASK)) {
> -            err->exception_index = 0; /* unused */
> -            err->error_code = env->error_code;
> -            err->cr2 = paddr;
> -            err->stage2 = S2_GPA;
> +            *err = (TranslateFault){
> +                .error_code = env->error_code,
> +                .cr2 = paddr,
> +                .stage2 = S2_GPA,
> +            };
>               return false;
>           }
>   
> @@ -494,10 +496,11 @@ do_check_protect_pse36:
>           }
>           break;
>       }
> -    err->exception_index = EXCP0E_PAGE;
> -    err->error_code = error_code;
> -    err->cr2 = addr;
> -    err->stage2 = S2_NONE;
> +    *err = (TranslateFault){
> +        .exception_index = EXCP0E_PAGE,
> +        .error_code = error_code,
> +        .cr2 = addr,
> +    };
>       return false;
>   }
>   
> @@ -564,9 +567,10 @@ static bool get_physical_address(CPUX86State *env, vaddr addr,
>                   int shift = in.pg_mode & PG_MODE_LA57 ? 56 : 47;
>                   int64_t sext = (int64_t)addr >> shift;
>                   if (sext != 0 && sext != -1) {
> -                    err->exception_index = EXCP0D_GPF;
> -                    err->error_code = 0;
> -                    err->cr2 = addr;
> +                    *err = (TranslateFault){
> +                        .exception_index = EXCP0D_GPF,
> +                        .cr2 = addr,
> +                    };
>                       return false;
>                   }
>               }