diff mbox

x86emul: fix {,i}mul and {,i}div

Message ID 57ED2E5A0200007800113BDC@prv-mh.provo.novell.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jan Beulich Sept. 29, 2016, 1:08 p.m. UTC 
Commit a3db233ede ("x86emul: use DstEax also for {,I}{MUL,DIV}") went
a little too far: DstEax and SrcEax weren't really meant to be used
together with ModRM - they assume modrm_reg remains zero by the time
the destination / source register pointer gets calculated. Don't fully
undo that commit though, but instead just correct the register pointer,
and don't use dst.val as input for mul and imul (div and idiv did avoid
that already).

Reported-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
x86emul: fix {,i}mul and {,i}div

Commit a3db233ede ("x86emul: use DstEax also for {,I}{MUL,DIV}") went
a little too far: DstEax and SrcEax weren't really meant to be used
together with ModRM - they assume modrm_reg remains zero by the time
the destination / source register pointer gets calculated. Don't fully
undo that commit though, but instead just correct the register pointer,
and don't use dst.val as input for mul and imul (div and idiv did avoid
that already).

Reported-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -3845,18 +3845,19 @@ x86_emulate(
             emulate_1op("neg", dst, _regs.eflags);
             break;
         case 4: /* mul */
+            dst.reg = (unsigned long *)&_regs.eax;
             _regs.eflags &= ~(EFLG_OF|EFLG_CF);
             switch ( dst.bytes )
             {
             case 1:
-                dst.val = (uint8_t)dst.val;
+                dst.val = (uint8_t)_regs.eax;
                 dst.val *= src.val;
                 if ( (uint8_t)dst.val != (uint16_t)dst.val )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
                 dst.bytes = 2;
                 break;
             case 2:
-                dst.val = (uint16_t)dst.val;
+                dst.val = (uint16_t)_regs.eax;
                 dst.val *= src.val;
                 if ( (uint16_t)dst.val != (uint32_t)dst.val )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
@@ -3864,7 +3865,7 @@ x86_emulate(
                 break;
 #ifdef __x86_64__
             case 4:
-                dst.val = (uint32_t)dst.val;
+                dst.val = _regs._eax;
                 dst.val *= src.val;
                 if ( (uint32_t)dst.val != dst.val )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
@@ -3873,7 +3874,7 @@ x86_emulate(
 #endif
             default:
                 u[0] = src.val;
-                u[1] = dst.val;
+                u[1] = _regs.eax;
                 if ( mul_dbl(u) )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
                 _regs.edx = u[1];
@@ -3882,12 +3883,13 @@ x86_emulate(
             }
             break;
         case 5: /* imul */
+            dst.reg = (unsigned long *)&_regs.eax;
         imul:
             _regs.eflags &= ~(EFLG_OF|EFLG_CF);
             switch ( dst.bytes )
             {
             case 1:
-                dst.val = (int8_t)src.val * (int8_t)dst.val;
+                dst.val = (int8_t)src.val * (int8_t)_regs.eax;
                 if ( (int8_t)dst.val != (int16_t)dst.val )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
                 ASSERT(b > 0x6b);
@@ -3895,7 +3897,7 @@ x86_emulate(
                 break;
             case 2:
                 dst.val = ((uint32_t)(int16_t)src.val *
-                           (uint32_t)(int16_t)dst.val);
+                           (uint32_t)(int16_t)_regs.eax);
                 if ( (int16_t)dst.val != (int32_t)dst.val )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
                 if ( b > 0x6b )
@@ -3904,7 +3906,7 @@ x86_emulate(
 #ifdef __x86_64__
             case 4:
                 dst.val = ((uint64_t)(int32_t)src.val *
-                           (uint64_t)(int32_t)dst.val);
+                           (uint64_t)(int32_t)_regs.eax);
                 if ( (int32_t)dst.val != dst.val )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
                 if ( b > 0x6b )
@@ -3913,7 +3915,7 @@ x86_emulate(
 #endif
             default:
                 u[0] = src.val;
-                u[1] = dst.val;
+                u[1] = _regs.eax;
                 if ( imul_dbl(u) )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
                 if ( b > 0x6b )
@@ -3923,6 +3925,7 @@ x86_emulate(
             }
             break;
         case 6: /* div */
+            dst.reg = (unsigned long *)&_regs.eax;
             switch ( src.bytes )
             {
             case 1:
@@ -3968,6 +3971,7 @@ x86_emulate(
             }
             break;
         case 7: /* idiv */
+            dst.reg = (unsigned long *)&_regs.eax;
             switch ( src.bytes )
             {
             case 1:

Comments

Andrew Cooper Sept. 29, 2016, 6:12 p.m. UTC | #1 
On 29/09/16 14:08, Jan Beulich wrote:
> Commit a3db233ede ("x86emul: use DstEax also for {,I}{MUL,DIV}") went
> a little too far: DstEax and SrcEax weren't really meant to be used
> together with ModRM - they assume modrm_reg remains zero by the time
> the destination / source register pointer gets calculated. Don't fully
> undo that commit though, but instead just correct the register pointer,
> and don't use dst.val as input for mul and imul (div and idiv did avoid
> that already).
>
> Reported-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Konrad Rzeszutek Wilk Sept. 29, 2016, 6:21 p.m. UTC | #2 
On Thu, Sep 29, 2016 at 07:08:10AM -0600, Jan Beulich wrote:
> Commit a3db233ede ("x86emul: use DstEax also for {,I}{MUL,DIV}") went
> a little too far: DstEax and SrcEax weren't really meant to be used
> together with ModRM - they assume modrm_reg remains zero by the time
> the destination / source register pointer gets calculated. Don't fully
> undo that commit though, but instead just correct the register pointer,
> and don't use dst.val as input for mul and imul (div and idiv did avoid
> that already).
> 
> Reported-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

and
Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
diff mbox

Patch

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -3845,18 +3845,19 @@  x86_emulate(
             emulate_1op("neg", dst, _regs.eflags);
             break;
         case 4: /* mul */
+            dst.reg = (unsigned long *)&_regs.eax;
             _regs.eflags &= ~(EFLG_OF|EFLG_CF);
             switch ( dst.bytes )
             {
             case 1:
-                dst.val = (uint8_t)dst.val;
+                dst.val = (uint8_t)_regs.eax;
                 dst.val *= src.val;
                 if ( (uint8_t)dst.val != (uint16_t)dst.val )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
                 dst.bytes = 2;
                 break;
             case 2:
-                dst.val = (uint16_t)dst.val;
+                dst.val = (uint16_t)_regs.eax;
                 dst.val *= src.val;
                 if ( (uint16_t)dst.val != (uint32_t)dst.val )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
@@ -3864,7 +3865,7 @@  x86_emulate(
                 break;
 #ifdef __x86_64__
             case 4:
-                dst.val = (uint32_t)dst.val;
+                dst.val = _regs._eax;
                 dst.val *= src.val;
                 if ( (uint32_t)dst.val != dst.val )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
@@ -3873,7 +3874,7 @@  x86_emulate(
 #endif
             default:
                 u[0] = src.val;
-                u[1] = dst.val;
+                u[1] = _regs.eax;
                 if ( mul_dbl(u) )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
                 _regs.edx = u[1];
@@ -3882,12 +3883,13 @@  x86_emulate(
             }
             break;
         case 5: /* imul */
+            dst.reg = (unsigned long *)&_regs.eax;
         imul:
             _regs.eflags &= ~(EFLG_OF|EFLG_CF);
             switch ( dst.bytes )
             {
             case 1:
-                dst.val = (int8_t)src.val * (int8_t)dst.val;
+                dst.val = (int8_t)src.val * (int8_t)_regs.eax;
                 if ( (int8_t)dst.val != (int16_t)dst.val )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
                 ASSERT(b > 0x6b);
@@ -3895,7 +3897,7 @@  x86_emulate(
                 break;
             case 2:
                 dst.val = ((uint32_t)(int16_t)src.val *
-                           (uint32_t)(int16_t)dst.val);
+                           (uint32_t)(int16_t)_regs.eax);
                 if ( (int16_t)dst.val != (int32_t)dst.val )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
                 if ( b > 0x6b )
@@ -3904,7 +3906,7 @@  x86_emulate(
 #ifdef __x86_64__
             case 4:
                 dst.val = ((uint64_t)(int32_t)src.val *
-                           (uint64_t)(int32_t)dst.val);
+                           (uint64_t)(int32_t)_regs.eax);
                 if ( (int32_t)dst.val != dst.val )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
                 if ( b > 0x6b )
@@ -3913,7 +3915,7 @@  x86_emulate(
 #endif
             default:
                 u[0] = src.val;
-                u[1] = dst.val;
+                u[1] = _regs.eax;
                 if ( imul_dbl(u) )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
                 if ( b > 0x6b )
@@ -3923,6 +3925,7 @@  x86_emulate(
             }
             break;
         case 6: /* div */
+            dst.reg = (unsigned long *)&_regs.eax;
             switch ( src.bytes )
             {
             case 1:
@@ -3968,6 +3971,7 @@  x86_emulate(
             }
             break;
         case 7: /* idiv */
+            dst.reg = (unsigned long *)&_regs.eax;
             switch ( src.bytes )
             {
             case 1: