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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <adcfe1b4c5b36b3c398a5d456da9543e0390cba3.camel@linux.ibm.com>
Date:   Wed, 27 Nov 2019 16:25:55 -0300
From:   Leonardo Bras <leonardo@...ux.ibm.com>
To:     Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     Paul Mackerras <paulus@...abs.org>,
        Radim Krčmář <rkrcmar@...hat.com>,
        kvm-ppc@...r.kernel.org, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: Add separate helper for putting borrowed reference
 to kvm

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 





Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)