[PATCH 0/3] KVM: arm64: Assorted vgic fixes for 6.14

[PATCH 0/3] KVM: arm64: Assorted vgic fixes for 6.14

[Marc Zyngier]

Alexander, while fuzzing KVM/arm64, found an annoying set of problems,
all stemming from the fact that the vgic can be destroyed in parallel
with the rest of the guest still being live.

Yes, this is annoying.

Fixing this is not going to happen overnight (though I have some
ideas), but we can make what we have today a bit more robust.

This is what patch #2 is doing. Patch #1 is just removing a loud
WARN_ON() that serves little purpose, and patch #3 fixes the actual
bug that Alex reported.

Hopefully, none of that is controversial...

Marc Zyngier (3):
  KVM: arm64: timer: Drop warning on failed interrupt signalling
  KVM: arm64: vgic: Check for unallocated PPI/SPI arrays
  KVM: arm64: vgic: Gracefully handle resetting an unallocated interrupt

 arch/arm64/kvm/arch_timer.c | 16 +++++++---------
 arch/arm64/kvm/vgic/vgic.c  |  7 +++++++
 2 files changed, 14 insertions(+), 9 deletions(-)

-- 
2.39.2

[PATCH 1/3] KVM: arm64: timer: Drop warning on failed interrupt signalling

[Marc Zyngier]

We currently spit out a warning if making a timer interrupt pending
fails. But not only this is loud and easy to trigger from userspace,
we also fail to do anything useful with that information.

Dropping the warning is the easiest thing to do for now. We can
always add error reporting if we really want in the future.

Reported-by: Alexander Potapenko <glider@google.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/arch_timer.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 231c0cd9c7b4b..70802e4c91cf5 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -447,21 +447,19 @@ static void kvm_timer_update_status(struct arch_timer_context *ctx, bool level)
 static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
 				 struct arch_timer_context *timer_ctx)
 {
-	int ret;
-
 	kvm_timer_update_status(timer_ctx, new_level);
 
 	timer_ctx->irq.level = new_level;
 	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_irq(timer_ctx),
 				   timer_ctx->irq.level);
 
-	if (!userspace_irqchip(vcpu->kvm)) {
-		ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu,
-					  timer_irq(timer_ctx),
-					  timer_ctx->irq.level,
-					  timer_ctx);
-		WARN_ON(ret);
-	}
+	if (userspace_irqchip(vcpu->kvm))
+		return;
+
+	kvm_vgic_inject_irq(vcpu->kvm, vcpu,
+			    timer_irq(timer_ctx),
+			    timer_ctx->irq.level,
+			    timer_ctx);
 }
 
 /* Only called for a fully emulated timer */
-- 
2.39.2

Re: [PATCH 1/3] KVM: arm64: timer: Drop warning on failed interrupt signalling

[Alexander Potapenko]

On Thu, Feb 6, 2025 at 4:21 PM Marc Zyngier <maz@kernel.org> wrote:
>
> We currently spit out a warning if making a timer interrupt pending
> fails. But not only this is loud and easy to trigger from userspace,
> we also fail to do anything useful with that information.
>
> Dropping the warning is the easiest thing to do for now. We can
> always add error reporting if we really want in the future.
>
> Reported-by: Alexander Potapenko <glider@google.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
Tested-by: Alexander Potapenko <glider@google.com>

Thanks!

[PATCH 2/3] KVM: arm64: vgic: Check for unallocated PPI/SPI arrays

[Marc Zyngier]

Alexander's fuzzing has exhibited a large variety of races that
all end-up with taking the address of a PPI or SPI structure while
the vgic was torn down (because nuking it is only an ioctl() away,
and syzkaller is amazing at finding holes).

In order to preserve some sanity, always evaluate whether the array
containing the PPI/SPI is allocated.

Suggested-by: Alexander Potapenko <glider@google.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/vgic/vgic.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index cc8c6b9b5dd8b..f454cef59e24b 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -89,6 +89,8 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, u32 intid)
 	/* SPIs */
 	if (intid >= VGIC_NR_PRIVATE_IRQS &&
 	    intid < (kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS)) {
+		if (unlikely(!kvm->arch.vgic.spis))
+			return NULL;
 		intid = array_index_nospec(intid, kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS);
 		return &kvm->arch.vgic.spis[intid - VGIC_NR_PRIVATE_IRQS];
 	}
