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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 5/25/22 06:01, Sean Christopherson wrote:
> 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 <aik@ozlabs.ru>
>> ---
>>
>> 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().

Huh it used to be release() actually, nice:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5422e95103cf9663bc


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

I could probably borrow some docs from here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6f868405faf067e8cfb


Thanks for the review! At very least I'll add the dummies.


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




-- 
Alexey