* [PATCH] KVM: VMX: do not use uninitialized gfn_to_hva_cache
@ 2021-11-22 23:21 Paolo Bonzini
2021-11-23 9:26 ` David Woodhouse
0 siblings, 1 reply; 2+ messages in thread
From: Paolo Bonzini @ 2021-11-22 23:21 UTC (permalink / raw)
To: linux-kernel, kvm; +Cc: David Woodhouse, syzbot+7b7db8bb4db6fd5e157b
An uninitialized gfn_to_hva_cache has ghc->len == 0, which causes
the accessors to croak very loudly. While a BUG_ON is definitely
_too_ loud and a bug on its own, there is indeed an issue of using
the caches in such a way that they could not have been initialized,
because ghc->gpa == 0 might match and thus kvm_gfn_to_hva_cache_init
would not be called.
For the vmcs12_cache, the solution is simply to invoke
kvm_gfn_to_hva_cache_init unconditionally: we already know
that the cache does not match the current VMCS pointer.
For the shadow_vmcs12_cache, there is no similar condition
that checks the VMCS link pointer, so invalidate the cache
on VMXON.
Fixes: cee66664dcd6 ("KVM: nVMX: Use a gfn_to_hva_cache for vmptrld")
Cc: David Woodhouse <dwmw@amazon.co.uk>
Reported-by: syzbot+7b7db8bb4db6fd5e157b@syzkaller.appspotmail.com
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
arch/x86/kvm/vmx/nested.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 1e2f66951566..315fa456d368 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -4857,6 +4857,7 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu)
if (!vmx->nested.cached_vmcs12)
goto out_cached_vmcs12;
+ vmx->nested.shadow_vmcs12_cache.gpa = INVALID_GPA;
vmx->nested.cached_shadow_vmcs12 = kzalloc(VMCS12_SIZE, GFP_KERNEL_ACCOUNT);
if (!vmx->nested.cached_shadow_vmcs12)
goto out_cached_shadow_vmcs12;
@@ -5289,8 +5290,7 @@ static int handle_vmptrld(struct kvm_vcpu *vcpu)
struct gfn_to_hva_cache *ghc = &vmx->nested.vmcs12_cache;
struct vmcs_hdr hdr;
- if (ghc->gpa != vmptr &&
- kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, vmptr, VMCS12_SIZE)) {
+ if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, vmptr, VMCS12_SIZE)) {
/*
* Reads from an unbacked page return all 1s,
* which means that the 32 bits located at the
--
2.27.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] KVM: VMX: do not use uninitialized gfn_to_hva_cache
2021-11-22 23:21 [PATCH] KVM: VMX: do not use uninitialized gfn_to_hva_cache Paolo Bonzini
@ 2021-11-23 9:26 ` David Woodhouse
0 siblings, 0 replies; 2+ messages in thread
From: David Woodhouse @ 2021-11-23 9:26 UTC (permalink / raw)
To: Paolo Bonzini, linux-kernel, kvm; +Cc: syzbot+7b7db8bb4db6fd5e157b
[-- Attachment #1: Type: text/plain, Size: 2681 bytes --]
On Mon, 2021-11-22 at 18:21 -0500, Paolo Bonzini wrote:
> An uninitialized gfn_to_hva_cache has ghc->len == 0, which causes
> the accessors to croak very loudly. While a BUG_ON is definitely
> _too_ loud and a bug on its own, there is indeed an issue of using
> the caches in such a way that they could not have been initialized,
> because ghc->gpa == 0 might match and thus kvm_gfn_to_hva_cache_init
> would not be called.
Hm, in real usage, I thought an initialized-to-zeroes cache would have
been considered invalid regardless of the gpa, because ghc->generation
wouldn't have matched slots->generation.
But in the syzbot case perhaps it hadn't actually added any memslots
yet, so slots->generation == 0 and that doesn't help. Although....
would it even have got that far if there weren't any memslots at all?
> For the vmcs12_cache, the solution is simply to invoke
> kvm_gfn_to_hva_cache_init unconditionally: we already know
> that the cache does not match the current VMCS pointer.
> For the shadow_vmcs12_cache, there is no similar condition
> that checks the VMCS link pointer, so invalidate the cache
> on VMXON.
>
> Fixes: cee66664dcd6 ("KVM: nVMX: Use a gfn_to_hva_cache for vmptrld")
> Cc: David Woodhouse <dwmw@amazon.co.uk>
> Reported-by: syzbot+7b7db8bb4db6fd5e157b@syzkaller.appspotmail.com
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: David Woodhouse <dwmw@amazon.co.uk>
Thanks, I was just staring at the report when I realised you'd already
fixed it.
> ---
> arch/x86/kvm/vmx/nested.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 1e2f66951566..315fa456d368 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -4857,6 +4857,7 @@ static int enter_vmx_operation(struct kvm_vcpu
> *vcpu)
> if (!vmx->nested.cached_vmcs12)
> goto out_cached_vmcs12;
>
> + vmx->nested.shadow_vmcs12_cache.gpa = INVALID_GPA;
> vmx->nested.cached_shadow_vmcs12 = kzalloc(VMCS12_SIZE,
> GFP_KERNEL_ACCOUNT);
> if (!vmx->nested.cached_shadow_vmcs12)
> goto out_cached_shadow_vmcs12;
> @@ -5289,8 +5290,7 @@ static int handle_vmptrld(struct kvm_vcpu
> *vcpu)
> struct gfn_to_hva_cache *ghc = &vmx-
> >nested.vmcs12_cache;
> struct vmcs_hdr hdr;
>
> - if (ghc->gpa != vmptr &&
> - kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, vmptr,
> VMCS12_SIZE)) {
> + if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, vmptr,
> VMCS12_SIZE)) {
> /*
> * Reads from an unbacked page return all 1s,
> * which means that the 32 bits located at the
>
[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-11-23 9:26 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-22 23:21 [PATCH] KVM: VMX: do not use uninitialized gfn_to_hva_cache Paolo Bonzini
2021-11-23 9:26 ` David Woodhouse
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).