[RFC] KVM: SVM: ignore type when setting segment registers

[RFC] KVM: SVM: ignore type when setting segment registers

[Gioh Kim]

Current code sets unusable as 1 if present is 1 and type is 0.
In Long mode, type value in segment descriptor is ignored.
So I think type should be ignored when setting the segment registers,
if type means the descriptor type in the segment descriptor.

Is the type field of struct kvm_segment the descriptor type?
If so, why type is checked when setting segment registers?

If the type field is not the descriptor type,
is it ok to set unusable when present is 1?

I'm copying a code as following to show what code I'm asking.

----------------------------- 8< ---------------------------------
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 5f48f62..0133f6f 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1803,7 +1803,7 @@ static void svm_get_segment(struct kvm_vcpu *vcpu,
 	 * AMD's VMCB does not have an explicit unusable field, so emulate it
 	 * for cross vendor migration purposes by "not present"
 	 */
-	var->unusable = !var->present || (var->type == 0);
+	var->unusable = !var->present;
 
 	switch (seg) {
 	case VCPU_SREG_TR:
-- 
2.5.0

Re: [RFC] KVM: SVM: ignore type when setting segment registers

[Radim Krčmář]

2017-05-29 15:24+0200, Gioh Kim:
> Current code sets unusable as 1 if present is 1 and type is 0.
> In Long mode, type value in segment descriptor is ignored.
> So I think type should be ignored when setting the segment registers,
> if type means the descriptor type in the segment descriptor.
> 
> Is the type field of struct kvm_segment the descriptor type?

Yes.

> If so, why type is checked when setting segment registers?

No idea.  19bca6ab75d8 ("KVM: SVM: Fix cross vendor migration issue with
unusable bit") also moved the assigment up to initialize it before use
and I think that is enough.

> If the type field is not the descriptor type,
> is it ok to set unusable when present is 1?

Looks like a bug.  type = 0 can be a usable read-only data segment.

> I'm copying a code as following to show what code I'm asking.

Please send it as a patch,

thanks.

> ----------------------------- 8< ---------------------------------
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 5f48f62..0133f6f 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1803,7 +1803,7 @@ static void svm_get_segment(struct kvm_vcpu *vcpu,
>  	 * AMD's VMCB does not have an explicit unusable field, so emulate it
>  	 * for cross vendor migration purposes by "not present"
>  	 */
> -	var->unusable = !var->present || (var->type == 0);
> +	var->unusable = !var->present;
>  
>  	switch (seg) {
>  	case VCPU_SREG_TR:
> -- 
> 2.5.0
> 

Re: [RFC] KVM: SVM: ignore type when setting segment registers

[Gioh Kim]

On Tue, May 30, 2017 at 02:54:21PM +0200, Radim Krčmář wrote:
> 2017-05-29 15:24+0200, Gioh Kim:
> > Current code sets unusable as 1 if present is 1 and type is 0.
> > In Long mode, type value in segment descriptor is ignored.
> > So I think type should be ignored when setting the segment registers,
> > if type means the descriptor type in the segment descriptor.
> > 
> > Is the type field of struct kvm_segment the descriptor type?
> 
> Yes.
> 
> > If so, why type is checked when setting segment registers?
> 
> No idea.  19bca6ab75d8 ("KVM: SVM: Fix cross vendor migration issue with
> unusable bit") also moved the assigment up to initialize it before use
> and I think that is enough.
> 
> > If the type field is not the descriptor type,
> > is it ok to set unusable when present is 1?
> 
> Looks like a bug.  type = 0 can be a usable read-only data segment.
> 
> > I'm copying a code as following to show what code I'm asking.
> 
> Please send it as a patch,

Hi Radim,

Thank you for reply.
I sent a patch: https://lkml.org/lkml/2017/5/30/459
I'd appreciate if if you could review it.

-- 
Best regards,
Gi-Oh Kim
TEL: 0176 2697 8962

Re: [RFC] KVM: SVM: ignore type when setting segment registers

[Matt Mullins]

On Tue, May 30, 2017 at 02:54:21PM +0200, Radim Krčmář wrote:
> 2017-05-29 15:24+0200, Gioh Kim:
> > If so, why type is checked when setting segment registers?
> 
> No idea.  19bca6ab75d8 ("KVM: SVM: Fix cross vendor migration issue with
> unusable bit") also moved the assigment up to initialize it before use
> and I think that is enough.

Was this perhaps intended to instead check for a zero selector, which is also
an unusable segment?

Re: [RFC] KVM: SVM: ignore type when setting segment registers

[Gi-Oh Kim]

On Tue, May 30, 2017 at 8:03 PM, Matt Mullins <mmullins@mmlx.us> wrote:
> On Tue, May 30, 2017 at 02:54:21PM +0200, Radim Krčmář wrote:
>> 2017-05-29 15:24+0200, Gioh Kim:
>> > If so, why type is checked when setting segment registers?
>>
>> No idea.  19bca6ab75d8 ("KVM: SVM: Fix cross vendor migration issue with
>> unusable bit") also moved the assigment up to initialize it before use
>> and I think that is enough.
>
> Was this perhaps intended to instead check for a zero selector, which is also
> an unusable segment?

I think that is what present value is for.


-- 
Best regards,
Gi-Oh Kim
TEL: 0176 2697 8962