[Qemu-devel] kvm: grub breakage due to SS.RPL/DPL alignment

[Qemu-devel] kvm: grub breakage due to SS.RPL/DPL alignment

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1672 bytes --]

Hi,

when interrupting a guest (grub in graphical mode) in this state

EAX=00000011 EBX=0004bc88 ECX=0000000d EDX=000db51d
ESI=000008ff EDI=002462da EBP=00000000 ESP=00001fbc
EIP=000078b6 EFL=00000006 [-----P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0000 00000000 0000ffff 00009300 DPL=0 DS16 [-WA]
CS =2d32 0002d320 0000ffff 00009b00 DPL=0 CS16 [-RA]
SS =45aa 00045aa0 0000ffff 00009300 DPL=0 DS16 [-WA]
DS =2d32 0002d320 0000ffff 00009300 DPL=0 DS16 [-WA]
FS =0000 00000000 0000ffff 00009300 DPL=0 DS16 [-WA]
GS =0000 00000000 0000ffff 00009300 DPL=0 DS16 [-WA]
LDT=0000 00000000 ffffffff 00000000
TR =0048 00266009 00000067 00008b00 DPL=0 TSS32-busy
GDT=     0002dd48 0000004f
IDT=     0026607a 000007ff
CR0=00000011 CR2=00000000 CR3=00000000 CR4=00000000

the SS alignment to CPL corrupts the guest state on write back:

    if (env->cr[0] & CR0_PE_MASK) {
	/* force ss cpl to cs cpl */
	sregs.ss.selector = (sregs.ss.selector & ~3) |
		(sregs.cs.selector & 3);
	sregs.ss.dpl = sregs.ss.selector & 3;
    }

Aligning SS.RPL to CPL is not problematic here (as it already is), but
forcing SS.DPL to CPL is definitely wrong and causes an immediate guest
reboot.

Looking at commit 292a55081e5eee62db42209463cf385e7ff1d86d of qemu-kvm
which introduced this workaround I wonder if it still applies and if it
wasn't misplaced from day one anyway. If we really need this (when
BTW?), doesn't it belong into the kernel, particularly into vmx.c as it
addresses an _Intel_ quirk?

Jan

PS: There is another problem, namely in set_seg's DPL transfer, which
contributes to the guest crash, but that one is easily fixable. Patch
will follow.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] Re: kvm: grub breakage due to SS.RPL/DPL alignment

[Avi Kivity]

On 12/27/2010 04:37 PM, Jan Kiszka wrote:
> Hi,
>
> when interrupting a guest (grub in graphical mode) in this state
>
> EAX=00000011 EBX=0004bc88 ECX=0000000d EDX=000db51d
> ESI=000008ff EDI=002462da EBP=00000000 ESP=00001fbc
> EIP=000078b6 EFL=00000006 [-----P-] CPL=0 II=0 A20=1 SMM=0 HLT=0
> ES =0000 00000000 0000ffff 00009300 DPL=0 DS16 [-WA]
> CS =2d32 0002d320 0000ffff 00009b00 DPL=0 CS16 [-RA]
> SS =45aa 00045aa0 0000ffff 00009300 DPL=0 DS16 [-WA]
> DS =2d32 0002d320 0000ffff 00009300 DPL=0 DS16 [-WA]
> FS =0000 00000000 0000ffff 00009300 DPL=0 DS16 [-WA]
> GS =0000 00000000 0000ffff 00009300 DPL=0 DS16 [-WA]
> LDT=0000 00000000 ffffffff 00000000
> TR =0048 00266009 00000067 00008b00 DPL=0 TSS32-busy
> GDT=     0002dd48 0000004f
> IDT=     0026607a 000007ff
> CR0=00000011 CR2=00000000 CR3=00000000 CR4=00000000
>
> the SS alignment to CPL corrupts the guest state on write back:
>
>      if (env->cr[0]&  CR0_PE_MASK) {
> 	/* force ss cpl to cs cpl */
> 	sregs.ss.selector = (sregs.ss.selector&  ~3) |
> 		(sregs.cs.selector&  3);
> 	sregs.ss.dpl = sregs.ss.selector&  3;
>      }
>
> Aligning SS.RPL to CPL is not problematic here (as it already is), but
> forcing SS.DPL to CPL is definitely wrong and causes an immediate guest
> reboot.
>
> Looking at commit 292a55081e5eee62db42209463cf385e7ff1d86d of qemu-kvm
> which introduced this workaround I wonder if it still applies and if it
> wasn't misplaced from day one anyway.

Hey, that commit was done before kvm supported real mode or mmio 
emulation; those were done by qemu.

It definitely doesn't fit today's kvm (where everything to do with a cpu 
is emulated in the kernel).

> If we really need this (when
> BTW?), doesn't it belong into the kernel, particularly into vmx.c as it
> addresses an _Intel_ quirk?
>

I don't think it's needed, see enter_pmode():

     vmcs_write16(GUEST_SS_SELECTOR, 0);
     vmcs_write32(GUEST_SS_AR_BYTES, 0x93);

     vmcs_write16(GUEST_CS_SELECTOR,
              vmcs_read16(GUEST_CS_SELECTOR) & ~SELECTOR_RPL_MASK);

so ss.rpl and cpl are set to compatible values during the transition.  
We don't even exit to qemu so the code isn't exercised.  I think it can 
be safely removed with no backward compatibility issues to any supported 
kernel.

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