KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

From 9743b5299bae1779c2b893cbeb86122bcccb9b2d Mon Sep 17 00:00:00 2001
From: Sheng Yang <[email protected]>
Date: Wed, 2 Jan 2008 14:49:22 +0800
Subject: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

When executing a test program called "crashme", we found the KVM guest cannot survived more than ten seconds, then encounterd kernel panic. The basic concept of "crashme" is graduating random assembly code and trying to execute them in a fork process.

After some fix on emulator valid judgment, we found it's hard to get the current emulator handle the invalid instructions correctly, for the #UD trap for hypercall patching caused troubles. The problem is, if the opcode itself was OK, but combination of opcode and modrm_reg was invalid, and one operand of the opcode was memory(SrcMem or DstMem), emulator would fetched the memory operand first rather than judged the validity, and may encounter error there.

For example, ".byte 0xfe, 0x34, 0xcd" got this trouble.

In the patch, we simply check that if the invalid opcode isn't vmcall/vmmcall, then return from emulate_instruction() and inject a #UD to guest. With the patch, the guest had been run for more than 12 hours.

Signed-off-by Feng (Eric) Liu <[email protected]>

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/kvm/svm.c         |    2 +-
 arch/x86/kvm/vmx.c         |    2 +-
 arch/x86/kvm/x86.c         |   18 +++++++++++++++---
 include/asm-x86/kvm_host.h |    4 +++-
 4 files changed, 20 insertions(+), 6 deletions(-)

arch/x86/kvm/svm.c +1 -1