@@ -107,6 +109,8 @@ struct vgic_irq *vgic_get_vcpu_irq(struct kvm_vcpu *vcpu, u32 intid)
 
 	/* SGIs and PPIs */
 	if (intid < VGIC_NR_PRIVATE_IRQS) {
+		if (unlikely(!vcpu->arch.vgic_cpu.private_irqs))
+			return NULL;
 		intid = array_index_nospec(intid, VGIC_NR_PRIVATE_IRQS);
 		return &vcpu->arch.vgic_cpu.private_irqs[intid];
 	}
-- 
2.39.2

Re: [PATCH 2/3] KVM: arm64: vgic: Check for unallocated PPI/SPI arrays

[Alexander Potapenko]

On Thu, Feb 6, 2025 at 4:21 PM Marc Zyngier <maz@kernel.org> wrote:
>
> Alexander's fuzzing has exhibited a large variety of races that
> all end-up with taking the address of a PPI or SPI structure while
> the vgic was torn down (because nuking it is only an ioctl() away,
> and syzkaller is amazing at finding holes).
>
> In order to preserve some sanity, always evaluate whether the array
> containing the PPI/SPI is allocated.
>
> Suggested-by: Alexander Potapenko <glider@google.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
Tested-by: Alexander Potapenko <glider@google.com>

[PATCH 3/3] KVM: arm64: vgic: Gracefully handle resetting an unallocated interrupt

[Marc Zyngier]

Playing with racing vcpu reset and vgic teardown makes it relatively
easy to trigger a case where, by the time we try to reset a mapped
interrupt such as a timer's, the vgic is gone and there is no
interrupt to play with.

Check for NULL upfront to avoid further embarassement.

Reported-by: Alexander Potapenko <glider@google.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/vgic/vgic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index f454cef59e24b..2ea6d1d1d3091 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -542,6 +542,9 @@ void kvm_vgic_reset_mapped_irq(struct kvm_vcpu *vcpu, u32 vintid)
 	struct vgic_irq *irq = vgic_get_vcpu_irq(vcpu, vintid);
 	unsigned long flags;
 
+	if (!irq)
+		return;
+
 	if (!irq->hw)
 		goto out;
 
-- 
2.39.2

Re: [PATCH 3/3] KVM: arm64: vgic: Gracefully handle resetting an unallocated interrupt

[Alexander Potapenko]

On Thu, Feb 6, 2025 at 4:21 PM Marc Zyngier <maz@kernel.org> wrote:
>
> Playing with racing vcpu reset and vgic teardown makes it relatively
> easy to trigger a case where, by the time we try to reset a mapped
> interrupt such as a timer's, the vgic is gone and there is no
> interrupt to play with.
>
> Check for NULL upfront to avoid further embarassement.
>
> Reported-by: Alexander Potapenko <glider@google.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
Tested-by: Alexander Potapenko <glider@google.com>

Re: [PATCH 0/3] KVM: arm64: Assorted vgic fixes for 6.14

[Oliver Upton]

On Thu, Feb 06, 2025 at 03:20:57PM +0000, Marc Zyngier wrote:
> Alexander, while fuzzing KVM/arm64, found an annoying set of problems,
> all stemming from the fact that the vgic can be destroyed in parallel
> with the rest of the guest still being live.
> 
> Yes, this is annoying.
> 
> Fixing this is not going to happen overnight (though I have some
> ideas), but we can make what we have today a bit more robust.
> 
> This is what patch #2 is doing. Patch #1 is just removing a loud
> WARN_ON() that serves little purpose, and patch #3 fixes the actual
> bug that Alex reported.
> 
> Hopefully, none of that is controversial...

I'm a bit grumbly about slapping bandaids on the problem, but given the
fact that glider reported all of this a while ago and we still haven't
fixed it is enough to justify these patches. So:

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

-- 
Thanks,
Oliver

Re: [PATCH 0/3] KVM: arm64: Assorted vgic fixes for 6.14

[Marc Zyngier]

