linux-kernel - Re: [PATCH] KVM: VMX: do not use uninitialized gfn_to_hva

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <606db16f34214998d0c7e00e379b40d8d33f9f92.camel@infradead.org>
Date:   Tue, 23 Nov 2021 09:26:10 +0000
From:   David Woodhouse <dwmw2@...radead.org>
To:     Paolo Bonzini <pbonzini@...hat.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     syzbot+7b7db8bb4db6fd5e157b@...kaller.appspotmail.com
Subject: Re: [PATCH] KVM: VMX: do not use uninitialized gfn_to_hva_cache

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@...zon.co.uk>
> Reported-by: syzbot+7b7db8bb4db6fd5e157b@...kaller.appspotmail.com
> 
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>

Acked-by: David Woodhouse <dwmw@...zon.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
> 


Download attachment "smime.p7s" of type "application/pkcs7-signature" (5174 bytes)