[PATCH] Fix up vmx_set_segment for booting older guests.

[PATCH] Fix up vmx_set_segment for booting older guests.

[Chris Lalancette]

If a guest happens to be unlucky enough to use an address
such as 0xc0000000 in the CS base address field, the next attempt
to VM enter will fail.  This is because the vmcs_writel() that
writes the base address into the VMCS will sign-extend the field
to 64-bits, and the Intel manual states that bits 63:32 of this
field *must* be 0.  Use vmcs_write32() where appropriate.
This fixes booting of an absolutely ancient Red Hat Linux 5.2
(not Enterprise Linux!) guest.

Signed-off-by: Chris Lalancette <clalance@redhat.com>
---
 arch/x86/kvm/vmx.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 364263a..311afd4 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1846,7 +1846,22 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu,
 		vmx->rmode.tr.ar = vmx_segment_access_rights(var);
 		return;
 	}
-	vmcs_writel(sf->base, var->base);
+
+	/* Intel 64 and IA-32 Architecture Software Developer's Manual Vol. 3b,
+	 * section 22.3.1.2 states that VMENTRY will fail if bits 63:32 of the
+	 * base address for CS, SS, DS, ES are not 0 and the register is usable.
+	 *
+	 * If var->base happens to have bit 31 set, then it will get sign
+	 * extended on the vmcs_writel(), causing this check to fail.  Make
+	 * sure to use the 32-bit version where appropriate.
+	 */
+	if (sf->base == GUEST_CS_BASE ||
+	    ((~sf->ar_bytes & 0x00010000) && (sf->base == GUEST_SS_BASE ||
+					      sf->base == GUEST_DS_BASE ||
+					      sf->base == GUEST_ES_BASE)))
+		vmcs_write32(sf->base, var->base);
+	else
+		vmcs_writel(sf->base, var->base);
 	vmcs_write32(sf->limit, var->limit);
 	vmcs_write16(sf->selector, var->selector);
 	if (vmx->rmode.vm86_active && var->s) {
-- 
1.6.0.6

Re: [PATCH] Fix up vmx_set_segment for booting older guests.

[Avi Kivity]

On 10/20/2009 04:50 PM, Chris Lalancette wrote:
> If a guest happens to be unlucky enough to use an address
> such as 0xc0000000 in the CS base address field, the next attempt
> to VM enter will fail.  This is because the vmcs_writel() that
> writes the base address into the VMCS will sign-extend the field
> to 64-bits, and the Intel manual states that bits 63:32 of this
> field *must* be 0.  Use vmcs_write32() where appropriate.
> This fixes booting of an absolutely ancient Red Hat Linux 5.2
> (not Enterprise Linux!) guest.
>
>
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 364263a..311afd4 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1846,7 +1846,22 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu,
>   		vmx->rmode.tr.ar = vmx_segment_access_rights(var);
>   		return;
>   	}
> -	vmcs_writel(sf->base, var->base);
> +
> +	/* Intel 64 and IA-32 Architecture Software Developer's Manual Vol. 3b,
> +	 * section 22.3.1.2 states that VMENTRY will fail if bits 63:32 of the
> +	 * base address for CS, SS, DS, ES are not 0 and the register is usable.
> +	 *
> +	 * If var->base happens to have bit 31 set, then it will get sign
> +	 * extended on the vmcs_writel(), causing this check to fail.  Make
> +	 * sure to use the 32-bit version where appropriate.
> +	 */
> +	if (sf->base == GUEST_CS_BASE ||
> +	    ((~sf->ar_bytes&  0x00010000)&&  (sf->base == GUEST_SS_BASE ||
> +					      sf->base == GUEST_DS_BASE ||
> +					      sf->base == GUEST_ES_BASE)))
> +		vmcs_write32(sf->base, var->base);
>    

This will leave high bits untouched, so if any were set, this will fail.

> +	else
> +		vmcs_writel(sf->base, var->base);
>   	vmcs_write32(sf->limit, var->limit);
>    

I think the correct fix is to zero extend in vmcs_writel() rather than 
here.  But as far as I can tell, it already does.  Where does the sign 
extension occur?  Perhaps in userspace?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH] Fix up vmx_set_segment for booting older guests.

[Chris Lalancette]

Avi Kivity wrote:
>> +	else
>> +		vmcs_writel(sf->base, var->base);
>>   	vmcs_write32(sf->limit, var->limit);
>>    
> 
> I think the correct fix is to zero extend in vmcs_writel() rather than 
> here.  But as far as I can tell, it already does.  Where does the sign 
> extension occur?  Perhaps in userspace?

Very good question Avi.  I should have dug a bit deeper before posting.  I
traced this further back, and here's what it looks like is going on:

arch/x86/kvm/x86.c:kvm_load_segment_descriptor() is responsible for loading the
CPU segment descriptor into the VMCS area.  It does this by calling
load_segment_descriptor_to_kvm_desct(), doing a few minor transformations of the
data, then calling kvm_set_segment() to load it into the VMCS.

The problem arises in load_segment_descriptor_to_kvm_desct() ->
seg_desct_to_kvm_desct().  seg_desct_to_kvm_desct() takes the struct desc_struct
(in this case, base0 == 0x0, base1 == 0x0, and base2 == 0xc0), then calls
get_desc_base() and stores the result in the struct kvm_segment.  The return
value from get_desc_base is It's here that the sign-extension occurs, which
eventually causes that VM entry failure.

get_desc_base() sign-extends because of some complicated u8 to unsigned rules
that I'm not completely sure of.  The below patch fixes my original issue, but
I'm not at all sure that this is the right thing to do.  I could also change
get_desc_base() itself to do the casting, which should do the right thing for
all callers, but I'm not sure if that's what all callers want.  Anybody else
have an opinion?


diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a93ba29..b58bda2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3997,7 +3997,7 @@ static void kvm_set_segment(struct kvm_vcpu *vcpu,
 static void seg_desct_to_kvm_desct(struct desc_struct *seg_desc, u16 selector,
                                   struct kvm_segment *kvm_desct)
 {
-       kvm_desct->base = get_desc_base(seg_desc);
+       kvm_desct->base = (unsigned)get_desc_base(seg_desc);
        kvm_desct->limit = get_desc_limit(seg_desc);
        if (seg_desc->g) {
                kvm_desct->limit <<= 12;


-- 
Chris Lalancette

Re: [PATCH] Fix up vmx_set_segment for booting older guests.

[Avi Kivity]

On 10/20/2009 07:02 PM, Chris Lalancette wrote:
> get_desc_base() sign-extends because of some complicated u8 to unsigned rules
> that I'm not completely sure of.  The below patch fixes my original issue, but
> I'm not at all sure that this is the right thing to do.  I could also change
> get_desc_base() itself to do the casting, which should do the right thing for
> all callers, but I'm not sure if that's what all callers want.  Anybody else
> have an opinion?
>    

get_desc_base() is broken and should be fixed.  No caller could possibly 
want this sign extension (64-bit segment bases are only possible using 
MSR_FS_BASE/MSR_GS_BASE/MSR_KERNEL_GS_BASE).

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.