Patchwork [v2] x86: Fixed incorrect segment base address addition in 64-bits mode

login
register
mail settings
Submitter Vitaly Chipounov
Date July 2, 2012, 10:20 p.m.
Message ID <1341267649-18231-1-git-send-email-vitaly.chipounov@epfl.ch>
Download mbox | patch
Permalink /patch/168651/
State New
Headers show

Comments

Vitaly Chipounov - July 2, 2012, 10:20 p.m.
According to the Intel manual
"Intel® 64 and IA-32 Architectures Software Developer’s Manual
Volume 3", "3.4.4 Segment Loading Instructions in IA-32e Mode":

"When in compatibility mode, FS and GS overrides operate as defined by
32-bit mode behavior regardless of the value loaded into the upper 32
linear-address bits of the hidden descriptor register base field.
Compatibility mode ignores the upper 32 bits when calculating an effective address."

However, the code misses the 64-bit mode case, where an instruction with
address and segment size override would be translated incorrectly. For example,
inc dword ptr gs:260h[ebx*4] gets incorrectly translated to:

(uint32_t)(gs.base + ebx * 4 + 0x260)
instead of
gs.base + (uint32_t)(ebx * 4 + 0x260)

Signed-off-by: Vitaly Chipounov <vitaly.chipounov@epfl.ch>
---
 target-i386/translate.c |   43 +++++++++++++++++++++++++------------------
 1 files changed, 25 insertions(+), 18 deletions(-)
Max Filippov - July 3, 2012, 12:52 p.m.
On Tue, Jul 3, 2012 at 2:20 AM, Vitaly Chipounov
<vitaly.chipounov@epfl.ch> wrote:
> According to the Intel manual
> "Intel® 64 and IA-32 Architectures Software Developer’s Manual
> Volume 3", "3.4.4 Segment Loading Instructions in IA-32e Mode":
>
> "When in compatibility mode, FS and GS overrides operate as defined by
> 32-bit mode behavior regardless of the value loaded into the upper 32
> linear-address bits of the hidden descriptor register base field.
> Compatibility mode ignores the upper 32 bits when calculating an effective address."
>
> However, the code misses the 64-bit mode case, where an instruction with
> address and segment size override would be translated incorrectly. For example,
> inc dword ptr gs:260h[ebx*4] gets incorrectly translated to:
>
> (uint32_t)(gs.base + ebx * 4 + 0x260)
> instead of
> gs.base + (uint32_t)(ebx * 4 + 0x260)
>
> Signed-off-by: Vitaly Chipounov <vitaly.chipounov@epfl.ch>

Reviewed-by: Max Filippov <jcmvbkbc@gmail.com>
Blue Swirl - July 29, 2012, 9:38 a.m.
Thanks, applied.

