Re: [PATCH kernel] KVM: Don't null dereference ops->destroy

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 <[email protected]>
> ---
> 
> 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.