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

[Leonardo Bras <leonardo@linux.ibm.com>]

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

On Wed, 2019-11-27 at 11:47 -0800, Sean Christopherson wrote:
> On Wed, Nov 27, 2019 at 04:25:55PM -0300, Leonardo Bras wrote:
> > 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()).
> 
> No, using kvm_put_kvm_no_destroy() changes how a bug would manifest, as
> you note below.  Replacing kvm_put_kvm() with kvm_put_kvm_no_destroy()
> when the refcount is _guaranteed_ to be >1 has no impact on correctness.
> 

Humm, so what about the above example with T1 and T2?


> > 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?
> 
> No, the only way to avoid use-after-free bugs of this nature is to not
> screw up the refcounting :-)  This funky "borrowed reference" pattern is
> not very common.  It's necessary here because KVM needs to take an extra
> reference to itself on behalf of the child device before installing the
> child's file descriptor, because once the fd is installed it can be
> closed by userspace and free the child's reference.  The error path,
> which uses kvm_put_kvm_no_destroy(), is used if and only if installing
> the fd fails, in which case the extra reference is deliberately thrown
> away.
> 
> kvm_put_kvm_no_destroy() is asserting "N > 0" as a way to detect a
> refcounting bug that wouldn't be detected (until later) by the normal
> refcounting behavior, which asserts "N >= 0".
> 
> > Is it possible? 
> 
> No.  Similar to above, userspace gets a fd by doing open("/dev/kvm"), and
> the semantics of KVM are such that each fd is a reference to KVM. From
> userspace's perspective, having a valid fd *is* how it knows that it has
> a valid KVM reference.
> 
> > Humm, but if it frees kvm before running ->release(), would it mean the
> > VM is destroyed incorrectly, and will probably crash?
> 
> More than likely the host will crash due to corrupting memory.  The guest
> will crash too, but that's a secondary concern.

Thanks for explaining, it's way more clear to me now how it works.

Best regards,

Leonardo Bras

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