KVM: arm64: Restore the VM's feature bitmap when kvm_setup_vcpu() fails
Fuad Tabba
reviewerPATCH 1/1 KVM: arm64: Restore the VM's feature bitmap when kvm_setup_vcpu() fails
__kvm_vcpu_set_target() copies the requested features into the VM-wide bitmap before kvm_setup_vcpu() runs and doesn't undo it when setup fails, so a rejected KVM_ARM_VCPU_INIT leaves the VM recording features that were never set up. With HAS_EL2 | HAS_EL2_E2H0 on a host without FEAT_NV1, kvm_vcpu_init_nested() returns -EINVAL before it allocates any nested stage-2 MMU, and vcpu_has_nv() is then true with nested_mmus_size == 0; its -ENOMEM paths do the same on a VM's first INIT.
Nothing in the tree loads a vCPU whose init failed, so this is latent. The pending KVM_PRE_FAULT_MEMORY series for arm64 does, and the second such load NULL-dereferences in get_s2_mmu_nested() under mmu_lock.
Setup reads the VM-wide bitmap, so the copy can't be deferred; restore the previous value instead when kvm_setup_vcpu() fails.
---
The series' generic kvm_vcpu_pre_fault_memory() calls vcpu_load() before any arm64 hook, so there is no arm64 check it can pass through first. Reproduced on kvmarm/next plus the series under
QEMU (-cpu max with an Apple M2 MIDR, which has_nv1() denies; kvm-arm.mode=nested): KVM_ARM_VCPU_INIT with HAS_EL2 | HAS_EL2_E2H0 returns -EINVAL, then KVM_PRE_FAULT_MEMORY twice. The first call loads with hw_mmu still the canonical MMU and its vcpu_put() clears hw_mmu; the second takes the !hw_mmu path into get_s2_mmu_nested(), whose search over nested_mmus_size == 0 leaves s2_mmu NULL for the BUG_ON(atomic_read(&s2_mmu->refcnt)):
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000074 Call trace: kvm_vcpu_load_hw_mmu+0x94/0x2c0 (P) kvm_arch_vcpu_load+0x2a8/0x5e8 kvm_vcpu_pre_fault_memory+0xa0/0x1b8 kvm_vcpu_ioctl+0x40c/0x6d0
The thread dies with mmu_lock held for write and an RCU stall in queued_write_lock_slowpath() follows. With the fix both calls return -ENOENT and the host is unaffected. Applies unchanged to v7.3-rc3.
arch/arm64/kvm/arm.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
arch/arm64/kvm/arm.c +8 -1
@@ -1685,6 +1685,7 @@ static int kvm_setup_vcpu(struct kvm_vcpu *vcpu)static int __kvm_vcpu_set_target (struct kvm_vcpu *vcpu,const struct kvm_vcpu_init *init) { +DECLARE_BITMAP (old_features,KVM_VCPU_MAX_FEATURES );unsigned long features = init->features[0 ];struct kvm *kvm = vcpu->kvm;int ret = -EINVAL ;
@@ -1695,11 +1696,17 @@ static int __kvm_vcpu_set_target(struct kvm_vcpu *vcpu, kvm_vcpu_init_changed(vcpu, init)) goto out_unlock; +/* Setup reads the VM-wide bitmap, so undo the copy if setup fails. */ +bitmap_copy (old_features, kvm->arch.vcpu_features, +KVM_VCPU_MAX_FEATURES );bitmap_copy (kvm->arch.vcpu_features, &features,KVM_VCPU_MAX_FEATURES ); ret =kvm_setup_vcpu (vcpu); -if (ret) +if (ret) { +bitmap_copy (kvm->arch.vcpu_features, old_features, +KVM_VCPU_MAX_FEATURES ); goto out_unlock; + }/* Now we know what it is, we can reset it. */ kvm_reset_vcpu (vcpu);
base-commit: 089e4f3c4862ba3f29dff2361caa8084879194fd
Signature · 1 line
2.39.5
Quoted 8 lines · Fuad Tabba
__kvm_vcpu_set_target()copies the requested features into the VM-wide bitmap beforekvm_setup_vcpu()runs and doesn't undo it when setup fails, so a rejectedKVM_ARM_VCPU_INITleaves the VM recording features that were never set up. WithHAS_EL2|HAS_EL2_E2H0on a host withoutFEAT_NV1,kvm_vcpu_init_nested()returns-EINVALbefore it allocates any nested stage-2 MMU, andvcpu_has_nv()is then true withnested_mmus_size== 0; its-ENOMEMpaths do the same on a VM's first INIT.
It seems obviously correct (TM) to undo setting the features on error.
Quoted 3 lines
Nothing in the tree loads a vCPU whose init failed, so this is latent. The pending
KVM_PRE_FAULT_MEMORYseries for arm64 does, and the second
I think this needs clarification :)
The KVM_PRE_FAULT_MEMORY series doesn't do this, or rather it only exposes the ability for the pre-existing generic kvm_vcpu_pre_fault_memory() function to be invoked by setting flags such as to expose pre-faulting as available for arm64.
So something like:
The upcoming series that enables KVM_PRE_FAULT_MEMORY exposes an existing bug in how vCPU initialisation is performed as it allows the generic kvm_vcpu_pre_fault_memory() function to be called which calls vcpu_load() before the vCPU might be initialised.
For that bit.
Quoted 1 line
such load NULL-dereferences in
get_s2_mmu_nested()undermmu_lock.
Also I think this is a bit unclear, esp. the prior reference to 'second', not clear what that refers to. I think you should bring in some of the context you provide under the fold for the patch.
So clarify what you do i.e.:
1. KVM_ARM_VCPU_INIT ioctl with an invalid configuration (HAS_EL2 | HAS_EL2_E2H0) - establishes the invalid kvm->arch.vcpu_features and doesn't clear down.
2. KVM_PRE_FAULT_MEMORY - This call is benign, but it calls vcpu_put() which calls kvm_arch_vcpu_put() and that has:
if (vcpu_has_nv(vcpu)) kvm_vcpu_put_hw_mmu(vcpu);
Since the broken config causes vcpu_has_nv() to return true, incorrectly, and causes:
vcpu->arch.hw_mmu = NULL;
To be run even though the vCPU in fact has the canonical MMU...
3. KVM_PRE_FAULT_MEMORY again - This time on load there's an issue:
kvm_vcpu_pre_fault_memory() -> vcpu_load() -> kvm_arch_vcpu_load()
Which has:
if (vcpu_has_nv(vcpu))
kvm_vcpu_load_hw_mmu(vcpu);
This leads to this path being taken:
if (!vcpu->arch.hw_mmu) {
scoped_guard(write_lock, &vcpu->kvm->mmu_lock)
vcpu->arch.hw_mmu = get_s2_mmu_nested(vcpu);
} And in get_s2_mmu_nested() it searches through the non-existent nested mmus leaving s2_mmu NULL, resulting in a NULL pointer deref on this line:
BUG_ON(atomic_read(&s2_mmu->refcnt)); /* We have struct MMUs to spare */
Quoted 6 lines
Setup reads the VM-wide bitmap, so the copy can't be deferred; restore the previous value instead when
kvm_setup_vcpu()fails.Fixes 1de10b7d13a97 ("KVM: arm64: Get rid of vCPU-scoped feature bitmap")Fixes 427733579744e ("KVM: arm64: Select default PMU in KVM_ARM_VCPU_INIT handler")
Hmm are 2 fixes really needed? It seems to me that commit 427733579744e ("KVM: arm64: Select default PMU in KVM_ARM_VCPU_INIT handler") is the right one because that added a step _after_ setting the bitmap that can go wrong.
Quoted 2 lines
With the comments re: commit message/fixes tag addressed this seems correct and a very good spot thanks! We very much need this for the pre fault functionality.
Quoted 12 lines
---
The series' generickvm_vcpu_pre_fault_memory()callsvcpu_load()before any arm64 hook, so there is no arm64 check it can pass through first. Reproduced on kvmarm/next plus the series under
QEMU (-cpu max with an Apple M2 MIDR, whichhas_nv1()denies; kvm-arm.mode=nested):KVM_ARM_VCPU_INITwithHAS_EL2|HAS_EL2_E2H0returns-EINVAL, thenKVM_PRE_FAULT_MEMORYtwice. The first call loads withhw_mmustill the canonical MMU and itsvcpu_put()clearshw_mmu; the second takes the !hw_mmu path intoget_s2_mmu_nested(), whose search overnested_mmus_size== 0 leavess2_mmuNULL for the BUG_ON(atomic_read(&s2_mmu->refcnt)):
Quoted 6 lines
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000074 Call trace: kvm_vcpu_load_hw_mmu+0x94/0x2c0 (P) kvm_arch_vcpu_load+0x2a8/0x5e8 kvm_vcpu_pre_fault_memory+0xa0/0x1b8 kvm_vcpu_ioctl+0x40c/0x6d0
I also think you should put the stack trace in the commit message, ideally decoded via scripts/decode_stacktrace.sh!
Quoted 40 lines
The thread dies with
mmu_lockheld for write and an RCU stall inqueued_write_lock_slowpath()follows. With the fix both calls return(quoted diff) +0
-1-ENOENT and the host is unaffected. Applies unchanged to v7.3-rc3. arch/arm64/kvm/arm.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)arch/arm64/kvm/arm.c +8
-1@@ -1685,6 +1685,7 @@ static int kvm_setup_vcpu(struct kvm_vcpu *vcpu)static int __kvm_vcpu_set_target (struct kvm_vcpu *vcpu,const struct kvm_vcpu_init *init) { +DECLARE_BITMAP (old_features,KVM_VCPU_MAX_FEATURES );unsigned long features = init->features[0 ];struct kvm *kvm = vcpu->kvm;int ret = -EINVAL ;@@ -1695,11 +1696,17 @@ static int __kvm_vcpu_set_target(struct kvm_vcpu *vcpu, kvm_vcpu_init_changed(vcpu, init)) goto out_unlock; +/* Setup reads the VM-wide bitmap, so undo the copy if setup fails. */ +bitmap_copy (old_features, kvm->arch.vcpu_features, +KVM_VCPU_MAX_FEATURES );bitmap_copy (kvm->arch.vcpu_features, &features,KVM_VCPU_MAX_FEATURES ); ret =kvm_setup_vcpu (vcpu); -if (ret) +if (ret) { +bitmap_copy (kvm->arch.vcpu_features, old_features, +KVM_VCPU_MAX_FEATURES ); goto out_unlock; + }/* Now we know what it is, we can reset it. */ kvm_reset_vcpu (vcpu);
Quoted 4 lines
base-commit:
089e4f3c4862ba3f29dff2361caa8084879194fd--
2.39.5
--
Cheers, Lorenzo
Fuad Tabba
reviewerHi Lorenzo,
On Fri, 18 Sep 2026 17:10:43 +0100, Lorenzo Stoakes (ARM) <[email protected]> wrote:
[...]
Quoted 9 lines
Quoted 2 lines
Nothing in the tree loads a vCPU whose init failed, so this is latent. The pending
KVM_PRE_FAULT_MEMORYseries for arm64 does, and the secondI think this needs clarification :)
The
KVM_PRE_FAULT_MEMORYseries doesn't do this, or rather it only exposes the ability for the pre-existing generickvm_vcpu_pre_fault_memory()function to be invoked by setting flags such as to expose pre-faulting as available for arm64.
Quoted 8 lines
So something like:
The upcoming series that enables
KVM_PRE_FAULT_MEMORYexposes an existing bug in how vCPU initialisation is performed as it allows the generickvm_vcpu_pre_fault_memory()function to be called which callsvcpu_load()before the vCPU might be initialised.For that bit.
That's what "does" was standing in for. I'll say that the series enables the generic kvm_vcpu_pre_fault_memory(), which calls vcpu_load() whether or not the vCPU has been initialised.
Quoted 6 lines
Quoted 2 lines
such load NULL-dereferences in
get_s2_mmu_nested()undermmu_lock.Also I think this is a bit unclear, esp. the prior reference to 'second', not clear what that refers to. I think you should bring in some of the context you provide under the fold for the patch.
[...]
I'll bring the two-ioctl walk up from below the fold: the first KVM_PRE_FAULT_MEMORY's vcpu_put() clearing hw_mmu, then the second's vcpu_load() reaching get_s2_mmu_nested() with no nested MMUs to find.
Quoted 7 lines
Quoted 2 lines
Fixes 1de10b7d13a97 ("KVM: arm64: Get rid of vCPU-scoped feature bitmap")Fixes 427733579744e ("KVM: arm64: Select default PMU in KVM_ARM_VCPU_INIT handler")Hmm are 2 fixes really needed? It seems to me that commit
427733579744e("KVM: arm64: Select default PMU inKVM_ARM_VCPU_INIThandler") is the right one because that added a step _after_ setting the bitmap that can go wrong.
I'd added the first one because it's where the copy into the VM-wide bitmap moved ahead of the reset, but nothing after the copy could fail there. I'll drop it and keep 427733579744e.
[...]
Quoted 5 lines
With the comments re: commit message/fixes tag addressed this seems correct and a very good spot thanks! We very much need this for the pre fault functionality.
Reviewed-by Lorenzo Stoakes (ARM) <[email protected]>
Thanks!
[...]
Quoted 2 lines
I also think you should put the stack trace in the commit message, ideally decoded via
scripts/decode_stacktrace.sh!
Sure, will do.
Thanks,
/fuad