| Message ID | 20211122232149.2927356-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: VMX: do not use uninitialized gfn_to_hva_cache | expand |
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 >
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
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(-)