diff mbox

x86/emul: Avoid #UD in SIMD stubs

Message ID 1488975001-20431-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Cooper March 8, 2017, 12:10 p.m. UTC 
v{,u}comis{s,d}, and vcvt{,t}s{s,d}2si are two-operand instructions, while
vzero{all,upper} take no operands, so require vex.reg set to ~0 to avoid #UD.

Spotted while fuzzing with AFL
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Jan Beulich March 8, 2017, 1:02 p.m. UTC | #1 
>>> On 08.03.17 at 13:10, <andrew.cooper3@citrix.com> wrote:
> v{,u}comis{s,d}, and vcvt{,t}s{s,d}2si are two-operand instructions, while
> vzero{all,upper} take no operands, so require vex.reg set to ~0 to avoid 
> #UD.
> 
> Spotted while fuzzing with AFL
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
Andrew Cooper March 8, 2017, 1:24 p.m. UTC | #2 
On 08/03/17 13:02, Jan Beulich wrote:
>>>> On 08.03.17 at 13:10, <andrew.cooper3@citrix.com> wrote:
>> v{,u}comis{s,d}, and vcvt{,t}s{s,d}2si are two-operand instructions, while
>> vzero{all,upper} take no operands, so require vex.reg set to ~0 to avoid 
>> #UD.
>>
>> Spotted while fuzzing with AFL
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
>

Thanks,

I took this opportunity to test the stub recovery from the point of view
of a malicious guest.

(XEN) d2v0 exception 6 (ec=0000) in emulation stub (line 6239)
(XEN) d2v0 stub: c4 e1 44 77 c3 80 d0 82 ff ff ff d1 90 ec 90

It is good to see that such a bug won't even been a security issue in
Xen 4.9!

One observation however.  It would probably be safer to poison the stub
with 0xcc each time (especially if we have a path which omits the ret),
instead of leaving partial instructions in place.

~Andrew
Jan Beulich March 8, 2017, 1:35 p.m. UTC | #3 
>>> On 08.03.17 at 14:24, <andrew.cooper3@citrix.com> wrote:
> One observation however.  It would probably be safer to poison the stub
> with 0xcc each time (especially if we have a path which omits the ret),
> instead of leaving partial instructions in place.

I too have been considering this.

Jan
diff mbox

Patch

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index e09975c..8134e76 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -5620,7 +5620,7 @@  x86_emulate(
         {
             if ( ctxt->vendor == X86_VENDOR_AMD )
                 vex.l = 0;
-            generate_exception_if(vex.l, EXC_UD);
+            generate_exception_if(vex.l || vex.reg != 0xf, EXC_UD);
             host_and_vcpu_must_have(avx);
             get_fpu(X86EMUL_FPU_ymm, &fic);
         }
@@ -5673,6 +5673,7 @@  x86_emulate(
         }
         else
         {
+            generate_exception_if(vex.reg != 0xf, EXC_UD);
             host_and_vcpu_must_have(avx);
             get_fpu(X86EMUL_FPU_ymm, &fic);
         }
@@ -6273,6 +6274,7 @@  x86_emulate(
     case X86EMUL_OPC_VEX(0x0f, 0x77):    /* vzero{all,upper} */
         if ( vex.opcx != vex_none )
         {
+            generate_exception_if(vex.reg != 0xf, EXC_UD);
             host_and_vcpu_must_have(avx);
             get_fpu(X86EMUL_FPU_ymm, &fic);
         }