On Mon, Jul 2, 2012 at 10:20 PM, Vitaly Chipounov
<vitaly.chipounov@epfl.ch> wrote:
> According to the Intel manual
> "Intel® 64 and IA-32 Architectures Software Developer’s Manual
> Volume 3", "3.4.4 Segment Loading Instructions in IA-32e Mode":
>
> "When in compatibility mode, FS and GS overrides operate as defined by
> 32-bit mode behavior regardless of the value loaded into the upper 32
> linear-address bits of the hidden descriptor register base field.
> Compatibility mode ignores the upper 32 bits when calculating an effective address."
>
> However, the code misses the 64-bit mode case, where an instruction with
> address and segment size override would be translated incorrectly. For example,
> inc dword ptr gs:260h[ebx*4] gets incorrectly translated to:
>
> (uint32_t)(gs.base + ebx * 4 + 0x260)
> instead of
> gs.base + (uint32_t)(ebx * 4 + 0x260)
>
> Signed-off-by: Vitaly Chipounov <vitaly.chipounov@epfl.ch>
> ---
>  target-i386/translate.c |   43 +++++++++++++++++++++++++------------------
>  1 files changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index a902f4a..dfec735 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -456,12 +456,19 @@ static inline void gen_op_movl_A0_seg(int reg)
>      tcg_gen_ld32u_tl(cpu_A0, cpu_env, offsetof(CPUX86State, segs[reg].base) + REG_L_OFFSET);
>  }
>
> -static inline void gen_op_addl_A0_seg(int reg)
> +static inline void gen_op_addl_A0_seg(DisasContext *s, int reg)
>  {
>      tcg_gen_ld_tl(cpu_tmp0, cpu_env, offsetof(CPUX86State, segs[reg].base));
> -    tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0);
>  #ifdef TARGET_X86_64
> -    tcg_gen_andi_tl(cpu_A0, cpu_A0, 0xffffffff);
> +    if (CODE64(s)) {
> +        tcg_gen_andi_tl(cpu_A0, cpu_A0, 0xffffffff);
> +        tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0);
> +    } else {
> +        tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0);
> +        tcg_gen_andi_tl(cpu_A0, cpu_A0, 0xffffffff);
> +    }
> +#else
> +    tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0);
>  #endif
>  }
>
> @@ -617,7 +624,7 @@ static inline void gen_string_movl_A0_ESI(DisasContext *s)
>              override = R_DS;
>          gen_op_movl_A0_reg(R_ESI);
>          gen_op_andl_A0_ffff();
> -        gen_op_addl_A0_seg(override);
> +        gen_op_addl_A0_seg(s, override);
>      }
>  }
>
> @@ -638,7 +645,7 @@ static inline void gen_string_movl_A0_EDI(DisasContext *s)
>      } else {
>          gen_op_movl_A0_reg(R_EDI);
>          gen_op_andl_A0_ffff();
> -        gen_op_addl_A0_seg(R_ES);
> +        gen_op_addl_A0_seg(s, R_ES);
>      }
>  }
>
> @@ -2063,7 +2070,7 @@ static void gen_lea_modrm(DisasContext *s, int modrm, int *reg_ptr, int *offset_
>              } else
>  #endif
>              {
> -                gen_op_addl_A0_seg(override);
> +                gen_op_addl_A0_seg(s, override);
>              }
>          }
>      } else {
> @@ -2130,7 +2137,7 @@ static void gen_lea_modrm(DisasContext *s, int modrm, int *reg_ptr, int *offset_
>                  else
>                      override = R_DS;
>              }
> -            gen_op_addl_A0_seg(override);
> +            gen_op_addl_A0_seg(s, override);
>          }
>      }
>
> @@ -2207,7 +2214,7 @@ static void gen_add_A0_ds_seg(DisasContext *s)
>          } else
>  #endif
>          {
> -            gen_op_addl_A0_seg(override);
> +            gen_op_addl_A0_seg(s, override);
>          }
>      }
>  }
> @@ -2460,12 +2467,12 @@ static void gen_push_T0(DisasContext *s)
>          if (s->ss32) {
>              if (s->addseg) {
>                  tcg_gen_mov_tl(cpu_T[1], cpu_A0);
> -                gen_op_addl_A0_seg(R_SS);
> +                gen_op_addl_A0_seg(s, R_SS);
>              }
>          } else {
>              gen_op_andl_A0_ffff();
>              tcg_gen_mov_tl(cpu_T[1], cpu_A0);
> -            gen_op_addl_A0_seg(R_SS);
> +            gen_op_addl_A0_seg(s, R_SS);
>          }
>          gen_op_st_T0_A0(s->dflag + 1 + s->mem_index);
>          if (s->ss32 && !s->addseg)
> @@ -2500,11 +2507,11 @@ static void gen_push_T1(DisasContext *s)
>              gen_op_addl_A0_im(-4);
>          if (s->ss32) {
>              if (s->addseg) {
> -                gen_op_addl_A0_seg(R_SS);
> +                gen_op_addl_A0_seg(s, R_SS);
>              }
>          } else {
>              gen_op_andl_A0_ffff();
> -            gen_op_addl_A0_seg(R_SS);
> +            gen_op_addl_A0_seg(s, R_SS);
>          }
>          gen_op_st_T1_A0(s->dflag + 1 + s->mem_index);
>
> @@ -2528,10 +2535,10 @@ static void gen_pop_T0(DisasContext *s)
>          gen_op_movl_A0_reg(R_ESP);
>          if (s->ss32) {
>              if (s->addseg)
> -                gen_op_addl_A0_seg(R_SS);
> +                gen_op_addl_A0_seg(s, R_SS);
>          } else {
>              gen_op_andl_A0_ffff();
> -            gen_op_addl_A0_seg(R_SS);
> +            gen_op_addl_A0_seg(s, R_SS);
>          }
>          gen_op_ld_T0_A0(s->dflag + 1 + s->mem_index);
>      }
> @@ -2556,7 +2563,7 @@ static void gen_stack_A0(DisasContext *s)
>          gen_op_andl_A0_ffff();
>      tcg_gen_mov_tl(cpu_T[1], cpu_A0);
>      if (s->addseg)
> -        gen_op_addl_A0_seg(R_SS);
> +        gen_op_addl_A0_seg(s, R_SS);
>  }
>
>  /* NOTE: wrap around in 16 bit not fully handled */
> @@ -2569,7 +2576,7 @@ static void gen_pusha(DisasContext *s)
>          gen_op_andl_A0_ffff();
>      tcg_gen_mov_tl(cpu_T[1], cpu_A0);
>      if (s->addseg)
> -        gen_op_addl_A0_seg(R_SS);
> +        gen_op_addl_A0_seg(s, R_SS);
>      for(i = 0;i < 8; i++) {
>          gen_op_mov_TN_reg(OT_LONG, 0, 7 - i);
>          gen_op_st_T0_A0(OT_WORD + s->dflag + s->mem_index);
> @@ -2588,7 +2595,7 @@ static void gen_popa(DisasContext *s)
>      tcg_gen_mov_tl(cpu_T[1], cpu_A0);
>      tcg_gen_addi_tl(cpu_T[1], cpu_T[1], 16 <<  s->dflag);
>      if (s->addseg)
> -        gen_op_addl_A0_seg(R_SS);
> +        gen_op_addl_A0_seg(s, R_SS);
>      for(i = 0;i < 8; i++) {
>          /* ESP is not reloaded */
>          if (i != 3) {
> @@ -2638,7 +2645,7 @@ static void gen_enter(DisasContext *s, int esp_addend, int level)
>              gen_op_andl_A0_ffff();
>          tcg_gen_mov_tl(cpu_T[1], cpu_A0);
>          if (s->addseg)
> -            gen_op_addl_A0_seg(R_SS);
> +            gen_op_addl_A0_seg(s, R_SS);
>          /* push bp */
>          gen_op_mov_TN_reg(OT_LONG, 0, R_EBP);
>          gen_op_st_T0_A0(ot + s->mem_index);
> --
> 1.7.4.1
>
>

Patch

diff --git a/target-i386/translate.c b/target-i386/translate.c
index a902f4a..dfec735 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -456,12 +456,19 @@  static inline void gen_op_movl_A0_seg(int reg)
     tcg_gen_ld32u_tl(cpu_A0, cpu_env, offsetof(CPUX86State, segs[reg].base) + REG_L_OFFSET);
 }
 
-static inline void gen_op_addl_A0_seg(int reg)
+static inline void gen_op_addl_A0_seg(DisasContext *s, int reg)
 {
     tcg_gen_ld_tl(cpu_tmp0, cpu_env, offsetof(CPUX86State, segs[reg].base));
-    tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0);
 #ifdef TARGET_X86_64
-    tcg_gen_andi_tl(cpu_A0, cpu_A0, 0xffffffff);
+    if (CODE64(s)) {
+        tcg_gen_andi_tl(cpu_A0, cpu_A0, 0xffffffff);
+        tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0);
+    } else {
+        tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0);
+        tcg_gen_andi_tl(cpu_A0, cpu_A0, 0xffffffff);
+    }
+#else
+    tcg_gen_add_tl(cpu_A0, cpu_A0, cpu_tmp0);
 #endif
 }
 
@@ -617,7 +624,7 @@  static inline void gen_string_movl_A0_ESI(DisasContext *s)
             override = R_DS;
         gen_op_movl_A0_reg(R_ESI);
         gen_op_andl_A0_ffff();
-        gen_op_addl_A0_seg(override);
+        gen_op_addl_A0_seg(s, override);
     }
 }
 
@@ -638,7 +645,7 @@  static inline void gen_string_movl_A0_EDI(DisasContext *s)
     } else {
         gen_op_movl_A0_reg(R_EDI);
         gen_op_andl_A0_ffff();
-        gen_op_addl_A0_seg(R_ES);
+        gen_op_addl_A0_seg(s, R_ES);
     }
 }
 
@@ -2063,7 +2070,7 @@  static void gen_lea_modrm(DisasContext *s, int modrm, int *reg_ptr, int *offset_
             } else
 #endif
             {
-                gen_op_addl_A0_seg(override);
+                gen_op_addl_A0_seg(s, override);
             }
         }
     } else {
@@ -2130,7 +2137,7 @@  static void gen_lea_modrm(DisasContext *s, int modrm, int *reg_ptr, int *offset_
                 else
                     override = R_DS;
             }
-            gen_op_addl_A0_seg(override);
+            gen_op_addl_A0_seg(s, override);
         }
     }
 