On Fri, 07 Feb 2025 18:03:55 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Thu, Feb 06, 2025 at 03:20:57PM +0000, Marc Zyngier wrote:
> > Alexander, while fuzzing KVM/arm64, found an annoying set of problems,
> > all stemming from the fact that the vgic can be destroyed in parallel
> > with the rest of the guest still being live.
> > 
> > Yes, this is annoying.
> > 
> > Fixing this is not going to happen overnight (though I have some
> > ideas), but we can make what we have today a bit more robust.
> > 
> > This is what patch #2 is doing. Patch #1 is just removing a loud
> > WARN_ON() that serves little purpose, and patch #3 fixes the actual
> > bug that Alex reported.
> > 
> > Hopefully, none of that is controversial...
> 
> I'm a bit grumbly about slapping bandaids on the problem, but given the
> fact that glider reported all of this a while ago and we still haven't
> fixed it is enough to justify these patches. So:

Yeah, same here. I'm starting to think that we need to either prevent
the vgic from being asynchronously destroyed, or start refcounting all
IRQs just like LPIs. Which is very annoying since we don't have a
global namespace for SGIs and PPIs.

But maybe simply refcounting the vgic itself would be enough.
Thoughts?

> Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/3] KVM: arm64: Assorted vgic fixes for 6.14

[Oliver Upton]

On Fri, Feb 07, 2025 at 06:10:49PM +0000, Marc Zyngier wrote:
> On Fri, 07 Feb 2025 18:03:55 +0000,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > On Thu, Feb 06, 2025 at 03:20:57PM +0000, Marc Zyngier wrote:
> > > Alexander, while fuzzing KVM/arm64, found an annoying set of problems,
> > > all stemming from the fact that the vgic can be destroyed in parallel
> > > with the rest of the guest still being live.
> > > 
> > > Yes, this is annoying.
> > > 
> > > Fixing this is not going to happen overnight (though I have some
> > > ideas), but we can make what we have today a bit more robust.
> > > 
> > > This is what patch #2 is doing. Patch #1 is just removing a loud
> > > WARN_ON() that serves little purpose, and patch #3 fixes the actual
> > > bug that Alex reported.
> > > 
> > > Hopefully, none of that is controversial...
> > 
> > I'm a bit grumbly about slapping bandaids on the problem, but given the
> > fact that glider reported all of this a while ago and we still haven't
> > fixed it is enough to justify these patches. So:
> 
> Yeah, same here. I'm starting to think that we need to either prevent
> the vgic from being asynchronously destroyed, or start refcounting all
> IRQs just like LPIs. Which is very annoying since we don't have a
> global namespace for SGIs and PPIs.
> 
> But maybe simply refcounting the vgic itself would be enough.
> Thoughts?

So would we refcount on the owning structure for a particular IRQ? i.e.
private IRQs are counted against the owning vCPU and SPIs against the
distributor?

Adding a vgic_put_vcpu_irq() could help disambiguate private IRQs too.

-- 
Thanks,
Oliver

Re: [PATCH 0/3] KVM: arm64: Assorted vgic fixes for 6.14

[Marc Zyngier]

On Fri, 07 Feb 2025 18:50:32 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Fri, Feb 07, 2025 at 06:10:49PM +0000, Marc Zyngier wrote:
> > On Fri, 07 Feb 2025 18:03:55 +0000,
> > Oliver Upton <oliver.upton@linux.dev> wrote:
> > > 
> > > On Thu, Feb 06, 2025 at 03:20:57PM +0000, Marc Zyngier wrote:
> > > > Alexander, while fuzzing KVM/arm64, found an annoying set of problems,
> > > > all stemming from the fact that the vgic can be destroyed in parallel
> > > > with the rest of the guest still being live.
> > > > 
> > > > Yes, this is annoying.
> > > > 
> > > > Fixing this is not going to happen overnight (though I have some
> > > > ideas), but we can make what we have today a bit more robust.
> > > > 
> > > > This is what patch #2 is doing. Patch #1 is just removing a loud
> > > > WARN_ON() that serves little purpose, and patch #3 fixes the actual
> > > > bug that Alex reported.
> > > > 
> > > > Hopefully, none of that is controversial...
> > > 
> > > I'm a bit grumbly about slapping bandaids on the problem, but given the
> > > fact that glider reported all of this a while ago and we still haven't
> > > fixed it is enough to justify these patches. So:
> > 
> > Yeah, same here. I'm starting to think that we need to either prevent
> > the vgic from being asynchronously destroyed, or start refcounting all
> > IRQs just like LPIs. Which is very annoying since we don't have a
> > global namespace for SGIs and PPIs.
> > 
> > But maybe simply refcounting the vgic itself would be enough.
> > Thoughts?
> 
> So would we refcount on the owning structure for a particular IRQ? i.e.
> private IRQs are counted against the owning vCPU and SPIs against the
> distributor?
> 
> Adding a vgic_put_vcpu_irq() could help disambiguate private IRQs too.

