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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
