* [PATCH v2] KVM: x86: Handle SRCU initialization failure during page track init
@ 2021-09-03 2:37 tcs.kernel
2021-09-03 6:59 ` Jarkko Sakkinen
2021-09-03 16:18 ` Sean Christopherson
0 siblings, 2 replies; 4+ messages in thread
From: tcs.kernel @ 2021-09-03 2:37 UTC (permalink / raw)
To: pbonzini, seanjc, vkuznets, wanpengli, jmattson, kvm,
linux-kernel, jarkko
Cc: Haimin Zhang
From: Haimin Zhang <tcs_kernel@tencent.com>
Check the return of init_srcu_struct(), which can fail due to OOM, when
initializing the page track mechanism. Lack of checking leads to a NULL
pointer deref found by a modified syzkaller.
Signed-off-by: Haimin Zhang <tcs_kernel@tencent.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Reported-by: TCS Robot <tcs_robot@tencent.com>
---
arch/x86/include/asm/kvm_page_track.h | 2 +-
arch/x86/kvm/mmu/page_track.c | 4 ++--
arch/x86/kvm/x86.c | 6 +++++-
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/kvm_page_track.h b/arch/x86/include/asm/kvm_page_track.h
index 8766adb..9cd9230 100644
--- a/arch/x86/include/asm/kvm_page_track.h
+++ b/arch/x86/include/asm/kvm_page_track.h
@@ -46,7 +46,7 @@ struct kvm_page_track_notifier_node {
struct kvm_page_track_notifier_node *node);
};
-void kvm_page_track_init(struct kvm *kvm);
+int kvm_page_track_init(struct kvm *kvm);
void kvm_page_track_cleanup(struct kvm *kvm);
void kvm_page_track_free_memslot(struct kvm_memory_slot *slot);
diff --git a/arch/x86/kvm/mmu/page_track.c b/arch/x86/kvm/mmu/page_track.c
index a9e2e02..859800f 100644
--- a/arch/x86/kvm/mmu/page_track.c
+++ b/arch/x86/kvm/mmu/page_track.c
@@ -170,13 +170,13 @@ void kvm_page_track_cleanup(struct kvm *kvm)
cleanup_srcu_struct(&head->track_srcu);
}
-void kvm_page_track_init(struct kvm *kvm)
+int kvm_page_track_init(struct kvm *kvm)
{
struct kvm_page_track_notifier_head *head;
head = &kvm->arch.track_notifier_head;
- init_srcu_struct(&head->track_srcu);
INIT_HLIST_HEAD(&head->track_notifier_list);
+ return init_srcu_struct(&head->track_srcu);
}
/*
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 86539c1..9a122af 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11143,6 +11143,8 @@ void kvm_arch_free_vm(struct kvm *kvm)
int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
{
+ int ret;
+
if (type)
return -EINVAL;
@@ -11178,7 +11180,9 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
kvm_apicv_init(kvm);
kvm_hv_init_vm(kvm);
- kvm_page_track_init(kvm);
+ ret = kvm_page_track_init(kvm);
+ if (ret)
+ return ret;
kvm_mmu_init_vm(kvm);
kvm_xen_init_vm(kvm);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2] KVM: x86: Handle SRCU initialization failure during page track init
2021-09-03 2:37 [PATCH v2] KVM: x86: Handle SRCU initialization failure during page track init tcs.kernel
@ 2021-09-03 6:59 ` Jarkko Sakkinen
2021-09-03 16:18 ` Sean Christopherson
1 sibling, 0 replies; 4+ messages in thread
From: Jarkko Sakkinen @ 2021-09-03 6:59 UTC (permalink / raw)
To: tcs.kernel, pbonzini, seanjc, vkuznets, wanpengli, jmattson, kvm,
linux-kernel
Cc: Haimin Zhang
On Fri, 2021-09-03 at 10:37 +0800, tcs.kernel@gmail.com wrote:
> From: Haimin Zhang <tcs_kernel@tencent.com>
>
> Check the return of init_srcu_struct(), which can fail due to OOM, when
> initializing the page track mechanism. Lack of checking leads to a NULL
> pointer deref found by a modified syzkaller.
>
> Signed-off-by: Haimin Zhang <tcs_kernel@tencent.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Reported-by: TCS Robot <tcs_robot@tencent.com>
I'd drop reported-by. It's not a person (I guess) and the desc
is self-contained already.
Anyway,
Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
/Jarkko
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] KVM: x86: Handle SRCU initialization failure during page track init
2021-09-03 2:37 [PATCH v2] KVM: x86: Handle SRCU initialization failure during page track init tcs.kernel
2021-09-03 6:59 ` Jarkko Sakkinen
@ 2021-09-03 16:18 ` Sean Christopherson
2021-09-21 17:47 ` Paolo Bonzini
1 sibling, 1 reply; 4+ messages in thread
From: Sean Christopherson @ 2021-09-03 16:18 UTC (permalink / raw)
To: tcs.kernel
Cc: pbonzini, vkuznets, wanpengli, jmattson, kvm, linux-kernel,
jarkko, Haimin Zhang
On Fri, Sep 03, 2021, tcs.kernel@gmail.com wrote:
> From: Haimin Zhang <tcs_kernel@tencent.com>
>
> Check the return of init_srcu_struct(), which can fail due to OOM, when
> initializing the page track mechanism. Lack of checking leads to a NULL
> pointer deref found by a modified syzkaller.
>
> Signed-off-by: Haimin Zhang <tcs_kernel@tencent.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Neither myself nor Vitaly provided an SOB, nor is one needed. Review feedback
can be attributed/noted in the part that git ignores (below the three dashes).
> Reported-by: TCS Robot <tcs_robot@tencent.com>
>
> ---
Notes about version changes, e.g. to document/attribute review feedback, go here.
v2:
- Blah blah blah [Vitaly, Sean]
> arch/x86/include/asm/kvm_page_track.h | 2 +-
> arch/x86/kvm/mmu/page_track.c | 4 ++--
> arch/x86/kvm/x86.c | 6 +++++-
> 3 files changed, 8 insertions(+), 4 deletions(-)
>
...
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 86539c1..9a122af 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11143,6 +11143,8 @@ void kvm_arch_free_vm(struct kvm *kvm)
>
> int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> {
> + int ret;
> +
> if (type)
> return -EINVAL;
>
> @@ -11178,7 +11180,9 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>
> kvm_apicv_init(kvm);
> kvm_hv_init_vm(kvm);
> - kvm_page_track_init(kvm);
> + ret = kvm_page_track_init(kvm);
> + if (ret)
> + return ret;
Before moving forward with a fix, I'd like to get Paolo's input on dropping
track_srcu in favor of kvm->srcu and avoiding this altogheter. Note, Paolo is on
vacation at the moment, so this won't get attention for a week or more.
[*] https://lkml.kernel.org/r/YS5Bn6I6wVEL8wKS@google.com
> kvm_mmu_init_vm(kvm);
> kvm_xen_init_vm(kvm);
>
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] KVM: x86: Handle SRCU initialization failure during page track init
2021-09-03 16:18 ` Sean Christopherson
@ 2021-09-21 17:47 ` Paolo Bonzini
0 siblings, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2021-09-21 17:47 UTC (permalink / raw)
To: Sean Christopherson, tcs.kernel
Cc: vkuznets, wanpengli, jmattson, kvm, linux-kernel, jarkko, Haimin Zhang
On 03/09/21 18:18, Sean Christopherson wrote:
> Before moving forward with a fix, I'd like to get Paolo's input on dropping
> track_srcu in favor of kvm->srcu and avoiding this altogheter. Note, Paolo is on
> vacation at the moment, so this won't get attention for a week or more.
The reason for track_srcu's existence is to avoid complications in
kvm_arch_flush_shadow_memslot, which is called from the _write_ side of
kvm->srcu but is on the _read_ side of track_srcu.
I think this should be fixed easily by taking slots_lock in
kvm_page_track_register_notifier and kvm_page_track_unregister_notifier,
however it's a bit more complicated from the point of view of the lock
hierarchy and possible deadlocks.
So I'm open to patches that drop track_srcu, but for now I applied this
patch.
Paolo
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-09-21 17:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-03 2:37 [PATCH v2] KVM: x86: Handle SRCU initialization failure during page track init tcs.kernel
2021-09-03 6:59 ` Jarkko Sakkinen
2021-09-03 16:18 ` Sean Christopherson
2021-09-21 17:47 ` Paolo Bonzini
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.