From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Christopherson Date: Tue, 24 May 2022 20:01:58 +0000 Subject: Re: [PATCH kernel] KVM: Don't null dereference ops->destroy Message-Id: List-Id: References: <20220524055208.1269279-1-aik@ozlabs.ru> In-Reply-To: <20220524055208.1269279-1-aik@ozlabs.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Alexey Kardashevskiy Cc: kvm@vger.kernel.org, Paolo Bonzini , kvm-ppc@vger.kernel.org On Tue, May 24, 2022, Alexey Kardashevskiy wrote: > There are 2 places calling kvm_device_ops::destroy(): > 1) when creating a KVM device failed; > 2) when a VM is destroyed: kvm_destroy_devices() destroys all devices > from &kvm->devices. > > All 3 Book3s's interrupt controller KVM devices (XICS, XIVE, XIVE-native) > do not define kvm_device_ops::destroy() and only define release() which > is normally fine as device fds are closed before KVM gets to 2) but > by then the &kvm->devices list is empty. > > However Syzkaller manages to trigger 1). > > This adds checks in 1) and 2). > > Signed-off-by: Alexey Kardashevskiy > --- > > I could define empty handlers for XICS/XIVE guys but > kvm_ioctl_create_device() already checks for ops->init() so I guess > kvm_device_ops are expected to not have certain handlers. Oof. IMO, ->destroy() should be mandatory in order to pair with ->create(). kvmppc_xive_create(), kvmppc_xics_create(), and kvmppc_core_destroy_vm() are doing some truly funky stuff to avoid leaking the device that's allocate in ->create(). A nop/dummy ->destroy() would be a good place to further document those shenanigans. There's a comment at the end of the ->release() hooks, but that's still not very obvious. The comment above kvmppc_xive_get_device() strongly suggests that keeping the allocation is a hack to avoid having to audit all relevant code paths, i.e. isn't done for performance reasons.