KVM: arm64: Assorted vgic fixes for 6.14
Marc Zyngier
maintainerv2 cover KVM: arm64: Assorted vgic fixes for 6.14
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(-)
Signature · 1 line
2.39.2
Marc Zyngier
maintainerPATCH v2 1/2 KVM: arm64: timer: Drop warning on failed interrupt signalling
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.
--- arch/arm64/kvm/arch_timer.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
arch/arm64/kvm/arch_timer.c +7 -9
@@ -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 */
Signature · 1 line
2.39.2
Marc Zyngier
maintainerPATCH v2 2/2 KVM: arm64: vgic: Hoist SGI/PPI alloc from vgic_init() to kvm_create_vgic()
In reply to Marc ZyngierIf 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.
--- arch/arm64/kvm/vgic/vgic-init.c | 74 ++++++++++++++++----------------- 1 file changed, 37 insertions(+), 37 deletions(-)
arch/arm64/kvm/vgic/vgic-init.c +37 -37
@@ -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
Signature · 1 line
2.39.2
Re: [PATCH v2 2/2] KVM: arm64: vgic: Hoist SGI/PPI alloc from vgic_init() to kvm_create_vgic()
Hi Marc,
Quoted 11 lines · 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.
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.
Quoted 21 lines
With that, we have the invariant that when
irqchip_in_kernel()is true, all vcpus have their private interrupts.Reported-by Alexander Potapenko <[email protected]>Signed-off-by Marc Zyngier <[email protected]>--- arch/arm64/kvm/vgic/vgic-init.c | 74 ++++++++++++++++----------------- 1 file changed, 37 insertions(+), 37 deletions(-)arch/arm64/kvm/vgic/vgic-init.c +1
-1-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;
arch/arm64/kvm/vgic/vgic-init.c static int vgic_allocate_private_irqs_locked(struct kvm_vcpu *vcpu) · line 244
Quoted 9 lines
arch/arm64/kvm/vgic/vgic-init.c +6
-0@@ -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.
Quoted 22 lines
arch/arm64/kvm/vgic/vgic-init.c +7
-2+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;
arch/arm64/kvm/vgic/vgic-init.c int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu) · line 293
Quoted 9 lines
arch/arm64/kvm/vgic/vgic-init.c +1
-1@@ -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
Quoted 4 lines · 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.
Quoted 8 lines
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 :)
Quoted 1 line
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.
Quoted 12 lines
Marc Zyngier (2):
KVM: arm64: timer: Drop warning on failed interrupt signalling
KVM: arm64: vgic: Hoist SGI/PPI alloc fromvgic_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(-)Signature · 1 line
2.39.2
Quoted 15 lines · Oliver Upton
Quoted 4 lines · 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.
Quoted 8 lines
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.
Quoted 8 lines
Much better place now! Here's to the next pile of syzkaller bugs :)
Reviewed-by Oliver Upton <[email protected]>Quoted 1 line
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.
Quoted 19 lines · Alexander Potapenko
Quoted 17 lines · Oliver Upton
Quoted 4 lines · 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.
Quoted 8 lines
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 :)
Quoted 8 lines
Quoted 6 lines
Reviewed-by Oliver Upton <[email protected]>Quoted 1 line
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]
...