Re: [PATCH] KVM: emulator: Only allow VMCALL/VMMCALL trapped by #UD

["Yang, Sheng" <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Sunday 06 January 2008 16:40:16 Avi Kivity wrote:
> Yang, Sheng wrote:
> > From 9743b5299bae1779c2b893cbeb86122bcccb9b2d Mon Sep 17 00:00:00 2001
> > From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > 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.
>
> 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.

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. 

-- 
Thanks
Yang, Sheng

-------------------------------------------------------------------------
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/