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

[PATCH v2 0/2] 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 initialised or
destroyed in parallel with the rest of the guest still being live.

Yes, this is annoying.

This second version takes a different approach at the problem,
plugging the glaring hole we have between vgic creation and private
interrupt allocation.

Although this is more invasive, I'm more confident about this one than
the initial version I posted a week ago.

Alex, I'd very much appreciate your testing on this.

Marc Zyngier (2):
  KVM: arm64: timer: Drop warning on failed interrupt signalling
  KVM: arm64: vgic: Hoist SGI/PPI alloc from vgic_init() to
    kvm_create_vgic()

 arch/arm64/kvm/arch_timer.c     | 16 ++++---
 arch/arm64/kvm/vgic/vgic-init.c | 74 ++++++++++++++++-----------------
 2 files changed, 44 insertions(+), 46 deletions(-)

-- 
2.39.2

[PATCH v2 1/2] 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

[PATCH v2 2/2] KVM: arm64: vgic: Hoist SGI/PPI alloc from vgic_init() to kvm_create_vgic()

[Marc Zyngier]

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.

Reported-by: Alexander Potapenko <glider@google.com>
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 bc7e22ab5d812..775461cf2d2db 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;
 
@@ -180,7 +198,7 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned int nr_spis)
 	return 0;
 }
 
-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;
@@ -218,17 +236,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;
@@ -258,7 +287,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;
 
@@ -295,7 +324,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);
@@ -315,35 +344,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

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

[Oliver Upton]

On Wed, Feb 12, 2025 at 06:25:56PM +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 initialised or
> destroyed in parallel with the rest of the guest still being live.
> 
> Yes, this is annoying.
> 
> This second version takes a different approach at the problem,
> plugging the glaring hole we have between vgic creation and private
> interrupt allocation.
> 
> Although this is more invasive, I'm more confident about this one than
> the initial version I posted a week ago.

Much better place now! Here's to the next pile of syzkaller bugs :)

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

> Alex, I'd very much appreciate your testing on this.

I too would like to see the tires kicked before we pick this up, if it
isn't too much trouble Alex.

> Marc Zyngier (2):
>   KVM: arm64: timer: Drop warning on failed interrupt signalling
>   KVM: arm64: vgic: Hoist SGI/PPI alloc from vgic_init() to
>     kvm_create_vgic()
> 
>  arch/arm64/kvm/arch_timer.c     | 16 ++++---
>  arch/arm64/kvm/vgic/vgic-init.c | 74 ++++++++++++++++-----------------
>  2 files changed, 44 insertions(+), 46 deletions(-)
> 
> -- 
> 2.39.2
> 

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

[Alexander Potapenko]

On Thu, Feb 13, 2025 at 5:59 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> On Wed, Feb 12, 2025 at 06:25:56PM +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 initialised or
> > destroyed in parallel with the rest of the guest still being live.
> >
> > Yes, this is annoying.
> >
> > This second version takes a different approach at the problem,
> > plugging the glaring hole we have between vgic creation and private
> > interrupt allocation.
> >
> > Although this is more invasive, I'm more confident about this one than
> > the initial version I posted a week ago.
>
> Much better place now! Here's to the next pile of syzkaller bugs :)
>
> Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
>
> > Alex, I'd very much appreciate your testing on this.
>
> I too would like to see the tires kicked before we pick this up, if it
> isn't too much trouble Alex.

I am on it, will report back today or tomorrow.

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

[Alexander Potapenko]

On Thu, Feb 13, 2025 at 11:29 AM Alexander Potapenko <glider@google.com> wrote:
>
> On Thu, Feb 13, 2025 at 5:59 AM Oliver Upton <oliver.upton@linux.dev> wrote:
> >
> > On Wed, Feb 12, 2025 at 06:25:56PM +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 initialised or
> > > destroyed in parallel with the rest of the guest still being live.
> > >
> > > Yes, this is annoying.
> > >
> > > This second version takes a different approach at the problem,
> > > plugging the glaring hole we have between vgic creation and private
> > > interrupt allocation.
> > >
> > > Although this is more invasive, I'm more confident about this one than
> > > the initial version I posted a week ago.
> >
> > Much better place now! Here's to the next pile of syzkaller bugs :)
> >
> > Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
> >
> > > Alex, I'd very much appreciate your testing on this.
> >
> > I too would like to see the tires kicked before we pick this up, if it
> > isn't too much trouble Alex.
>
> I am on it, will report back today or tomorrow.