Having slept (yay!) on it, I don't think this is enough.

The observation is that a lot of the issues we have are triggered by
the private interrupt array being allocated on demand on VGIC
creation. We had way fewer problems when the private interrupts were
directly part of struct vcpu, which really was hiding the basic
problem.

We have an annoying discrepancy between knowing that a GIC is present
(VM-wide state) and the interrupts for a vcpu being allocated (vcpu
state). This is what Alexander's fuzzing is all about (create a vcpu,
init it, and while this is happening, create/destroy a vgic).

So it isn't just destruction, but also creation that is problematic,
and I'm not sure that refcounting is the correct solution anymore.

Now, for the issue that this series is trying to fix, I came up with
an alternative approach, based on the above epiphany. We currently
allocate private interrupts in two locations:

- at vcpu creation time *if* a vgic has already been created

- from vgic_init(), which is itself called on:

  - KVM_DEV_ARM_VGIC_CTRL_INIT for the modern stuff,

  - or lazily for anything old and specific to GICv2 (huh).

Crucially, there is a gaping window between the creation of a vgic
*after* a vcpu has been created, and its initialisation. Anything that
checks for irqchip_in_kernel() during that interval may assume that
private interrupts are allocated, because this is what it used to be.
Waiting for vgic_init() to mop-up the "early" vcpus is what creates
the issue.

Given that, the solution is obvious: hoist the early vcpu mop-up from
vgic_init() into kvm_vgic_create(). This guarantees that any vcpu
observing irqchip_in_kernel() being true will also see its private
interrupts.

The patch below fixes it for me (tested with kvmtool, qemu, and Alex's
tickling reproducer).

Thoughts?

	M.

From c150c2300d5b8918909ad46a769aae864a58025f Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Sat, 8 Feb 2025 14:58:43 +0000
Subject: [PATCH] KVM: arm64: vgic: Hoist SGI/PPI alloc from vgic_init() to
 kvm_create_vgic()

If userspace creates vcpus, then a vgic, we end-up in a situation
where irqchip_in_kernel() will return true, but no private interrupt
has been allocated for these vcpus. This situation will continue
until userspace initialises the vgic, at which point we fix the
early vcpus. Should a vcpu run or be initialised in the interval,
bad things may happen.

An obvious solution is to move this fix-up phase to the point where
the vgic is created. This ensures that from that point onwards,
all vcpus have their private interrupts, as new vcpus will directly
allocate them.

With that, we have the invariant that when irqchip_in_kernel() is
true, all vcpus have their private interrupts.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/vgic/vgic-init.c | 74 ++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index c7a82bd0c276d..1f33e71c2a731 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -34,9 +34,9 @@
  *
  * CPU Interface:
  *
- * - kvm_vgic_vcpu_init(): initialization of static data that
- *   doesn't depend on any sizing information or emulation type. No
- *   allocation is allowed there.
+ * - kvm_vgic_vcpu_init(): initialization of static data that doesn't depend
+ *   on any sizing information. Private interrupts are allocated if not
+ *   already allocated at vgic-creation time.
  */
 
 /* EARLY INIT */
@@ -58,6 +58,8 @@ void kvm_vgic_early_init(struct kvm *kvm)
 
 /* CREATION */
 
