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

[Avi Kivity <avi@redhat.com>]

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.

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

-- 
error compiling committee.c: too many arguments to function