I am seeing the following crashes, do you think these could be related
to your changes?

==================================================================
BUG: KASAN: null-ptr-deref in _raw_spin_lock_irqsave+0xa8/0x174
include/linux/instrumented.h:96
Write of size 4 at addr 0000000000000d20 by task syz.3.8387/5166

CPU: 1 UID: 0 PID: 5166 Comm: syz.3.8387 Not tainted
6.14.0-rc2-00002-g4b305a8c5b85 #159
Hardware name: linux,dummy-virt (DT)
Call trace:
 show_stack+0x2c/0x3c arch/arm64/kernel/stacktrace.c:466 (C)
 __dump_stack lib/dump_stack.c:94 [inline]
 dump_stack_lvl+0x94/0xc0 lib/dump_stack.c:120
 print_report+0xf8/0x7d4 mm/kasan/report.c:492
 kasan_report+0xcc/0x128 mm/kasan/report.c:602
 kasan_check_range+0x264/0x2a4
 __kasan_check_write+0x20/0x30 mm/kasan/shadow.c:37
 _raw_spin_lock_irqsave+0xa8/0x174 include/linux/instrumented.h:96
 kvm_vgic_set_owner+0x15c/0x23c arch/arm64/kvm/vgic/vgic.c:611
 kvm_timer_enable+0x174/0x5b0 arch/arm64/kvm/arch_timer.c:1574
 kvm_arch_vcpu_run_pid_change+0x184/0x28c arch/arm64/kvm/arm.c:824
 kvm_vcpu_ioctl+0xa94/0xba8 virt/kvm/kvm_main.c:4366
 __do_sys_ioctl fs/ioctl.c:51 [inline]
...

Re: [PATCH v2 2/2] KVM: arm64: vgic: Hoist SGI/PPI alloc from vgic_init() to kvm_create_vgic()

[Changyuan Lyu]

Hi Marc,

On Wed, Feb 12, 2025 at 18:25:58 +0000, Marc Zyngier <maz@kernel.org> wrote:
> 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.

I have a concern that this patch might cause an issue if userspace creates
VCPUs *after* a VGIC is created but *before* it has been initialized via
`KVM_DEV_ARM_VGIC_CTRL_INIT`.

For example, consider the following call sequence:

1.  Create VGIC via `KVM_CREATE_DEVICE`.
2.  Create all VCPUs.
3.  Issue `KVM_ARM_VCPU_INIT` to all VCPUs.
4.  Issue `KVM_DEV_ARM_VGIC_CTRL_INIT` to the VGIC.

> With that, we have the invariant that when irqchip_in_kernel() is
> true, all vcpus have their private interrupts.
>
> Reported-by: Alexander Potapenko <glider@google.com>
> 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 bc7e22ab5d812..775461cf2d2db 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> [...]
>
> -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;
> @@ -218,17 +236,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;

`vgic_allocate_private_irqs_locked()` appears to be called in two scenarios:

a) When the VGIC is created, in `kvm_vgic_create()`.
b) When a VCPU is created, in `kvm_vm_ioctl_create_vcpu()`.

For scenario (b), the call path is:

`kvm_vm_ioctl_create_vcpu()`
-> `kvm_arch_vcpu_create()`
  -> `kvm_vgic_vcpu_init()`
    -> `vgic_allocate_private_irqs()`
      -> `vgic_allocate_private_irqs_locked()`

However, since the VCPU has just been created, its `MPIDR_EL1` register
value has not been assigned at this point. The VCPU's `MPIDR_EL1`
register is populated later when `KVM_ARM_VCPU_INIT` is issued, via
the following call path:

`kvm_arch_vcpu_ioctl_vcpu_init()`
-> `kvm_vcpu_set_target()`
  -> `__kvm_vcpu_set_target()`
    -> `kvm_reset_vcpu()`
      -> `kvm_reset_sys_regs()`
        -> `reset_mpidr()`

Therefore, with the call sequence I mentioned at the beginning,
`irq->mpidr` would be assigned an uninitialized value. This suggests that
for `irq->mpidr = kvm_vcpu_get_mpidr_aff(vcpu)` to work correctly, we
must rely on the code path from scenario (a). This would imply that
with this patch, all VCPUs must be created and initialized before the
VGIC is created.

> +		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;
> @@ -258,7 +287,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;
>
> [...]

Please let me know if my understanding is incorrect. Thank you!

Best,
Changyuan