* [PATCH] KVM: vmx: VMXOFF emulation in vm86 should cause #UD
@ 2014-08-29 8:26 Nadav Amit
2014-08-29 8:36 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Nadav Amit @ 2014-08-29 8:26 UTC (permalink / raw)
To: pbonzini; +Cc: kvm, Nadav Amit
Unlike VMCALL, the instructions VMXOFF, VMLAUNCH and VMRESUME should cause a UD
exception in real-mode or vm86. However, the emulator considers all these
instructions the same for the matter of mode checks, and emulation upon exit
due to #UD exception.
As a result, the hypervisor behaves incorrectly on vm86 mode. VMXOFF, VMLAUNCH
or VMRESUME cause on vm86 exit due to #UD. The hypervisor then emulates these
instruction and inject #GP to the guest instead of #UD.
This patch creates a new group for these instructions and mark only VMCALL as
an instruction which can be emulated.
Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
arch/x86/kvm/emulate.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index e5bf130..a240fac 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3139,12 +3139,8 @@ static int em_clts(struct x86_emulate_ctxt *ctxt)
static int em_vmcall(struct x86_emulate_ctxt *ctxt)
{
- int rc;
-
- if (ctxt->modrm_mod != 3 || ctxt->modrm_rm != 1)
- return X86EMUL_UNHANDLEABLE;
+ int rc = ctxt->ops->fix_hypercall(ctxt);
- rc = ctxt->ops->fix_hypercall(ctxt);
if (rc != X86EMUL_CONTINUE)
return rc;
@@ -3562,6 +3558,12 @@ static int check_perm_out(struct x86_emulate_ctxt *ctxt)
F2bv(((_f) | DstReg | SrcMem | ModRM) & ~Lock, _e), \
F2bv(((_f) & ~Lock) | DstAcc | SrcImm, _e)
+static const struct opcode group7_rm0[] = {
+ N,
+ I(SrcNone | Priv | EmulateOnUD, em_vmcall),
+ N, N, N, N, N, N,
+};
+
static const struct opcode group7_rm1[] = {
DI(SrcNone | Priv, monitor),
DI(SrcNone | Priv, mwait),
@@ -3655,7 +3657,7 @@ static const struct group_dual group7 = { {
II(SrcMem16 | Mov | Priv, em_lmsw, lmsw),
II(SrcMem | ByteOp | Priv | NoAccess, em_invlpg, invlpg),
}, {
- I(SrcNone | Priv | EmulateOnUD, em_vmcall),
+ EXT(0, group7_rm0),
EXT(0, group7_rm1),
N, EXT(0, group7_rm3),
II(SrcNone | DstMem | Mov, em_smsw, smsw), N,
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: vmx: VMXOFF emulation in vm86 should cause #UD
2014-08-29 8:26 [PATCH] KVM: vmx: VMXOFF emulation in vm86 should cause #UD Nadav Amit
@ 2014-08-29 8:36 ` Paolo Bonzini
2014-08-29 8:52 ` Nadav Amit
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2014-08-29 8:36 UTC (permalink / raw)
To: Nadav Amit; +Cc: kvm
Il 29/08/2014 10:26, Nadav Amit ha scritto:
> Unlike VMCALL, the instructions VMXOFF, VMLAUNCH and VMRESUME should cause a UD
> exception in real-mode or vm86. However, the emulator considers all these
> instructions the same for the matter of mode checks, and emulation upon exit
> due to #UD exception.
>
> As a result, the hypervisor behaves incorrectly on vm86 mode. VMXOFF, VMLAUNCH
> or VMRESUME cause on vm86 exit due to #UD. The hypervisor then emulates these
> instruction and inject #GP to the guest instead of #UD.
>
> This patch creates a new group for these instructions and mark only VMCALL as
> an instruction which can be emulated.
>
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
Patch looks good, but where is the check that MOD == 3 in the "case
RMExt"? Am I just not seeing it?
Paolo
> ---
> arch/x86/kvm/emulate.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index e5bf130..a240fac 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -3139,12 +3139,8 @@ static int em_clts(struct x86_emulate_ctxt *ctxt)
>
> static int em_vmcall(struct x86_emulate_ctxt *ctxt)
> {
> - int rc;
> -
> - if (ctxt->modrm_mod != 3 || ctxt->modrm_rm != 1)
> - return X86EMUL_UNHANDLEABLE;
> + int rc = ctxt->ops->fix_hypercall(ctxt);
>
> - rc = ctxt->ops->fix_hypercall(ctxt);
> if (rc != X86EMUL_CONTINUE)
> return rc;
>
> @@ -3562,6 +3558,12 @@ static int check_perm_out(struct x86_emulate_ctxt *ctxt)
> F2bv(((_f) | DstReg | SrcMem | ModRM) & ~Lock, _e), \
> F2bv(((_f) & ~Lock) | DstAcc | SrcImm, _e)
>
> +static const struct opcode group7_rm0[] = {
> + N,
> + I(SrcNone | Priv | EmulateOnUD, em_vmcall),
> + N, N, N, N, N, N,
> +};
> +
> static const struct opcode group7_rm1[] = {
> DI(SrcNone | Priv, monitor),
> DI(SrcNone | Priv, mwait),
> @@ -3655,7 +3657,7 @@ static const struct group_dual group7 = { {
> II(SrcMem16 | Mov | Priv, em_lmsw, lmsw),
> II(SrcMem | ByteOp | Priv | NoAccess, em_invlpg, invlpg),
> }, {
> - I(SrcNone | Priv | EmulateOnUD, em_vmcall),
> + EXT(0, group7_rm0),
> EXT(0, group7_rm1),
> N, EXT(0, group7_rm3),
> II(SrcNone | DstMem | Mov, em_smsw, smsw), N,
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: vmx: VMXOFF emulation in vm86 should cause #UD
2014-08-29 8:36 ` Paolo Bonzini
@ 2014-08-29 8:52 ` Nadav Amit
2014-08-29 8:57 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Nadav Amit @ 2014-08-29 8:52 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Nadav Amit, KVM
On Aug 29, 2014, at 11:36 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 29/08/2014 10:26, Nadav Amit ha scritto:
>> Unlike VMCALL, the instructions VMXOFF, VMLAUNCH and VMRESUME should cause a UD
>> exception in real-mode or vm86. However, the emulator considers all these
>> instructions the same for the matter of mode checks, and emulation upon exit
>> due to #UD exception.
>>
>> As a result, the hypervisor behaves incorrectly on vm86 mode. VMXOFF, VMLAUNCH
>> or VMRESUME cause on vm86 exit due to #UD. The hypervisor then emulates these
>> instruction and inject #GP to the guest instead of #UD.
>>
>> This patch creates a new group for these instructions and mark only VMCALL as
>> an instruction which can be emulated.
>>
>> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
>
> Patch looks good, but where is the check that MOD == 3 in the "case
> RMExt"? Am I just not seeing it?
>
This seems to be part of the “case GroupDual”.
>> ---
>> arch/x86/kvm/emulate.c | 14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index e5bf130..a240fac 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -3139,12 +3139,8 @@ static int em_clts(struct x86_emulate_ctxt *ctxt)
>>
>> static int em_vmcall(struct x86_emulate_ctxt *ctxt)
>> {
>> - int rc;
>> -
>> - if (ctxt->modrm_mod != 3 || ctxt->modrm_rm != 1)
>> - return X86EMUL_UNHANDLEABLE;
>> + int rc = ctxt->ops->fix_hypercall(ctxt);
>>
>> - rc = ctxt->ops->fix_hypercall(ctxt);
>> if (rc != X86EMUL_CONTINUE)
>> return rc;
>>
>> @@ -3562,6 +3558,12 @@ static int check_perm_out(struct x86_emulate_ctxt *ctxt)
>> F2bv(((_f) | DstReg | SrcMem | ModRM) & ~Lock, _e), \
>> F2bv(((_f) & ~Lock) | DstAcc | SrcImm, _e)
>>
>> +static const struct opcode group7_rm0[] = {
>> + N,
>> + I(SrcNone | Priv | EmulateOnUD, em_vmcall),
>> + N, N, N, N, N, N,
>> +};
>> +
>> static const struct opcode group7_rm1[] = {
>> DI(SrcNone | Priv, monitor),
>> DI(SrcNone | Priv, mwait),
>> @@ -3655,7 +3657,7 @@ static const struct group_dual group7 = { {
>> II(SrcMem16 | Mov | Priv, em_lmsw, lmsw),
>> II(SrcMem | ByteOp | Priv | NoAccess, em_invlpg, invlpg),
>> }, {
>> - I(SrcNone | Priv | EmulateOnUD, em_vmcall),
>> + EXT(0, group7_rm0),
>> EXT(0, group7_rm1),
>> N, EXT(0, group7_rm3),
>> II(SrcNone | DstMem | Mov, em_smsw, smsw), N,
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] KVM: vmx: VMXOFF emulation in vm86 should cause #UD
2014-08-29 8:52 ` Nadav Amit
@ 2014-08-29 8:57 ` Paolo Bonzini
2014-08-29 9:12 ` Nadav Amit
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2014-08-29 8:57 UTC (permalink / raw)
To: Nadav Amit; +Cc: Nadav Amit, KVM
Il 29/08/2014 10:52, Nadav Amit ha scritto:
> > Patch looks good, but where is the check that MOD == 3 in the "case
> > RMExt"? Am I just not seeing it?
>
> This seems to be part of the “case GroupDual”.
GroupDual handles it, but the EXT() macro you're using is exactly what
you want:
#define RMExt (4<<15) /* Opcode extension in ModRM r/m if mod == 3 */
I guess what's missing is
------------------ 8< ------------------
Subject: [PATCH] Check ModRM for RMExt
Some group7 extensions are only defined for mod==3. Check this and
reject emulation if mod!=3.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 56657b0bb3bb..d472e4d50e3c 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4360,6 +4360,8 @@ done_prefixes:
opcode = opcode.u.gdual->mod012[goffset];
break;
case RMExt:
+ if ((ctxt->modrm >> 6) == 3)
+ return EMULATION_FAILED;
goffset = ctxt->modrm & 7;
opcode = opcode.u.group[goffset];
break;
What do you think?
Paolo
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: vmx: VMXOFF emulation in vm86 should cause #UD
2014-08-29 8:57 ` Paolo Bonzini
@ 2014-08-29 9:12 ` Nadav Amit
2014-08-29 9:47 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Nadav Amit @ 2014-08-29 9:12 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Nadav Amit, KVM
[-- Attachment #1: Type: text/plain, Size: 1678 bytes --]
On Aug 29, 2014, at 11:57 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 29/08/2014 10:52, Nadav Amit ha scritto:
>>> Patch looks good, but where is the check that MOD == 3 in the "case
>>> RMExt"? Am I just not seeing it?
>>
>> This seems to be part of the “case GroupDual”.
>
> GroupDual handles it, but the EXT() macro you're using is exactly what
> you want:
>
> #define RMExt (4<<15) /* Opcode extension in ModRM r/m if mod == 3 */
>
> I guess what's missing is
>
> ------------------ 8< ------------------
> Subject: [PATCH] Check ModRM for RMExt
>
> Some group7 extensions are only defined for mod==3. Check this and
> reject emulation if mod!=3.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 56657b0bb3bb..d472e4d50e3c 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -4360,6 +4360,8 @@ done_prefixes:
> opcode = opcode.u.gdual->mod012[goffset];
> break;
> case RMExt:
> + if ((ctxt->modrm >> 6) == 3)
> + return EMULATION_FAILED;
> goffset = ctxt->modrm & 7;
> opcode = opcode.u.group[goffset];
> break;
>
> What do you think?
I don’t know. I am looking at Intel SDM table A-6 (Opcode Extensions for One- and Two-byte Opcodes by Group Number).
According to the table, only group 7 needs RMExt, and in this case the “case GroupDual” makes the required checks, in the iteration prior to the “case RMExt”.
Therefore this code path, RMExt without GroupDual before it, should never occur. Nonetheless, if you want to avoid future bugs, perhaps it is good.
Nadav
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: vmx: VMXOFF emulation in vm86 should cause #UD
2014-08-29 9:12 ` Nadav Amit
@ 2014-08-29 9:47 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2014-08-29 9:47 UTC (permalink / raw)
To: Nadav Amit; +Cc: Nadav Amit, KVM
Il 29/08/2014 11:12, Nadav Amit ha scritto:
> I don’t know. I am looking at Intel SDM table A-6 (Opcode Extensions
> for One- and Two-byte Opcodes by Group Number). According to the
> table, only group 7 needs RMExt, and in this case the “case
> GroupDual” makes the required checks, in the iteration prior to the
> “case RMExt”. Therefore this code path, RMExt without GroupDual
> before it, should never occur. Nonetheless, if you want to avoid
> future bugs, perhaps it is good.
Oh, now I understand what you mean. Thanks,
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-29 9:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-29 8:26 [PATCH] KVM: vmx: VMXOFF emulation in vm86 should cause #UD Nadav Amit
2014-08-29 8:36 ` Paolo Bonzini
2014-08-29 8:52 ` Nadav Amit
2014-08-29 8:57 ` Paolo Bonzini
2014-08-29 9:12 ` Nadav Amit
2014-08-29 9:47 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox