* [PATCH] KVM: X86: Fix warning in handle_desc
@ 2019-09-16 7:12 Wanpeng Li
2019-09-16 19:12 ` Sean Christopherson
0 siblings, 1 reply; 3+ messages in thread
From: Wanpeng Li @ 2019-09-16 7:12 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Paolo Bonzini, Radim Krčmář,
Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, stable
From: Wanpeng Li <wanpengli@tencent.com>
Reported by syzkaller:
WARNING: CPU: 0 PID: 6544 at /home/kernel/data/kvm/arch/x86/kvm//vmx/vmx.c:4689 handle_desc+0x37/0x40 [kvm_intel]
CPU: 0 PID: 6544 Comm: a.out Tainted: G OE 5.3.0-rc4+ #4
RIP: 0010:handle_desc+0x37/0x40 [kvm_intel]
Call Trace:
vmx_handle_exit+0xbe/0x6b0 [kvm_intel]
vcpu_enter_guest+0x4dc/0x18d0 [kvm]
kvm_arch_vcpu_ioctl_run+0x407/0x660 [kvm]
kvm_vcpu_ioctl+0x3ad/0x690 [kvm]
do_vfs_ioctl+0xa2/0x690
ksys_ioctl+0x6d/0x80
__x64_sys_ioctl+0x1a/0x20
do_syscall_64+0x74/0x720
entry_SYSCALL_64_after_hwframe+0x49/0xbe
When CR4.UMIP is set, guest should have UMIP cpuid flag. Current
kvm set_sregs function doesn't have such check when userspace inputs
sregs values. SECONDARY_EXEC_DESC is enabled on writes to CR4.UMIP in
vmx_set_cr4 though guest doesn't have UMIP cpuid flag. The testcast
triggers handle_desc warning when executing ltr instruction since guest
architectural CR4 doesn't set UMIP. This patch fixes it by adding check
for guest UMIP cpuid flag when get sreg inputs from userspace.
Reported-by: syzbot+0f1819555fbdce992df9@syzkaller.appspotmail.com
Fixes: 0367f205a3b7 ("KVM: vmx: add support for emulating UMIP")
Cc: stable@vger.kernel.org
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
Note: syzbot report link https://lkml.org/lkml/2019/9/11/799
arch/x86/kvm/x86.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f7cfd8e..83288ba 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8645,6 +8645,10 @@ static int kvm_valid_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs)
(sregs->cr4 & X86_CR4_OSXSAVE))
return -EINVAL;
+ if (!guest_cpuid_has(vcpu, X86_FEATURE_UMIP) &&
+ (sregs->cr4 & X86_CR4_UMIP))
+ return -EINVAL;
+
if ((sregs->efer & EFER_LME) && (sregs->cr0 & X86_CR0_PG)) {
/*
* When EFER.LME and CR0.PG are set, the processor is in
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] KVM: X86: Fix warning in handle_desc
2019-09-16 7:12 [PATCH] KVM: X86: Fix warning in handle_desc Wanpeng Li
@ 2019-09-16 19:12 ` Sean Christopherson
2019-09-17 8:22 ` Wanpeng Li
0 siblings, 1 reply; 3+ messages in thread
From: Sean Christopherson @ 2019-09-16 19:12 UTC (permalink / raw)
To: Wanpeng Li
Cc: linux-kernel, kvm, Paolo Bonzini, Radim Krčmář,
Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, stable
On Mon, Sep 16, 2019 at 03:12:49PM +0800, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
>
> Reported by syzkaller:
>
> WARNING: CPU: 0 PID: 6544 at /home/kernel/data/kvm/arch/x86/kvm//vmx/vmx.c:4689 handle_desc+0x37/0x40 [kvm_intel]
> CPU: 0 PID: 6544 Comm: a.out Tainted: G OE 5.3.0-rc4+ #4
> RIP: 0010:handle_desc+0x37/0x40 [kvm_intel]
> Call Trace:
> vmx_handle_exit+0xbe/0x6b0 [kvm_intel]
> vcpu_enter_guest+0x4dc/0x18d0 [kvm]
> kvm_arch_vcpu_ioctl_run+0x407/0x660 [kvm]
> kvm_vcpu_ioctl+0x3ad/0x690 [kvm]
> do_vfs_ioctl+0xa2/0x690
> ksys_ioctl+0x6d/0x80
> __x64_sys_ioctl+0x1a/0x20
> do_syscall_64+0x74/0x720
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> When CR4.UMIP is set, guest should have UMIP cpuid flag. Current
> kvm set_sregs function doesn't have such check when userspace inputs
> sregs values. SECONDARY_EXEC_DESC is enabled on writes to CR4.UMIP in
> vmx_set_cr4 though guest doesn't have UMIP cpuid flag. The testcast
> triggers handle_desc warning when executing ltr instruction since guest
> architectural CR4 doesn't set UMIP. This patch fixes it by adding check
> for guest UMIP cpuid flag when get sreg inputs from userspace.
>
> Reported-by: syzbot+0f1819555fbdce992df9@syzkaller.appspotmail.com
> Fixes: 0367f205a3b7 ("KVM: vmx: add support for emulating UMIP")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
> Note: syzbot report link https://lkml.org/lkml/2019/9/11/799
>
> arch/x86/kvm/x86.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index f7cfd8e..83288ba 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8645,6 +8645,10 @@ static int kvm_valid_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs)
> (sregs->cr4 & X86_CR4_OSXSAVE))
> return -EINVAL;
>
> + if (!guest_cpuid_has(vcpu, X86_FEATURE_UMIP) &&
> + (sregs->cr4 & X86_CR4_UMIP))
Assuming vmx_set_cr4() fails because nested_cr4_valid() fails, isn't this
a generic problem with nested VMX that just happens to be visible because
of the WARN_ON() in handle_desc()?
In general, KVM lets userspace set broken combinations of CPUID vs. CRx so
that it doesn't dictate ordering, e.g. __set_sregs() intentionally calls
kvm_x86_ops->set_cr4() instead of kvm_set_cr4(), which has all the CPUID
checks.
The existing OSXSAVE check in kvm_valid_sregs() is more about ensuring
host support (see commit 6d1068b3a985, "KVM: x86: invalid opcode oops on
SET_SREGS with OSXSAVE bit set (CVE-2012-4461)").
Given that both vmx_set_cr4() and svm_set_cr4() can return failure and
cause __set_sregs() to silently fail, what about adding a new x86 ops to
pre-check cr4, e.g. vm_x86_ops->is_valid_cr4(), and then WARN if set_cr4()
fails during __set_sregs()?
> + return -EINVAL;
> +
> if ((sregs->efer & EFER_LME) && (sregs->cr0 & X86_CR0_PG)) {
> /*
> * When EFER.LME and CR0.PG are set, the processor is in
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] KVM: X86: Fix warning in handle_desc
2019-09-16 19:12 ` Sean Christopherson
@ 2019-09-17 8:22 ` Wanpeng Li
0 siblings, 0 replies; 3+ messages in thread
From: Wanpeng Li @ 2019-09-17 8:22 UTC (permalink / raw)
To: Sean Christopherson
Cc: LKML, kvm, Paolo Bonzini, Radim Krčmář,
Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
# v3 . 10+
On Tue, 17 Sep 2019 at 03:12, Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Mon, Sep 16, 2019 at 03:12:49PM +0800, Wanpeng Li wrote:
> > From: Wanpeng Li <wanpengli@tencent.com>
> >
> > Reported by syzkaller:
> >
> > WARNING: CPU: 0 PID: 6544 at /home/kernel/data/kvm/arch/x86/kvm//vmx/vmx.c:4689 handle_desc+0x37/0x40 [kvm_intel]
> > CPU: 0 PID: 6544 Comm: a.out Tainted: G OE 5.3.0-rc4+ #4
> > RIP: 0010:handle_desc+0x37/0x40 [kvm_intel]
> > Call Trace:
> > vmx_handle_exit+0xbe/0x6b0 [kvm_intel]
> > vcpu_enter_guest+0x4dc/0x18d0 [kvm]
> > kvm_arch_vcpu_ioctl_run+0x407/0x660 [kvm]
> > kvm_vcpu_ioctl+0x3ad/0x690 [kvm]
> > do_vfs_ioctl+0xa2/0x690
> > ksys_ioctl+0x6d/0x80
> > __x64_sys_ioctl+0x1a/0x20
> > do_syscall_64+0x74/0x720
> > entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >
> > When CR4.UMIP is set, guest should have UMIP cpuid flag. Current
> > kvm set_sregs function doesn't have such check when userspace inputs
> > sregs values. SECONDARY_EXEC_DESC is enabled on writes to CR4.UMIP in
> > vmx_set_cr4 though guest doesn't have UMIP cpuid flag. The testcast
> > triggers handle_desc warning when executing ltr instruction since guest
> > architectural CR4 doesn't set UMIP. This patch fixes it by adding check
> > for guest UMIP cpuid flag when get sreg inputs from userspace.
> >
> > Reported-by: syzbot+0f1819555fbdce992df9@syzkaller.appspotmail.com
> > Fixes: 0367f205a3b7 ("KVM: vmx: add support for emulating UMIP")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> > ---
> > Note: syzbot report link https://lkml.org/lkml/2019/9/11/799
> >
> > arch/x86/kvm/x86.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index f7cfd8e..83288ba 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -8645,6 +8645,10 @@ static int kvm_valid_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs)
> > (sregs->cr4 & X86_CR4_OSXSAVE))
> > return -EINVAL;
> >
> > + if (!guest_cpuid_has(vcpu, X86_FEATURE_UMIP) &&
> > + (sregs->cr4 & X86_CR4_UMIP))
>
> Assuming vmx_set_cr4() fails because nested_cr4_valid() fails, isn't this
> a generic problem with nested VMX that just happens to be visible because
> of the WARN_ON() in handle_desc()?
>
> In general, KVM lets userspace set broken combinations of CPUID vs. CRx so
> that it doesn't dictate ordering, e.g. __set_sregs() intentionally calls
> kvm_x86_ops->set_cr4() instead of kvm_set_cr4(), which has all the CPUID
> checks.
>
> The existing OSXSAVE check in kvm_valid_sregs() is more about ensuring
> host support (see commit 6d1068b3a985, "KVM: x86: invalid opcode oops on
> SET_SREGS with OSXSAVE bit set (CVE-2012-4461)").
>
> Given that both vmx_set_cr4() and svm_set_cr4() can return failure and
> cause __set_sregs() to silently fail, what about adding a new x86 ops to
> pre-check cr4, e.g. vm_x86_ops->is_valid_cr4(), and then WARN if set_cr4()
> fails during __set_sregs()?
I'm a little lazy, just extract CPUID and CR4 combination checking and
add it to __set_sregs() in new version.
Wanpeng
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-09-17 8:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-16 7:12 [PATCH] KVM: X86: Fix warning in handle_desc Wanpeng Li
2019-09-16 19:12 ` Sean Christopherson
2019-09-17 8:22 ` Wanpeng Li
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).