Re: [PATCH v2 3/3] KVM: x86 emulator: Allow PM/VM86 switch during task switch

[Avi Kivity <avi@redhat.com>]

On 01/30/2012 01:05 PM, Kevin Wolf wrote:
> Am 30.01.2012 11:24, schrieb Avi Kivity:
> > On 01/27/2012 09:23 PM, Kevin Wolf wrote:
> >> Task switches can switch between Protected Mode and VM86. The current
> >> mode must be updated during the task switch emulation so that the new
> >> segment selectors are interpreted correctly and privilege checks
> >> succeed.
> >>
> >> VMX code calculates the CPL from the code segment selector and rflags,
> >> so it needs rflags to be updated in the vcpu struct. SVM stores the DPL
> >> of the code segment instead, so we must be sure to give the right one
> >> when updating the selector.
> > 
> > svm has vmcb_save_area::cpl; it's independent of CS.RPL.
>
> Right, but cpl in the VMCB is updated when you reload a segment (and I
> think only then), 

Gah, it's broken.  It should be qualified by more - real mode should
keep cpl at zero, vm86 at 3.  And setting rflags.vm86 should update cpl
as well.

For example, live migration while in real mode with cs&3!=0 or in vm86
mode with cs&3==0 would set the wrong cpl.

> so it's closely related.
>   
> >>  	ctxt->ops->get_segment(ctxt, &dummy, &desc, &base3, seg);
> >> +
> >> +	if (ctxt->mode == X86EMUL_MODE_REAL)
> >> +		desc.dpl = 0;
> > 
> > Can't happen.
>
> set_segment_selector is only called during task switches right now, so
> yes, this is impossible. Want me to drop the check? Or BUG()?

BUG()s are dangerous in rarely used code since they can be exploited as
a DoS.  So maybe a WARN_ON_ONCE().

> >> @@ -2273,6 +2281,24 @@ static int load_state_from_tss32(struct x86_emulate_ctxt *ctxt,
> >>  		return emulate_gp(ctxt, 0);
> >>  	ctxt->_eip = tss->eip;
> >>  	ctxt->eflags = tss->eflags | 2;
> >> +
> >> +	/*
> >> +	 * If we're switching between Protected Mode and VM86, we need to make
> >> +	 * sure to update the mode before loading the segment descriptors so
> >> +	 * that the selectors are interpreted correctly.
> >> +	 *
> >> +	 * Need to get it to the vcpu struct immediately because it influences
> >> +	 * the CPL which is checked at least when loading the segment
> >> +	 * descriptors and when pushing an error code to the new kernel stack.
> >> +	 */
> >> +	if (ctxt->eflags & X86_EFLAGS_VM)
> >> +		ctxt->mode = X86EMUL_MODE_VM86;
> >> +	else
> >> +		ctxt->mode = X86EMUL_MODE_PROT32;
> >> +
> > 
> > Shouldn't this be done after the set_segment_selector() block?  My
> > interpretation of the SDM is that if a fault happens while loading
> > descriptors the fault happens with old segment cache, that is, it needs
> > to be interpreted according to the old mode.
>
> This is closely related to my argument with Gleb whether CPL changes
> when rflags is reloaded or if it only happens when cs is reloaded. I
> argued that it makes more sense to couple it with cs, so I guess I have
> to agree with you mostly.
>
> The SDM says that any faults during loading the segment descriptors
> happen in the context of the new task, and the task switch is completed
> before an exception is generated. So it shouldn't make any difference to
> the guest what the exact point is at which we change CPL, it's an KVM
> internal decision.
>
> So what about this order:
>
> 1. Reload general purpose registers and eflags without updating mode
>    or writing back rflags to the vcpu struct.
>
> 2. Load segment selectors without changing the DPL yet.

(or changing anything in the segment cache)

>
> 3. Load segment descriptors, and disable the privilege checks in
>    load_segment_descriptor using a new flag. 

I don't like doing things that aren't directly traceable to the SDM. 
Perhaps we should pass the cpl as a parameter to
load_segment_descriptor().  Or we should ->set_cpl() just before.

> For SVM, this updates
>    the CPL when cs is reloaded.



>
> 4. Call ctxt->ops.set_rflag so that VMX updates the CPL. Should be
>    cleaned up in a follow-up patch, so that VMX uses set_segment
>    to update the CPL, like SVM does.
>
> Would this match your interpretation?

Not exactly.  I claim that the cpl is a separate state from
cs.cpl/ss.rpl/cr0.pe/eflags.vm - it cannot be derived just from those. 
On the transition from real mode to protected mode, you can have (say)
cs=0x13 and cpl=0.

Outside of mode switches, it's good to have set_segment() update the cpl
automatically.  But inside mode switches it can screw up further checks
or aborts.

> >>  static int emulator_get_cpl(struct x86_emulate_ctxt *ctxt)
> >>  {
> >>  	return kvm_x86_ops->get_cpl(emul_to_vcpu(ctxt));
> >> @@ -4199,6 +4204,7 @@ static struct x86_emulate_ops emulate_ops = {
> >>  	.set_idt	     = emulator_set_idt,
> >>  	.get_cr              = emulator_get_cr,
> >>  	.set_cr              = emulator_set_cr,
> >> +	.set_rflags          = emulator_set_rflags,
> >>  	.cpl                 = emulator_get_cpl,
> >>  	.get_dr              = emulator_get_dr,
> >>  	.set_dr              = emulator_set_dr,
> > 
> > It would be good to switch the entire emulator to use ->set_rflags()
> > instead of sometimes letting the caller do it.
>
> If we change the CPL only with a cs reload, set_rflags can be dropped
> completely in the end.

That's attractive, yes.

Suppose load_segment_descriptor(..., VCPU_SREG_LDTR) fails.  What should
the new cpl be?  Does it matter?

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