All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KVM: arm/arm64: fix reference to uninitialised VGIC
@ 2016-02-03 16:56 ` Andre Przywara
  0 siblings, 0 replies; 7+ messages in thread
From: Andre Przywara @ 2016-02-03 16:56 UTC (permalink / raw)
  To: Marc Zyngier, Christoffer Dall
  Cc: kvmarm, linux-arm-kernel, kvm, Cosmin Gorgovan, stable

Commit 4b4b4512da2a ("arm/arm64: KVM: Rework the arch timer to use
level-triggered semantics") brought the virtual architected timer
closer to the VGIC. There is one occasion were we don't properly
check for the VGIC actually having been initialized before, but
instead go on to check the active state of some IRQ number.
If userland hasn't instantiated a virtual GIC, we end up with a
kernel NULL pointer dereference:
=========
Unable to handle kernel NULL pointer dereference at virtual address 00000000
pgd = ffffffc9745c5000
[00000000] *pgd=00000009f631e003, *pud=00000009f631e003, *pmd=0000000000000000
Internal error: Oops: 96000006 [#2] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 2144 Comm: kvm_simplest-ar Tainted: G      D 4.5.0-rc2+ #1300
Hardware name: ARM Juno development board (r1) (DT)
task: ffffffc976da8000 ti: ffffffc976e28000 task.ti: ffffffc976e28000
PC is at vgic_bitmap_get_irq_val+0x78/0x90
LR is at kvm_vgic_map_is_active+0xac/0xc8
pc : [<ffffffc0000b7e28>] lr : [<ffffffc0000b972c>] pstate: 20000145
....
=========

Fix this by bailing out early of kvm_timer_flush_hwstate() if we don't
have a VGIC at all.

Reported-by: Cosmin Gorgovan <cosmin@linux-geek.org>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Cc: <stable@vger.kernel.org> # 4.4.x
---
 virt/kvm/arm/arch_timer.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 69bca18..ea60646 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -143,7 +143,7 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
  * Check if there was a change in the timer state (should we raise or lower
  * the line level to the GIC).
  */
-static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
+static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
 
@@ -154,10 +154,12 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
 	 * until we call this function from kvm_timer_flush_hwstate.
 	 */
 	if (!vgic_initialized(vcpu->kvm))
-	    return;
+		return -ENODEV;
 
 	if (kvm_timer_should_fire(vcpu) != timer->irq.level)
 		kvm_timer_update_irq(vcpu, !timer->irq.level);
+
+	return 0;
 }
 
 /*
@@ -218,7 +220,8 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 	bool phys_active;
 	int ret;
 
-	kvm_timer_update_state(vcpu);
+	if (kvm_timer_update_state(vcpu))
+		return;
 
 	/*
 	* If we enter the guest with the virtual input level to the VGIC
-- 
2.6.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH] KVM: arm/arm64: fix reference to uninitialised VGIC
@ 2016-02-03 16:56 ` Andre Przywara
  0 siblings, 0 replies; 7+ messages in thread
From: Andre Przywara @ 2016-02-03 16:56 UTC (permalink / raw)
  To: linux-arm-kernel

Commit 4b4b4512da2a ("arm/arm64: KVM: Rework the arch timer to use
level-triggered semantics") brought the virtual architected timer
closer to the VGIC. There is one occasion were we don't properly
check for the VGIC actually having been initialized before, but
instead go on to check the active state of some IRQ number.
If userland hasn't instantiated a virtual GIC, we end up with a
kernel NULL pointer dereference:
=========
Unable to handle kernel NULL pointer dereference at virtual address 00000000
pgd = ffffffc9745c5000
[00000000] *pgd=00000009f631e003, *pud=00000009f631e003, *pmd=0000000000000000
Internal error: Oops: 96000006 [#2] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 2144 Comm: kvm_simplest-ar Tainted: G      D 4.5.0-rc2+ #1300
Hardware name: ARM Juno development board (r1) (DT)
task: ffffffc976da8000 ti: ffffffc976e28000 task.ti: ffffffc976e28000
PC is at vgic_bitmap_get_irq_val+0x78/0x90
LR is at kvm_vgic_map_is_active+0xac/0xc8
pc : [<ffffffc0000b7e28>] lr : [<ffffffc0000b972c>] pstate: 20000145
....
=========

Fix this by bailing out early of kvm_timer_flush_hwstate() if we don't
have a VGIC at all.

Reported-by: Cosmin Gorgovan <cosmin@linux-geek.org>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Cc: <stable@vger.kernel.org> # 4.4.x
---
 virt/kvm/arm/arch_timer.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 69bca18..ea60646 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -143,7 +143,7 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
  * Check if there was a change in the timer state (should we raise or lower
  * the line level to the GIC).
  */
-static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
+static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
 
@@ -154,10 +154,12 @@ static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
 	 * until we call this function from kvm_timer_flush_hwstate.
 	 */
 	if (!vgic_initialized(vcpu->kvm))
-	    return;
+		return -ENODEV;
 
 	if (kvm_timer_should_fire(vcpu) != timer->irq.level)
 		kvm_timer_update_irq(vcpu, !timer->irq.level);
+
+	return 0;
 }
 
 /*
@@ -218,7 +220,8 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
 	bool phys_active;
 	int ret;
 
-	kvm_timer_update_state(vcpu);
+	if (kvm_timer_update_state(vcpu))
+		return;
 
 	/*
 	* If we enter the guest with the virtual input level to the VGIC
-- 
2.6.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] KVM: arm/arm64: fix reference to uninitialised VGIC
  2016-02-03 16:56 ` Andre Przywara
@ 2016-02-03 17:33   ` Marc Zyngier
  -1 siblings, 0 replies; 7+ messages in thread
From: Marc Zyngier @ 2016-02-03 17:33 UTC (permalink / raw)
  To: Andre Przywara, Christoffer Dall
  Cc: kvmarm, linux-arm-kernel, kvm, Cosmin Gorgovan, stable

On 03/02/16 16:56, Andre Przywara wrote:
> Commit 4b4b4512da2a ("arm/arm64: KVM: Rework the arch timer to use
> level-triggered semantics") brought the virtual architected timer
> closer to the VGIC. There is one occasion were we don't properly
> check for the VGIC actually having been initialized before, but
> instead go on to check the active state of some IRQ number.
> If userland hasn't instantiated a virtual GIC, we end up with a
> kernel NULL pointer dereference:
> =========
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> pgd = ffffffc9745c5000
> [00000000] *pgd=00000009f631e003, *pud=00000009f631e003, *pmd=0000000000000000
> Internal error: Oops: 96000006 [#2] PREEMPT SMP
> Modules linked in:
> CPU: 0 PID: 2144 Comm: kvm_simplest-ar Tainted: G      D 4.5.0-rc2+ #1300
> Hardware name: ARM Juno development board (r1) (DT)
> task: ffffffc976da8000 ti: ffffffc976e28000 task.ti: ffffffc976e28000
> PC is at vgic_bitmap_get_irq_val+0x78/0x90
> LR is at kvm_vgic_map_is_active+0xac/0xc8
> pc : [<ffffffc0000b7e28>] lr : [<ffffffc0000b972c>] pstate: 20000145
> ....
> =========
> 
> Fix this by bailing out early of kvm_timer_flush_hwstate() if we don't
> have a VGIC at all.
> 
> Reported-by: Cosmin Gorgovan <cosmin@linux-geek.org>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Cc: <stable@vger.kernel.org> # 4.4.x

Nice catch, thanks.

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] KVM: arm/arm64: fix reference to uninitialised VGIC
@ 2016-02-03 17:33   ` Marc Zyngier
  0 siblings, 0 replies; 7+ messages in thread
From: Marc Zyngier @ 2016-02-03 17:33 UTC (permalink / raw)
  To: linux-arm-kernel

On 03/02/16 16:56, Andre Przywara wrote:
> Commit 4b4b4512da2a ("arm/arm64: KVM: Rework the arch timer to use
> level-triggered semantics") brought the virtual architected timer
> closer to the VGIC. There is one occasion were we don't properly
> check for the VGIC actually having been initialized before, but
> instead go on to check the active state of some IRQ number.
> If userland hasn't instantiated a virtual GIC, we end up with a
> kernel NULL pointer dereference:
> =========
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> pgd = ffffffc9745c5000
> [00000000] *pgd=00000009f631e003, *pud=00000009f631e003, *pmd=0000000000000000
> Internal error: Oops: 96000006 [#2] PREEMPT SMP
> Modules linked in:
> CPU: 0 PID: 2144 Comm: kvm_simplest-ar Tainted: G      D 4.5.0-rc2+ #1300
> Hardware name: ARM Juno development board (r1) (DT)
> task: ffffffc976da8000 ti: ffffffc976e28000 task.ti: ffffffc976e28000
> PC is at vgic_bitmap_get_irq_val+0x78/0x90
> LR is at kvm_vgic_map_is_active+0xac/0xc8
> pc : [<ffffffc0000b7e28>] lr : [<ffffffc0000b972c>] pstate: 20000145
> ....
> =========
> 
> Fix this by bailing out early of kvm_timer_flush_hwstate() if we don't
> have a VGIC at all.
> 
> Reported-by: Cosmin Gorgovan <cosmin@linux-geek.org>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Cc: <stable@vger.kernel.org> # 4.4.x

Nice catch, thanks.

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] KVM: arm/arm64: fix reference to uninitialised VGIC
  2016-02-03 17:33   ` Marc Zyngier
  (?)
@ 2016-02-04  7:36     ` Pavel Fedin
  -1 siblings, 0 replies; 7+ messages in thread
From: Pavel Fedin @ 2016-02-04  7:36 UTC (permalink / raw)
  To: 'Marc Zyngier', 'Andre Przywara',
	'Christoffer Dall'
  Cc: stable, 'Cosmin Gorgovan', kvmarm, linux-arm-kernel, kvm

 Hello!

> > Fix this by bailing out early of kvm_timer_flush_hwstate() if we don't
> > have a VGIC at all.
> >
> > Reported-by: Cosmin Gorgovan <cosmin@linux-geek.org>
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > Cc: <stable@vger.kernel.org> # 4.4.x
> 
> Nice catch, thanks.

 Can you check this: http://www.spinics.net/lists/kvm/msg124540.html ? It also should fix this issue. Just omit the very first hunk
from it, if you don't want to apply the whole set.


Kind regards,
Pavel Fedin
Senior Engineer
Samsung Electronics Research center Russia



^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] KVM: arm/arm64: fix reference to uninitialised VGIC
@ 2016-02-04  7:36     ` Pavel Fedin
  0 siblings, 0 replies; 7+ messages in thread
From: Pavel Fedin @ 2016-02-04  7:36 UTC (permalink / raw)
  To: 'Marc Zyngier', 'Andre Przywara',
	'Christoffer Dall'
  Cc: linux-arm-kernel, 'Cosmin Gorgovan', kvmarm, stable, kvm

 Hello!

> > Fix this by bailing out early of kvm_timer_flush_hwstate() if we don't
> > have a VGIC at all.
> >
> > Reported-by: Cosmin Gorgovan <cosmin@linux-geek.org>
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > Cc: <stable@vger.kernel.org> # 4.4.x
> 
> Nice catch, thanks.

 Can you check this: http://www.spinics.net/lists/kvm/msg124540.html ? It also should fix this issue. Just omit the very first hunk
from it, if you don't want to apply the whole set.


Kind regards,
Pavel Fedin
Senior Engineer
Samsung Electronics Research center Russia

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] KVM: arm/arm64: fix reference to uninitialised VGIC
@ 2016-02-04  7:36     ` Pavel Fedin
  0 siblings, 0 replies; 7+ messages in thread
From: Pavel Fedin @ 2016-02-04  7:36 UTC (permalink / raw)
  To: linux-arm-kernel

 Hello!

> > Fix this by bailing out early of kvm_timer_flush_hwstate() if we don't
> > have a VGIC at all.
> >
> > Reported-by: Cosmin Gorgovan <cosmin@linux-geek.org>
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > Cc: <stable@vger.kernel.org> # 4.4.x
> 
> Nice catch, thanks.

 Can you check this: http://www.spinics.net/lists/kvm/msg124540.html ? It also should fix this issue. Just omit the very first hunk
from it, if you don't want to apply the whole set.


Kind regards,
Pavel Fedin
Senior Engineer
Samsung Electronics Research center Russia

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-02-04  7:36 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-03 16:56 [PATCH] KVM: arm/arm64: fix reference to uninitialised VGIC Andre Przywara
2016-02-03 16:56 ` Andre Przywara
2016-02-03 17:33 ` Marc Zyngier
2016-02-03 17:33   ` Marc Zyngier
2016-02-04  7:36   ` Pavel Fedin
2016-02-04  7:36     ` Pavel Fedin
2016-02-04  7:36     ` Pavel Fedin

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.