[PATCH v2] KVM: fix load_guest_segment_descriptor() to return X86EMUL_*

[PATCH v2] KVM: fix load_guest_segment_descriptor() to return X86EMUL_*

[Takuya Yoshikawa]

This patch fixes load_guest_segment_descriptor() to return
X86EMUL_PROPAGATE_FAULT when it tries to access the descriptor
table beyond the limit of it: suggested by Marcelo.

I have checked current callers of this helper function,
  - kvm_load_segment_descriptor()
  - kvm_task_switch()
and confirmed that this patch will change nothing in the
upper layers if we do not change the handling of this
return value from load_guest_segment_descriptor().

Next step: Although fixing the kvm_task_switch() to handle the
propagated faults properly seems difficult, and maybe not worth
it because TSS is not used commonly these days, we can fix
kvm_load_segment_descriptor(). By doing so, the injected #GP
becomes possible to be handled by the guest. The only problem
for this is how to differentiate this fault from the page faults
generated by kvm_read_guest_virt(). We may have to split this
function to achive this goal.


Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/x86.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d47ceda..e5335e5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4662,7 +4662,7 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
 
 	if (dtable.limit < index * 8 + 7) {
 		kvm_queue_exception_e(vcpu, GP_VECTOR, selector & 0xfffc);
-		return 1;
+		return X86EMUL_PROPAGATE_FAULT;
 	}
 	return kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
 }
-- 
1.6.3.3

Re: [PATCH v2] KVM: fix load_guest_segment_descriptor() to return X86EMUL_*

[Takuya Yoshikawa]

A bit more explanation,

Takuya Yoshikawa wrote:
> This patch fixes load_guest_segment_descriptor() to return
> X86EMUL_PROPAGATE_FAULT when it tries to access the descriptor
> table beyond the limit of it: suggested by Marcelo.
> 
> I have checked current callers of this helper function,
>   - kvm_load_segment_descriptor()
>   - kvm_task_switch()
> and confirmed that this patch will change nothing in the
> upper layers if we do not change the handling of this
> return value from load_guest_segment_descriptor().
> 
> Next step: Although fixing the kvm_task_switch() to handle the
> propagated faults properly seems difficult, and maybe not worth
> it because TSS is not used commonly these days, we can fix
> kvm_load_segment_descriptor(). By doing so, the injected #GP
> becomes possible to be handled by the guest. The only problem
> for this is how to differentiate this fault from the page faults
> generated by kvm_read_guest_virt(). We may have to split this
> function to achive this goal.
> 

My concern is we may have to inject different types of
faults/exceptions depending on callers when kvm_read_guest_virt()
returns X86EMUL_PROPAGATE_FAULT. Actually if always injecting
page faults in the load_guest_segment_descriptor() right after
kvm_read_guest_virt() is OK, we do not have any problems.

Personally I think we'd better to inject page faults for
kvm_load_segment_descriptor().

Is it right?

> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> ---
>  arch/x86/kvm/x86.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d47ceda..e5335e5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4662,7 +4662,7 @@ static int load_guest_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector,
>  
>  	if (dtable.limit < index * 8 + 7) {
>  		kvm_queue_exception_e(vcpu, GP_VECTOR, selector & 0xfffc);
> -		return 1;
> +		return X86EMUL_PROPAGATE_FAULT;
>  	}
>  	return kvm_read_guest_virt(dtable.base + index*8, seg_desc, sizeof(*seg_desc), vcpu);
>  }

Re: [PATCH v2] KVM: fix load_guest_segment_descriptor() to return X86EMUL_*

[Marcelo Tosatti]

On Tue, Feb 02, 2010 at 09:49:35PM +0900, Takuya Yoshikawa wrote:
> A bit more explanation,
> 
> Takuya Yoshikawa wrote:
> >This patch fixes load_guest_segment_descriptor() to return
> >X86EMUL_PROPAGATE_FAULT when it tries to access the descriptor
> >table beyond the limit of it: suggested by Marcelo.
> >
> >I have checked current callers of this helper function,
> >  - kvm_load_segment_descriptor()
> >  - kvm_task_switch()
> >and confirmed that this patch will change nothing in the
> >upper layers if we do not change the handling of this
> >return value from load_guest_segment_descriptor().
> >
> >Next step: Although fixing the kvm_task_switch() to handle the
> >propagated faults properly seems difficult, and maybe not worth
> >it because TSS is not used commonly these days, we can fix
> >kvm_load_segment_descriptor(). By doing so, the injected #GP
> >becomes possible to be handled by the guest. The only problem
> >for this is how to differentiate this fault from the page faults
> >generated by kvm_read_guest_virt(). We may have to split this
> >function to achive this goal.
> >
> 
> My concern is we may have to inject different types of
> faults/exceptions depending on callers when kvm_read_guest_virt()
> returns X86EMUL_PROPAGATE_FAULT. Actually if always injecting
> page faults in the load_guest_segment_descriptor() right after
> kvm_read_guest_virt() is OK, we do not have any problems.
> 
> Personally I think we'd better to inject page faults for
> kvm_load_segment_descriptor().
> 
> Is it right?

Yes, if kvm_read_guest_virt fails, inject page fault.

Applied patch, thanks.