@@ -2207,7 +2214,7 @@  static void gen_add_A0_ds_seg(DisasContext *s)
         } else
 #endif
         {
-            gen_op_addl_A0_seg(override);
+            gen_op_addl_A0_seg(s, override);
         }
     }
 }
@@ -2460,12 +2467,12 @@  static void gen_push_T0(DisasContext *s)
         if (s->ss32) {
             if (s->addseg) {
                 tcg_gen_mov_tl(cpu_T[1], cpu_A0);
-                gen_op_addl_A0_seg(R_SS);
+                gen_op_addl_A0_seg(s, R_SS);
             }
         } else {
             gen_op_andl_A0_ffff();
             tcg_gen_mov_tl(cpu_T[1], cpu_A0);
-            gen_op_addl_A0_seg(R_SS);
+            gen_op_addl_A0_seg(s, R_SS);
         }
         gen_op_st_T0_A0(s->dflag + 1 + s->mem_index);
         if (s->ss32 && !s->addseg)
@@ -2500,11 +2507,11 @@  static void gen_push_T1(DisasContext *s)
             gen_op_addl_A0_im(-4);
         if (s->ss32) {
             if (s->addseg) {
-                gen_op_addl_A0_seg(R_SS);
+                gen_op_addl_A0_seg(s, R_SS);
             }
         } else {
             gen_op_andl_A0_ffff();
-            gen_op_addl_A0_seg(R_SS);
+            gen_op_addl_A0_seg(s, R_SS);
         }
         gen_op_st_T1_A0(s->dflag + 1 + s->mem_index);
 
@@ -2528,10 +2535,10 @@  static void gen_pop_T0(DisasContext *s)
         gen_op_movl_A0_reg(R_ESP);
         if (s->ss32) {
             if (s->addseg)
-                gen_op_addl_A0_seg(R_SS);
+                gen_op_addl_A0_seg(s, R_SS);
         } else {
             gen_op_andl_A0_ffff();
-            gen_op_addl_A0_seg(R_SS);
+            gen_op_addl_A0_seg(s, R_SS);
         }
         gen_op_ld_T0_A0(s->dflag + 1 + s->mem_index);
     }
@@ -2556,7 +2563,7 @@  static void gen_stack_A0(DisasContext *s)
         gen_op_andl_A0_ffff();
     tcg_gen_mov_tl(cpu_T[1], cpu_A0);
     if (s->addseg)
-        gen_op_addl_A0_seg(R_SS);
+        gen_op_addl_A0_seg(s, R_SS);
 }
 
 /* NOTE: wrap around in 16 bit not fully handled */
@@ -2569,7 +2576,7 @@  static void gen_pusha(DisasContext *s)
         gen_op_andl_A0_ffff();
     tcg_gen_mov_tl(cpu_T[1], cpu_A0);
     if (s->addseg)
-        gen_op_addl_A0_seg(R_SS);
+        gen_op_addl_A0_seg(s, R_SS);
     for(i = 0;i < 8; i++) {
         gen_op_mov_TN_reg(OT_LONG, 0, 7 - i);
         gen_op_st_T0_A0(OT_WORD + s->dflag + s->mem_index);
@@ -2588,7 +2595,7 @@  static void gen_popa(DisasContext *s)
     tcg_gen_mov_tl(cpu_T[1], cpu_A0);
     tcg_gen_addi_tl(cpu_T[1], cpu_T[1], 16 <<  s->dflag);
     if (s->addseg)
-        gen_op_addl_A0_seg(R_SS);
+        gen_op_addl_A0_seg(s, R_SS);
     for(i = 0;i < 8; i++) {
         /* ESP is not reloaded */
         if (i != 3) {
@@ -2638,7 +2645,7 @@  static void gen_enter(DisasContext *s, int esp_addend, int level)
             gen_op_andl_A0_ffff();
         tcg_gen_mov_tl(cpu_T[1], cpu_A0);
         if (s->addseg)
-            gen_op_addl_A0_seg(R_SS);
+            gen_op_addl_A0_seg(s, R_SS);
         /* push bp */
         gen_op_mov_TN_reg(OT_LONG, 0, R_EBP);
         gen_op_st_T0_A0(ot + s->mem_index);