+static int vgic_allocate_private_irqs_locked(struct kvm_vcpu *vcpu, u32 type);
+
 /**
  * kvm_vgic_create: triggered by the instantiation of the VGIC device by
  * user space, either through the legacy KVM_CREATE_IRQCHIP ioctl (v2 only)
@@ -112,6 +114,22 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
 		goto out_unlock;
 	}
 
+	kvm_for_each_vcpu(i, vcpu, kvm) {
+		ret = vgic_allocate_private_irqs_locked(vcpu, type);
+		if (ret)
+			break;
+	}
+
+	if (ret) {
+		kvm_for_each_vcpu(i, vcpu, kvm) {
+			struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
+			kfree(vgic_cpu->private_irqs);
+			vgic_cpu->private_irqs = NULL;
+		}
+
+		goto out_unlock;
+	}
+
 	kvm->arch.vgic.in_kernel = true;
 	kvm->arch.vgic.vgic_model = type;
 
@@ -201,7 +219,7 @@ int kvm_vgic_vcpu_nv_init(struct kvm_vcpu *vcpu)
 	return ret;
 }
 
-static int vgic_allocate_private_irqs_locked(struct kvm_vcpu *vcpu)
+static int vgic_allocate_private_irqs_locked(struct kvm_vcpu *vcpu, u32 type)
 {
 	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	int i;
@@ -239,17 +257,28 @@ static int vgic_allocate_private_irqs_locked(struct kvm_vcpu *vcpu)
 			/* PPIs */
 			irq->config = VGIC_CONFIG_LEVEL;
 		}
+
+		switch (type) {
+		case KVM_DEV_TYPE_ARM_VGIC_V3:
+			irq->group = 1;
+			irq->mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
+			break;
+		case KVM_DEV_TYPE_ARM_VGIC_V2:
+			irq->group = 0;
+			irq->targets = BIT(vcpu->vcpu_id);
+			break;
+		}
 	}
 
 	return 0;
 }
 
-static int vgic_allocate_private_irqs(struct kvm_vcpu *vcpu)
+static int vgic_allocate_private_irqs(struct kvm_vcpu *vcpu, u32 type)
 {
 	int ret;
 
 	mutex_lock(&vcpu->kvm->arch.config_lock);
-	ret = vgic_allocate_private_irqs_locked(vcpu);
+	ret = vgic_allocate_private_irqs_locked(vcpu, type);
 	mutex_unlock(&vcpu->kvm->arch.config_lock);
 
 	return ret;
@@ -279,7 +308,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
 	if (!irqchip_in_kernel(vcpu->kvm))
 		return 0;
 
-	ret = vgic_allocate_private_irqs(vcpu);
+	ret = vgic_allocate_private_irqs(vcpu, dist->vgic_model);
 	if (ret)
 		return ret;
 
@@ -316,7 +345,7 @@ int vgic_init(struct kvm *kvm)
 {
 	struct vgic_dist *dist = &kvm->arch.vgic;
 	struct kvm_vcpu *vcpu;
-	int ret = 0, i;
+	int ret = 0;
 	unsigned long idx;
 
 	lockdep_assert_held(&kvm->arch.config_lock);
@@ -336,35 +365,6 @@ int vgic_init(struct kvm *kvm)
 	if (ret)
 		goto out;
 
-	/* Initialize groups on CPUs created before the VGIC type was known */
-	kvm_for_each_vcpu(idx, vcpu, kvm) {
-		ret = vgic_allocate_private_irqs_locked(vcpu);
-		if (ret)
-			goto out;
-
-		for (i = 0; i < VGIC_NR_PRIVATE_IRQS; i++) {
-			struct vgic_irq *irq = vgic_get_vcpu_irq(vcpu, i);
-
-			switch (dist->vgic_model) {
-			case KVM_DEV_TYPE_ARM_VGIC_V3:
-				irq->group = 1;
-				irq->mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
-				break;
-			case KVM_DEV_TYPE_ARM_VGIC_V2:
-				irq->group = 0;
-				irq->targets = 1U << idx;
-				break;
-			default:
-				ret = -EINVAL;
-			}
-
-			vgic_put_irq(kvm, irq);
-
-			if (ret)
-				goto out;
-		}
-	}
-
 	/*
 	 * If we have GICv4.1 enabled, unconditionally request enable the
 	 * v4 support so that we get HW-accelerated vSGIs. Otherwise, only
-- 
2.39.2


-- 
Without deviation from the norm, progress is not possible.