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

On 5/25/22 11:52, Alexey Kardashevskiy wrote:
> 
> 
> 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 <[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().


After reading
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id+de9b3ec8bdf60788e9e 
and neighboring commits, it sounds that each create() should be paired 
with either destroy() or release() but not necessarily both.

So I'm really not sure dummy handlers is a good idea. Thanks,


>> 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/?idT22e95103cf9663bc
> 
> 
>> 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/?ido868405faf067e8cfb
> 
> 
> 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