Re: [PATCH] KVM: Add separate helper for putting borrowed reference to kvm

From: Leonardo Bras <leonardo@linux.ibm.com>
To: Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <sean.j.christopherson@intel.com>
Cc: "Paul Mackerras" <paulus@ozlabs.org>,
	"Radim Krčmář" <rkrcmar@redhat.com>,
	kvm-ppc@vger.kernel.org, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] KVM: Add separate helper for putting borrowed reference to kvm
Date: Wed, 27 Nov 2019 16:25:55 -0300	[thread overview]
Message-ID: <adcfe1b4c5b36b3c398a5d456da9543e0390cba3.camel@linux.ibm.com> (raw)
In-Reply-To: <6beeff56-7676-5dfd-a578-1732730f8963@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 1914 bytes --]

On Wed, 2019-11-27 at 19:32 +0100, Paolo Bonzini wrote:
> On 27/11/19 19:24, Leonardo Bras wrote:
> > By what I could undestand up to now, these functions that use borrowed
> > references can only be called while the reference (file descriptor)
> > exists. 
> > So, suppose these threads, where:
> > - T1 uses a borrowed reference, and 
> > - T2 is releasing the reference (close, release):
> 
> Nit: T2 is releasing the *last* reference (as implied by your reference
> to close/release).

Correct.

> 
> > T1				| T2
> > kvm_get_kvm()			|
> > ...				| kvm_put_kvm()
> > kvm_put_kvm_no_destroy()	|
> > 
> > The above would not trigger a use-after-free bug, but will cause a
> > memory leak. Is my above understanding right?
> 
> Yes, this is correct.
> 

Then, what would not be a bug before (using kvm_put_kvm()) now is a
memory leak (using kvm_put_kvm_no_destroy()).

And it's the price to avoid use-after-free on other cases, which is a
worse bug. Ok, I get it. 

> Paolo

On Tue, 2019-11-26 at 10:14 -0800, Sean Christopherson wrote:
> If one these kvm_put_kvm() calls did unexpectedly free @kvm (due to 
> a bug somewhere else), KVM would still hit a use-after-free scenario 
> as the caller still thinks @kvm is valid.  Currently, this would 
> only happen on a subsequent ioctl() on the caller's file descriptor
> (which holds a pointer to @kvm), as the callers of these functions
> don't directly dereference @kvm after the functions return.  But, 
> not deferencing @kvm isn't deliberate or functionally required, it's
> just how the code happens to be written.

So, testing if the kvm reference is valid before running ioctl would be
enough to avoid these bugs? Is it possible? 

Humm, but if it frees kvm before running ->release(), would it mean the
VM is destroyed incorrectly, and will probably crash?

Thanks for the patience,

Leonardo 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]