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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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 <aik@xxxxxxxxx>
---

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=2bde9b3ec8bdf60788e9e 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/?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



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux