Skip to contentSkip to conversation

KVM: ARM: vgic: plug irq injection race

PATCH New Marc Zyngier 2 participants 4 messages Feb 14, 2014 – Mar 14, 2014

lore
Share & tools
lore

Marc Zyngier

author
#0 lore raw

PATCH 1/1 KVM: ARM: vgic: plug irq injection race

As it stands, nothing prevents userspace from injecting an interrupt before the guest's GIC is actually initialized.

This goes unnoticed so far (as everything is pretty much statically allocated), but ends up exploding in a spectacular way once we switch to a more dynamic allocation (the GIC data structure isn't there yet).

The fix is to test for the "ready" flag in the VGIC distributor before trying to inject the interrupt. Note that in order to avoid breaking userspace, we have to ignore what is essentially an error.

Signed-off-by Marc Zyngier <[email protected]>
---
 virt/kvm/arm/vgic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
virt/kvm/arm/vgic.c +2 -1
index be456ce..d40fe61 100644
@@ -1386,7 +1386,8 @@ out:
 int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
 			bool level)
 {
-	if (vgic_update_irq_state(kvm, cpuid, irq_num, level))
+	if (likely(vgic_initialized(kvm)) &&
+	    vgic_update_irq_state(kvm, cpuid, irq_num, level))
 		vgic_kick_vcpus(kvm);
 
 	return 0;
Signature · 1 line
1.8.3.4
#1 lore raw

PATCH 1/1 KVM: ARM: vgic: plug irq injection race

Quoted 21 lines · Marc Zyngier

As it stands, nothing prevents userspace from injecting an interrupt before the guest's GIC is actually initialized.

This goes unnoticed so far (as everything is pretty much statically allocated), but ends up exploding in a spectacular way once we switch to a more dynamic allocation (the GIC data structure isn't there yet).

The fix is to test for the "ready" flag in the VGIC distributor before trying to inject the interrupt. Note that in order to avoid breaking userspace, we have to ignore what is essentially an error.

Signed-off-by Marc Zyngier <[email protected]>
---
 virt/kvm/arm/vgic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
virt/kvm/arm/vgic.c +0 -0
index be456ce..d40fe61 100644

virt/kvm/arm/vgic.c out: · line 1389

Quoted 5 lines
virt/kvm/arm/vgic.c +1 -1
@@ -1386,7 +1386,8 @@ out:
 int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
 			bool level)
 {
-	if (vgic_update_irq_state(kvm, cpuid, irq_num, level))
+	if (likely(vgic_initialized(kvm)) &&

Do we need a barrier in kvm_vgic_init before setting the kvm->arch.vgic.ready to ensure we observe the correctly initialized values of the irq_spi_cpu field here?

Quoted 7 lines
virt/kvm/arm/vgic.c +1 -0
+	    vgic_update_irq_state(kvm, cpuid, irq_num, level))
 		vgic_kick_vcpus(kvm);
 
 	return 0;
Signature · 1 line
1.8.3.4

Otherwise looks good, nicely spotted!
-Christoffer

author
#2 lore raw

PATCH 1/1 KVM: ARM: vgic: plug irq injection race

Quoted 17 lines · Christoffer Dall
Quoted 16 lines · Marc Zyngier

As it stands, nothing prevents userspace from injecting an interrupt before the guest's GIC is actually initialized.

This goes unnoticed so far (as everything is pretty much statically allocated), but ends up exploding in a spectacular way once we switch to a more dynamic allocation (the GIC data structure isn't there yet).

The fix is to test for the "ready" flag in the VGIC distributor before trying to inject the interrupt. Note that in order to avoid breaking userspace, we have to ignore what is essentially an error.

Signed-off-by Marc Zyngier <[email protected]>
---
 virt/kvm/arm/vgic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Quoted 14 lines

virt/kvm/arm/vgic.c out: · line 1389

Quoted 10 lines
virt/kvm/arm/vgic.c +1 -1
index be456ce..d40fe61 100644
@@ -1386,7 +1386,8 @@ out:
 int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
 			bool level)
 {
-	if (vgic_update_irq_state(kvm, cpuid, irq_num, level))
+	if (likely(vgic_initialized(kvm)) &&

Do we need a barrier in kvm_vgic_init before setting the kvm->arch.vgic.ready to ensure we observe the correctly initialized values of the irq_spi_cpu field here?

Ah, I see. Yes, possibly.

Quoted 9 lines
Quoted 7 lines
virt/kvm/arm/vgic.c +1 -0
+	    vgic_update_irq_state(kvm, cpuid, irq_num, level))
 		vgic_kick_vcpus(kvm);
 
 	return 0;
Signature · 1 line
1.8.3.4

Otherwise looks good, nicely spotted!

I'll respin something early next week.

Cheers,

	M.
Signature · 1 line
Jazz is not dead. It just smells funny.
#3 lore raw

PATCH 1/1 KVM: ARM: vgic: plug irq injection race

Quoted 33 lines · Marc Zyngier
Quoted 17 lines · Christoffer Dall
Quoted 16 lines · Marc Zyngier

As it stands, nothing prevents userspace from injecting an interrupt before the guest's GIC is actually initialized.

This goes unnoticed so far (as everything is pretty much statically allocated), but ends up exploding in a spectacular way once we switch to a more dynamic allocation (the GIC data structure isn't there yet).

The fix is to test for the "ready" flag in the VGIC distributor before trying to inject the interrupt. Note that in order to avoid breaking userspace, we have to ignore what is essentially an error.

Signed-off-by Marc Zyngier <[email protected]>
---
 virt/kvm/arm/vgic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Quoted 14 lines

virt/kvm/arm/vgic.c out: · line 1389

Quoted 10 lines
virt/kvm/arm/vgic.c +1 -1
index be456ce..d40fe61 100644
@@ -1386,7 +1386,8 @@ out:
 int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
 			bool level)
 {
-	if (vgic_update_irq_state(kvm, cpuid, irq_num, level))
+	if (likely(vgic_initialized(kvm)) &&

Do we need a barrier in kvm_vgic_init before setting the kvm->arch.vgic.ready to ensure we observe the correctly initialized values of the irq_spi_cpu field here?

Quoted 2 lines

Ah, I see. Yes, possibly.

You can probably just mode the assignment of ready=true below the critical section and you should be good, right?

Thanks,
-Christoffer