index 11e5baf..2da9e11 100644
@@ -942,7 +942,7 @@ static int ud_interception(struct vcpu_svm *svm, struct 
 kvm_run *kvm_run)
 {
 	int er;
 
-	er = emulate_instruction(&svm->vcpu, kvm_run, 0, 0, 0);
+	er = emulate_instruction(&svm->vcpu, kvm_run, 0, 0, EMULTYPE_TRAP_UD);
 	if (er != EMULATE_DONE)
 		kvm_queue_exception(&svm->vcpu, UD_VECTOR);

arch/x86/kvm/svm.c +0 -0

 	return 1;

arch/x86/kvm/vmx.c +1 -1

index 4741806..5346e42 100644
@@ -1862,7 +1862,7 @@ static int handle_exception(struct kvm_vcpu *vcpu, 
 struct kvm_run *kvm_run)
 	}
 
 	if (is_invalid_opcode(intr_info)) {
-		er = emulate_instruction(vcpu, kvm_run, 0, 0, 0);
+		er = emulate_instruction(vcpu, kvm_run, 0, 0, EMULTYPE_TRAP_UD);
 		if (er != EMULATE_DONE)
 			kvm_queue_exception(vcpu, UD_VECTOR);

arch/x86/kvm/vmx.c +0 -0

 		return 1;

arch/x86/kvm/x86.c +15 -3

index c5b4825..2a659ce 100644
@@ -1840,9 +1840,10 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 			struct kvm_run *run,
 			unsigned long cr2,
 			u16 error_code,
-			int no_decode)
+			int emulation_type)
 {
 	int r;
+	struct decode_cache *c;
 
 	vcpu->arch.mmio_fault_cr2 = cr2;
 	kvm_x86_ops->cache_regs(vcpu);

@@ -1850,7 +1851,7 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 	vcpu->mmio_is_write = 0;
 	vcpu->arch.pio.string = 0;
 
-	if (!no_decode) {
+	if (!(emulation_type & EMULTYPE_NO_DECODE)) {
 		int cs_db, cs_l;
 		kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
 

@@ -1884,6 +1885,16 @@ int emulate_instruction(struct kvm_vcpu *vcpu,
 					get_segment_base(vcpu, VCPU_SREG_FS);
 
 		r = x86_decode_insn(&vcpu->arch.emulate_ctxt, &emulate_ops);
+
+		/* Reject the instructions other than VMCALL/VMMCALL when
+		 * try to emulate invalid opcode */
+		c = &vcpu->arch.emulate_ctxt.decode;
+		if ((emulation_type & EMULTYPE_TRAP_UD) &&
+		    (!(c->twobyte && c->b == 0x01 &&
+		      (c->modrm_reg == 0 || c->modrm_reg == 3) &&
+		       c->modrm_mod == 3 && c->modrm_rm == 1)))
+			return EMULATE_FAIL;
+
 		++vcpu->stat.insn_emulation;
 		if (r)  {
 			++vcpu->stat.insn_emulation_fail;

@@ -2639,7 +2650,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, 
 struct kvm_run *kvm_run)
 		vcpu->mmio_read_completed = 1;
 		vcpu->mmio_needed = 0;
 		r = emulate_instruction(vcpu, kvm_run,
-					vcpu->arch.mmio_fault_cr2, 0, 1);
+					vcpu->arch.mmio_fault_cr2, 0,
+					EMULTYPE_NO_DECODE);
 		if (r == EMULATE_DO_MMIO) {
 			/*

arch/x86/kvm/x86.c +0 -0

 			 * Read-modify-write.  Back to userspace.

include/asm-x86/kvm_host.h +3 -1

index 20597bc..4702b04 100644
@@ -415,8 +415,10 @@ enum emulation_result {
 	EMULATE_FAIL,         /* can't emulate this instruction */
 };
 
+#define EMULTYPE_NO_DECODE	    (1 << 0)
+#define EMULTYPE_TRAP_UD	    (1 << 1)
 int emulate_instruction(struct kvm_vcpu *vcpu, struct kvm_run *run,
-			unsigned long cr2, u16 error_code, int no_decode);
+			unsigned long cr2, u16 error_code, int emulation_type);
 void kvm_report_emulation_failure(struct kvm_vcpu *cvpu, const char 
 *context);
 void realmode_lgdt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);

void realmode_lidt(struct kvm_vcpu *vcpu, u16 size, unsigned long address);

------------------------------------------------------------------------

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
------------------------------------------------------------------------

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Quoted 13 lines · Sheng

From 9743b5299bae1779c2b893cbeb86122bcccb9b2d Mon Sep 17 00:00:00 2001
From: Sheng Yang <[email protected]>
Date: Wed, 2 Jan 2008 14:49:22 +0800
Subject: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

When executing a test program called "crashme", we found the KVM guest
cannot survived more than ten seconds, then encounterd kernel panic. The basic concept of "crashme" is graduating random assembly code and trying to execute them in a fork process.

Quoted 8 lines

After some fix on emulator valid judgment, we found it's hard to get the current emulator handle the invalid instructions correctly, for the #UD trap for hypercall patching caused troubles. The problem is, if the opcode itself was OK, but combination of opcode and modrm_reg was invalid, and one operand of the opcode was memory(SrcMem or DstMem), emulator would fetched the memory operand first rather than judged the validity, and may encounter error there.

Nice catch!

Regards,

Anthony Liguori

Quoted 22 lines · Anthony Liguori

Quoted 11 lines · Sheng

From 9743b5299bae1779c2b893cbeb86122bcccb9b2d Mon Sep 17 00:00:00 2001 From: Sheng Yang <[email protected]>
Date: Wed, 2 Jan 2008 14:49:22 +0800
Subject: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

When executing a test program called "crashme", we found the KVM guest cannot survived more than ten seconds, then encounterd kernel panic. The basic concept of "crashme" is graduating random assembly code and trying to execute them in a fork process.

Quoted 9 lines

After some fix on emulator valid judgment, we found it's hard to get the current emulator handle the invalid instructions correctly, for the #UD trap for hypercall patching caused troubles. The problem is, if the opcode itself was OK, but combination of opcode and modrm_reg was invalid, and one operand of the opcode was memory(SrcMem or DstMem), emulator would fetched the memory operand first rather than judged the validity, and may encounter error there.

Quoted 7 lines

Nice catch!

Regards,

Anthony Liguori

Anthony:
Actually I am wondering if the binary used for VMMCALL could be
assumed will never be used by Intel processor or vice versa. BTW, what is the nenefit to remove hypercall page, which provide more clean approach IMO?
thx,eddie

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Anthony:
Actually I am wondering if the binary used for VMMCALL could be
assumed will never be used by Intel processor or vice versa. BTW, what is the nenefit to remove hypercall page, which provide more clean approach IMO?

thx,eddie

Quoted 6 lines · Eddie

Anthony:
Actually I am wondering if the binary used for VMMCALL could be
assumed will never be used by Intel processor or vice versa. BTW, what is the nenefit to remove hypercall page, which provide more clean approach IMO?

A hypercall page doesn't help the situation with respect to migration between an AMD and Intel system.

Quoted 9 lines · Anthony Liguori

Quoted 7 lines · Eddie

Anthony:
Actually I am wondering if the binary used for VMMCALL could be
assumed will never be used by Intel processor or vice versa. BTW, what is the nenefit to remove hypercall page, which provide more clean approach IMO?

Quoted 3 lines

A hypercall page doesn't help the situation with respect to migration between an AMD and Intel system.

I guess I miss something. As if hypercall page is read only after creation
(by VMM), later memory migration won't overlap it. The page could be a "Reserved" in e820 table.

Quoted 15 lines · Eddie

Quoted 12 lines · Anthony Liguori

Quoted 7 lines · Eddie

Anthony:
Actually I am wondering if the binary used for VMMCALL could be
assumed will never be used by Intel processor or vice versa. BTW, what is the nenefit to remove hypercall page, which provide more clean approach IMO?

A hypercall page doesn't help the situation with respect to migration between an AMD and Intel system.

Quoted 4 lines

I guess I miss something. As if hypercall page is read only after creation (by VMM), later memory migration won't overlap it. The page could be a "Reserved" in e820 table.

If migration happens while rip is in the hypercall page, and if the

hypercall page is different in the target host, you may need to apply fixups. The fixups (and the hypercall page itself) may be different according to the guest mode (32-bit or 64-bit).

Quoted 25 lines · Avi Kivity

Quoted 23 lines · Eddie

Quoted 11 lines · Anthony Liguori

Quoted 8 lines · Eddie

Anthony:
Actually I am wondering if the binary used for VMMCALL could be
assumed will never be used by Intel processor or vice versa. BTW, what is the nenefit to remove hypercall page, which provide more clean approach IMO?

Quoted 4 lines

A hypercall page doesn't help the situation with respect to migration between an AMD and Intel system.

I guess I miss something. As if hypercall page is read only after creation (by VMM), later memory migration won't overlap it. The page could be a "Reserved" in e820 table.

Quoted 2 lines

If migration happens while rip is in the hypercall page, and if the

I didn't quit catch here. The source VM vCPU is in Qemu migration part, The target VM VCPU is always waiting for migration data/command. If you mean SMP case, all target VCPUs are in waiting for data/cmd, and I assume source VCPUs are all in Qemu known state, not?

Quoted 12 lines

Quoted 3 lines

If migration happens while rip is in the hypercall page, and if the

I didn't quit catch here. The source VM vCPU is in Qemu migration part, The target VM VCPU is always waiting for migration data/command. If you mean SMP case, all target VCPUs are in waiting for data/cmd, and I assume source VCPUs are all in Qemu known state, not?

I'm talking about the guest rip. The guest is not aware of the migration.

Suppose that on the last copy that the guest rip is (hypercall_page_virt + 3). This address might be in the middle of some instruction on the
hypercall page on the target machine. You need to fix up rip and

possibly modify registers so that when it resumes it works correctly.

Quoted 12 lines

Quoted 12 lines

Quoted 3 lines

If migration happens while rip is in the hypercall page, and if the

I didn't quit catch here. The source VM vCPU is in Qemu migration part, The target VM VCPU is always waiting for migration data/command. If you mean SMP case, all target VCPUs are in waiting for data/cmd, and I assume source VCPUs are all in Qemu known state, not?

Quoted 8 lines

I'm talking about the guest rip. The guest is not aware of the migration.

Suppose that on the last copy that the guest rip is (hypercall_page_virt + 3). This address might be in the middle of some instruction on the
hypercall page on the target machine. You need to fix up rip and

This depends on how the hypercall page instruction is generated. In Xen's construction, the code in hypercall page is exactly same between SVM & VMX except the VMCALL/VMMCALL instruction itself.

Quoted 22 lines · Eddie

Quoted 21 lines

Quoted 4 lines

Quoted 4 lines

If migration happens while rip is in the hypercall page, and if the

Quoted 9 lines

I didn't quit catch here. The source VM vCPU is in Qemu migration part, The target VM VCPU is always waiting for migration data/command. If you mean SMP case, all target VCPUs are in waiting for data/cmd, and I assume source VCPUs are all in Qemu known state, not?

I'm talking about the guest rip. The guest is not aware of the migration.

Suppose that on the last copy that the guest rip is (hypercall_page_virt + 3). This address might be in the middle of some instruction on the hypercall page on the target machine. You need to fix up rip and

Quoted 5 lines

This depends on how the hypercall page instruction is generated. In Xen's construction, the code in hypercall page is exactly same between SVM & VMX except the VMCALL/VMMCALL instruction itself.

If you make the assumption that the hypercall is a single 3-byte

instruction, then you might as well patch it directly. Of course it depends on Intel and AMD not reusing each other's opcodes.

From 9743b5299bae1779c2b893cbeb86122bcccb9b2d Mon Sep 17 00:00:00 2001
From: Sheng Yang <[email protected]>
Date: Wed, 2 Jan 2008 14:49:22 +0800
Subject: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

When executing a test program called "crashme", we found the KVM guest cannot survived more than ten seconds, then encounterd kernel panic. The basic concept of "crashme" is graduating random assembly code and trying to execute them in a fork process.

After some fix on emulator valid judgment, we found it's hard to get the current emulator handle the invalid instructions correctly, for the #UD trap for hypercall patching caused troubles. The problem is, if the opcode itself was OK, but combination of opcode and modrm_reg was invalid, and one operand of the opcode was memory(SrcMem or DstMem), emulator would fetched the memory operand first rather than judged the validity, and may encounter error there. For example, ".byte 0xfe, 0x34, 0xcd" got this trouble.

In the patch, we simply check that if the invalid opcode isn't vmcall/vmmcall, then return from emulate_instruction() and inject a #UD to guest. With the patch, the guest had been run for more than 12 hours.

Quoted 19 lines · Sheng

From 9743b5299bae1779c2b893cbeb86122bcccb9b2d Mon Sep 17 00:00:00 2001
From: Sheng Yang <[email protected]>
Date: Wed, 2 Jan 2008 14:49:22 +0800
Subject: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

When executing a test program called "crashme", we found the KVM guest cannot survived more than ten seconds, then encounterd kernel panic. The basic concept of "crashme" is graduating random assembly code and trying to execute them in a fork process.

After some fix on emulator valid judgment, we found it's hard to get the current emulator handle the invalid instructions correctly, for the #UD trap for hypercall patching caused troubles. The problem is, if the opcode itself was OK, but combination of opcode and modrm_reg was invalid, and one operand of the opcode was memory(SrcMem or DstMem), emulator would fetched the memory operand first rather than judged the validity, and may encounter error there. For example, ".byte 0xfe, 0x34, 0xcd" got this trouble.

Quoted 3 lines

In the patch, we simply check that if the invalid opcode isn't vmcall/vmmcall, then return from emulate_instruction() and inject a #UD to guest. With the patch, the guest had been run for more than 12 hours.

Applied, thanks. As Anthony says, good catch indeed.

I have a vague plan for improving decode; basically extend the decode tables to add group decoding. We add a bit to opcode_table and twobyte_table that is set for all instructions which need group decoding. When the bit is set, the rest of the value in opcode_table is interpreted as an index (together with modrm_reg) into a new group_table, so we can have different decoding for such instructions.

Quoted 7 lines

I have a vague plan for improving decode; basically extend the decode tables to add group decoding. We add a bit to opcode_table and twobyte_table that is set for all instructions which need group decoding. When the bit is set, the rest of the value in opcode_table is interpreted as an index (together with modrm_reg) into a new group_table, so we can have different decoding for such instructions.

I also have tried to propose a table for Grp opcode, but can't find a easy way. Using the rest of the value in opcode_table is a good idea, but I'm afraid the same value for different group exists, e.g. 0x82(Grp1) and 0xc0 (Grp2) got the same value as: ByteOp | DstMem | SrcImm | ModRM. If we add more factors to this, it would become unclear and more tricky, the table also may become larger...

Currently, if we want to using group_table, I think the straightforward way is
better: another big "switch"... The only exception is 1a, and we may use 0 instead of it.

Quoted 14 lines · Sheng

Quoted 6 lines

I have a vague plan for improving decode; basically extend the decode tables to add group decoding. We add a bit to opcode_table and twobyte_table that is set for all instructions which need group decoding. When the bit is set, the rest of the value in opcode_table is interpreted as an index (together with modrm_reg) into a new group_table, so we can have different decoding for such instructions.

I also have tried to propose a table for Grp opcode, but can't find a easy way. Using the rest of the value in opcode_table is a good idea, but I'm afraid the same value for different group exists, e.g. 0x82(Grp1) and 0xc0 (Grp2) got the same value as: ByteOp | DstMem | SrcImm | ModRM. If we add more factors to this, it would become unclear and more tricky, the table also may become larger...

Quoted 3 lines

Currently, if we want to using group_table, I think the straightforward way is better: another big "switch"... The only exception is 1a, and we may use 0 instead of it.

Not sure what you mean. I thought of adding code like

    if (c->d & Group) {
          c->group = c->d & GroupMask;

          // fetch modrm_reg
          c->d = group_table[c->group * 8 + modrm_reg];

    }

Instruction execution could continue to be in the regular switch, but the decode flags can be different for every instruction in the group.

Quoted 23 lines · On Monday 07 January 2008 17:22:43 Avi Kivity wrote:

Quoted 14 lines · Sheng

Quoted 7 lines

I have a vague plan for improving decode; basically extend the decode tables to add group decoding. We add a bit to opcode_table and twobyte_table that is set for all instructions which need group decoding. When the bit is set, the rest of the value in opcode_table is interpreted as an index (together with modrm_reg) into a new group_table, so we can have different decoding for such instructions.

I also have tried to propose a table for Grp opcode, but can't find a easy way. Using the rest of the value in opcode_table is a good idea, but I'm afraid the same value for different group exists, e.g. 0x82(Grp1) and 0xc0 (Grp2) got the same value as: ByteOp | DstMem | SrcImm | ModRM. If we add more factors to this, it would become unclear and more tricky, the table also may become larger...

Quoted 4 lines

Currently, if we want to using group_table, I think the straightforward way is better: another big "switch"... The only exception is 1a, and we may use 0 instead of it.

Not sure what you mean. I thought of adding code like

Quoted 3 lines

    if (c->d & Group) {
          c->group = c->d & GroupMask;

I meant the c->d & GroupMask is not sufficient to indicate different group. For example, 0x82(Grp1) and 0xc0(Grp2) have same c->d & GroupMask = ByteOp | DstMem | SrcImm | ModRM.

Quoted 3 lines

          // fetch modrm_reg
          c->d = group_table[c->group * 8 + modrm_reg];

In this case, how can you deal with c->group = ByteOp | DstMem | SrcImm | ModRM, and modrm_reg = 6. Is it a XOR or nothing?

Quoted 23 lines · Sheng

Quoted 22 lines · On Monday 07 January 2008 17:22:43 Avi Kivity wrote:

Quoted 14 lines · Sheng

Quoted 6 lines

I have a vague plan for improving decode; basically extend the decode tables to add group decoding. We add a bit to opcode_table and twobyte_table that is set for all instructions which need group decoding. When the bit is set, the rest of the value in opcode_table is interpreted as an index (together with modrm_reg) into a new group_table, so we can have different decoding for such instructions.

I also have tried to propose a table for Grp opcode, but can't find a easy way. Using the rest of the value in opcode_table is a good idea, but I'm afraid the same value for different group exists, e.g. 0x82(Grp1) and 0xc0 (Grp2) got the same value as: ByteOp | DstMem | SrcImm | ModRM. If we add more factors to this, it would become unclear and more tricky, the table also may become larger...

Quoted 3 lines

Currently, if we want to using group_table, I think the straightforward way is better: another big "switch"... The only exception is 1a, and we may use 0 instead of it.

Not sure what you mean. I thought of adding code like

Quoted 6 lines

Quoted 2 lines

    if (c->d & Group) {
          c->group = c->d & GroupMask;

I meant the c->d & GroupMask is not sufficient to indicate different group. For example, 0x82(Grp1) and 0xc0(Grp2) have same c->d & GroupMask = ByteOp | DstMem | SrcImm | ModRM.

I now see the source of confusion...

Quoted 5 lines

Quoted 2 lines

          // fetch modrm_reg
          c->d = group_table[c->group * 8 + modrm_reg];

In this case, how can you deal with c->group = ByteOp | DstMem | SrcImm | ModRM, and modrm_reg = 6. Is it a XOR or nothing?

In this case, we would have

   opcode_table[0x82] == Group | Grp1  (c->group == Grp1 == 1)
   opcode_table[0xc0] == Group | Grp2  (c->group == Grp2 == 2)
   group_table[Grp1 * 8 + 6] == ByteOp | DstMem | SrcImm | ModRM
   group_table[Grp2 * 8 + 0] == ByteOp | DstMem | SrcImm | ModRM

If Group is set in opcode_table, the rest of the value is the group number, with the reg part used to pick the final decode flags.