From: Alexey Kardashevskiy <aik@ozlabs.ru> To: Sean Christopherson <seanjc@google.com> Cc: kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>, kvm-ppc@vger.kernel.org Subject: Re: [PATCH kernel] KVM: Don't null dereference ops->destroy Date: Wed, 25 May 2022 12:58:41 +1000 [thread overview] Message-ID: <6d291eba-1055-51c3-f015-d029a434b2c0@ozlabs.ru> (raw) In-Reply-To: <cc19c541-0b5b-423e-4323-493fd8dafdd8@ozlabs.ru> 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@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(). 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
WARNING: multiple messages have this Message-ID (diff)
From: Alexey Kardashevskiy <aik@ozlabs.ru> To: Sean Christopherson <seanjc@google.com> Cc: kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>, kvm-ppc@vger.kernel.org Subject: Re: [PATCH kernel] KVM: Don't null dereference ops->destroy Date: Wed, 25 May 2022 02:58:41 +0000 [thread overview] Message-ID: <6d291eba-1055-51c3-f015-d029a434b2c0@ozlabs.ru> (raw) In-Reply-To: <cc19c541-0b5b-423e-4323-493fd8dafdd8@ozlabs.ru> 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@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(). 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
next prev parent reply other threads:[~2022-05-25 2:58 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-24 5:52 [PATCH kernel] KVM: Don't null dereference ops->destroy Alexey Kardashevskiy 2022-05-24 5:52 ` Alexey Kardashevskiy 2022-05-24 20:01 ` Sean Christopherson 2022-05-25 1:52 ` Alexey Kardashevskiy 2022-05-25 1:52 ` Alexey Kardashevskiy 2022-05-25 2:58 ` Alexey Kardashevskiy [this message] 2022-05-25 2:58 ` Alexey Kardashevskiy 2022-05-25 7:47 ` Paolo Bonzini 2022-05-25 7:47 ` Paolo Bonzini 2022-05-31 4:32 ` Alexey Kardashevskiy 2022-05-31 4:32 ` Alexey Kardashevskiy 2022-05-31 17:13 ` Paolo Bonzini 2022-05-31 17:13 ` Paolo Bonzini
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=6d291eba-1055-51c3-f015-d029a434b2c0@ozlabs.ru \ --to=aik@ozlabs.ru \ --cc=kvm-ppc@vger.kernel.org \ --cc=kvm@vger.kernel.org \ --cc=pbonzini@redhat.com \ --cc=seanjc@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.