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:
signature.asc
Description: This is a digitally signed message part
