From: Haimin Zhang <[email protected]>
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 <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
Signed-off-by: Vitaly Kuznetsov <[email protected]>
Reported-by: TCS Robot <[email protected]>
---
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
On Fri, 2021-09-03 at 10:37 +0800, [email protected] wrote:
> From: Haimin Zhang <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> Signed-off-by: Vitaly Kuznetsov <[email protected]>
> Reported-by: TCS Robot <[email protected]>
I'd drop reported-by. It's not a person (I guess) and the desc
is self-contained already.
Anyway,
Acked-by: Jarkko Sakkinen <[email protected]>
/Jarkko
On Fri, Sep 03, 2021, [email protected] wrote:
> From: Haimin Zhang <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> Signed-off-by: Vitaly Kuznetsov <[email protected]>
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 <[email protected]>
>
> ---
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/[email protected]
> kvm_mmu_init_vm(kvm);
> kvm_xen_init_vm(kvm);
>
> --
> 1.8.3.1
>
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