Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <adcfe1b4c5b36b3c398a5d456da9543e0390cba3.camel@linux.ibm.com>
Subject: 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 =?UTF-8?Q?Kr=C4=8Dm=C3=A1=C5=99?= <rkrcmar@redhat.com>,
        kvm-ppc@vger.kernel.org, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Wed, 27 Nov 2019 16:25:55 -0300
In-Reply-To: <6beeff56-7676-5dfd-a578-1732730f8963@redhat.com>
References: <20191021225842.23941-1-sean.j.christopherson@intel.com>
         <de313d549a5ae773aad6bbf04c20b395bea7811f.camel@linux.ibm.com>
         <20191126171416.GA22233@linux.intel.com>
         <0009c6c1bb635098fa68cb6db6414634555039fe.camel@linux.ibm.com>
         <e1a4218f-2a70-3de3-1403-dbebf8a8abdf@redhat.com>
         <bfa563e6a584bd85d3abe953ca088281dc0e167b.camel@linux.ibm.com>
         <6beeff56-7676-5dfd-a578-1732730f8963@redhat.com>
Content-Type: multipart/signed; micalg="pgp-sha256";
        protocol="application/pgp-signature"; boundary="=-a9kzp034ppH43UyepBK6"
User-Agent: Evolution 3.34.1 (3.34.1-1.fc31) 
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--=-a9kzp034ppH43UyepBK6
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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.=20
> > So, suppose these threads, where:
> > - T1 uses a borrowed reference, and=20
> > - T2 is releasing the reference (close, release):
>=20
> Nit: T2 is releasing the *last* reference (as implied by your reference
> to close/release).

Correct.

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

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.=20

> 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=20
> a bug somewhere else), KVM would still hit a use-after-free scenario=20
> as the caller still thinks @kvm is valid.  Currently, this would=20
> 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,=20
> 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?=20

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=20


--=-a9kzp034ppH43UyepBK6
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part
Content-Transfer-Encoding: 7bit

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEMdeUgIzgjf6YmUyOlQYWtz9SttQFAl3ezcMACgkQlQYWtz9S
ttQQBw//fTReHUPDTqQ3dhlnVQc6KZ76CgG2P0n8wJYLoD1w8pnrynqmlniGaIQ6
qcp+epmXKdexJ0dJm3LI8qKcOKVdQaflyUz7Krlko++aAPpO9eoJSH36wocXVfvI
v1n9GCaPstG4b8sPGvdZ+pKiIwnNoG6Z137zM1EbK8GmpfXcYfJhvHx7alvPd+E6
bKoAqlHrzsQk+wWpy9rj8PCB1q2aqE6/vVFYex2dBlQiprPwJcuoVxOeaR+t3S8q
6MuDRJZvo3PXdMXFzrUhabjIWdYz7G59f7lA8wibgdtLfQyDVx0ELwcIBgaIhgTt
2kqepMwa6D5YuSIDM2ZfF/yN/wvvNCDvSHHZexknY4Q2bisVMi8gB9DsRwqYv7aM
h1NhqxgqMjdwjBeb10hqfT7wmlny8CqTYCZYiIprnDajw5cSyV9orrKg4VknfszN
Rks2XwvyFqdu68Z6iWVml2cq3R7DuiE5JnHs3vqWP14SLlsMHJS/NC9HzTYHzEhq
2ECnt2k9g2mdL2KSG/5urNJpL3UJ0m7is/+NG3k70Cml0eL7qcBYvJGh8opHXnP5
24kOpjSWNtcLKOH3o+cDVF8g+LEQiebFzKFyIkUclhZQ+l1n2vbe9JvdPcfhvmRz
5QiAnqjECEPCwj4Fq+EDjvTOd3tFcR72H4frypRpIPrGj4Uasf4=
=AJN6
-----END PGP SIGNATURE-----

--=-a9kzp034ppH43UyepBK6--