* [PATCH] KVM: nVMX: Store vmcs.EXIT_QUALIFICATION as an unsigned long, not u32
@ 2020-04-23 0:11 Sean Christopherson
2020-04-24 11:44 ` Vitaly Kuznetsov
0 siblings, 1 reply; 3+ messages in thread
From: Sean Christopherson @ 2020-04-23 0:11 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
Use an unsigned long for 'exit_qual' in nested_vmx_reflect_vmexit(), the
EXIT_QUALIFICATION field is naturally sized, not a 32-bit field.
The bug is most easily observed by doing VMXON (or any VMX instruction)
in L2 with a negative displacement, in which case dropping the upper
bits on nested VM-Exit results in L1 calculating the wrong virtual
address for the memory operand, e.g. "vmxon -0x8(%rbp)" yields:
Unhandled cpu exception 14 #PF at ip 0000000000400553
rbp=0000000000537000 cr2=0000000100536ff8
Fixes: fbdd50250396d ("KVM: nVMX: Move VM-Fail check out of nested_vmx_exit_reflected()")
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
Sadly (for me), I can't blame a mishandled merge on this one. Even more
embarassing is that this is actually the second instance where I botched
the size for exit_qual, you'd think I'd have double-checked everything
after the first one...
arch/x86/kvm/vmx/nested.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index f228339cd0a0..3f32f81f5c59 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5814,7 +5814,8 @@ bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu)
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
u32 exit_reason = vmx->exit_reason;
- u32 exit_intr_info, exit_qual;
+ unsigned long exit_qual;
+ u32 exit_intr_info;
WARN_ON_ONCE(vmx->nested.nested_run_pending);
--
2.26.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: nVMX: Store vmcs.EXIT_QUALIFICATION as an unsigned long, not u32
2020-04-23 0:11 [PATCH] KVM: nVMX: Store vmcs.EXIT_QUALIFICATION as an unsigned long, not u32 Sean Christopherson
@ 2020-04-24 11:44 ` Vitaly Kuznetsov
2020-04-24 14:16 ` Sean Christopherson
0 siblings, 1 reply; 3+ messages in thread
From: Vitaly Kuznetsov @ 2020-04-24 11:44 UTC (permalink / raw)
To: Sean Christopherson
Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, Paolo Bonzini
Sean Christopherson <sean.j.christopherson@intel.com> writes:
> Use an unsigned long for 'exit_qual' in nested_vmx_reflect_vmexit(), the
> EXIT_QUALIFICATION field is naturally sized, not a 32-bit field.
>
> The bug is most easily observed by doing VMXON (or any VMX instruction)
> in L2 with a negative displacement, in which case dropping the upper
> bits on nested VM-Exit results in L1 calculating the wrong virtual
> address for the memory operand, e.g. "vmxon -0x8(%rbp)" yields:
>
> Unhandled cpu exception 14 #PF at ip 0000000000400553
> rbp=0000000000537000 cr2=0000000100536ff8
>
> Fixes: fbdd50250396d ("KVM: nVMX: Move VM-Fail check out of nested_vmx_exit_reflected()")
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>
> Sadly (for me), I can't blame a mishandled merge on this one. Even more
> embarassing is that this is actually the second instance where I botched
> the size for exit_qual, you'd think I'd have double-checked everything
> after the first one...
>
> arch/x86/kvm/vmx/nested.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index f228339cd0a0..3f32f81f5c59 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -5814,7 +5814,8 @@ bool nested_vmx_reflect_vmexit(struct kvm_vcpu *vcpu)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> u32 exit_reason = vmx->exit_reason;
> - u32 exit_intr_info, exit_qual;
> + unsigned long exit_qual;
> + u32 exit_intr_info;
>
> WARN_ON_ONCE(vmx->nested.nested_run_pending);
Too late but
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
I also did 'git grep -W 'u32.*exit_qual' kvm/queue' and I can see a few
more places where 'exit_qual' is u32:
nested_vmx_check_guest_state()
nested_vmx_enter_non_root_mode()
vmx_set_nested_state()
Being too lazy to check an even if there are no immediate issues with
that, should we just use 'unsigned long' everywhere?
--
Vitaly
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: nVMX: Store vmcs.EXIT_QUALIFICATION as an unsigned long, not u32
2020-04-24 11:44 ` Vitaly Kuznetsov
@ 2020-04-24 14:16 ` Sean Christopherson
0 siblings, 0 replies; 3+ messages in thread
From: Sean Christopherson @ 2020-04-24 14:16 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Wanpeng Li, Jim Mattson, Joerg Roedel, kvm, linux-kernel, Paolo Bonzini
On Fri, Apr 24, 2020 at 01:44:00PM +0200, Vitaly Kuznetsov wrote:
> I also did 'git grep -W 'u32.*exit_qual' kvm/queue' and I can see a few
> more places where 'exit_qual' is u32:
> nested_vmx_check_guest_state()
> nested_vmx_enter_non_root_mode()
> vmx_set_nested_state()
>
> Being too lazy to check an even if there are no immediate issues with
> that, should we just use 'unsigned long' everywhere?
Yes, absolutely, I'll send a patch.
The existing cases are benign, they're all related to setting the exit_qual
for a nested VM-Enter failure, which could fit in a u8. But still worth
fixing.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-04-24 14:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-23 0:11 [PATCH] KVM: nVMX: Store vmcs.EXIT_QUALIFICATION as an unsigned long, not u32 Sean Christopherson
2020-04-24 11:44 ` Vitaly Kuznetsov
2020-04-24 14:16 ` Sean Christopherson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).