Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.

[Gleb Natapov <gleb@redhat.com>]

On Mon, Jun 25, 2012 at 06:03:19PM +0300, Avi Kivity wrote:
> On 06/25/2012 05:55 PM, Gleb Natapov wrote:
> > On Mon, Jun 25, 2012 at 05:32:31PM +0300, Avi Kivity wrote:
> >> On 06/25/2012 05:17 PM, Gleb Natapov wrote:
> >> > On Mon, Jun 25, 2012 at 04:40:35PM +0300, Avi Kivity wrote:
> >> >> On 06/25/2012 04:12 PM, Gleb Natapov wrote:
> >> >> 
> >> >> >> Right.  But I think we can have x86_linearize() that doesn't take a
> >> >> >> context parameter, only ops.
> >> >> >> 
> >> >> > All ops take context parameter though.
> >> >> > 
> >> >> 
> >> >> context is meaningful for:
> >> >> - saving state between executions (decode/execute/execute)
> >> >> - passing state that is not provided via callbacks (regs/mode/flags)
> >> >> - returning results
> >> >> 
> >> >> Only the second is relevant, and we're trying to get rid of that too.
> >> >> 
> >> > Callbacks were passed pointer to vcpu, but they were changed to get ctxt
> >> > to better encapsulate emulator.c from rest of the KVM. Are you suggesting
> >> > this was a mistake and we need to rework callbacks to receive pointer
> >> > to vcpu again? I hope not :)
> >> 
> >> Ouch.  I guess we have to pass the context, but not initialize any of it
> >> except ops.
> > That's hacky and error pron. We need to audit that linearize() and all
> > callbacks/functions it uses do not rely on un-initialized state, which
> > is doable now, but who will remember to check that in the future, while
> > changing seemingly unrelated code, which, by a coincidence, called during
> > linearize()? Instant security vulnerability. For security (if not
> > sanity) sake we should really make sure that ctxt is initialized while
> > we are in emulator.c and make as many checks for it as possible.
> 
> Agree.  Though the security issue is limited; the structure won't be
> uninitialized, it would retain values from the previous call.  So it's
> limited to intra-guest vulnerabilities.
> 
Yes, that's the kind I mean, not host crash. Intra-guest vulnerabilities
should not be taken lightly. From guest POV they are like buggy CPUs
that allows privilege escalation.

> > 
> >>              Later we can extend x86_decode_insn() and the other
> >> functions to follow the same rule.
> >> 
> > What rule? We cannot not initialize a context. You can reduce things
> > that should be initialized to minimum (getting GP registers on demand,
> > etc), but still some initialization is needed since ctxt holds emulation
> > state and it needs to be reset before each emulation.
> 
> An alternative is to use two contexts, the base context only holds ops
> and is the parameter to all the callbacks on the non-state APIs, the
> derived context holds the state:
> 
> struct x86_emulation_ctxt {
>     struct x86_ops *ops;
>     /* state that always needs to be initialized, preferablt none */
> };
> 
> struct x86_insn_ctxt {
>     struct x86_emulation_ctxt em;
>     /* instruction state */
> }
> 
> and so we have a compile-time split between users of the state and
> non-users.
> 
I do not understand how you will divide current ctxt structure between
those two.

Where will you put those for instance: interruptibility, have_exception,
perm_ok, only_vendor_specific_insn and how can they not be initialized
before each instruction emulation?

--
			Gleb.