KVM: PPC: BE guest-exit fixes and nestedv2 vCPU limit
Subsystems (3)
Ritesh Harjani (IBM)
reviewercover KVM: PPC: BE guest-exit fixes and nestedv2 vCPU limit
These issues were identified while adding kvm powerpc selftests infrastructure support [1]. Patch-1 and Patch-2 were identified while running the selftests on Qemu+TCG+powernv+BE, while Patch-3 can also be reproduced on a real pseries LPAR with nestedv2 capabilities.
[1]: https://lore.kernel.org/linuxppc-dev/[email protected]/
Ritesh Harjani (IBM) (3):
KVM: PPC: Book3S HV: Read EX_DSISR as a 32-bit field on guest exit
KVM: PPC: Don't call __ffs() on an empty pending_exceptions bitmap
KVM: PPC: Book3S HV nestedv2: cap max vcpus to PAPR limit
arch/powerpc/include/asm/kvm_host.h | 1 + arch/powerpc/kvm/book3s.c | 22 ++++++++++++---------- arch/powerpc/kvm/book3s_hv_p9_entry.c | 13 +++++++++++-- arch/powerpc/kvm/powerpc.c | 6 ++++++ 4 files changed, 30 insertions(+), 12 deletions(-)
--
2.39.5
Ritesh Harjani (IBM)
reviewerRFC 1/3 KVM: PPC: Book3S HV: Read EX_DSISR as a 32-bit field on guest exit
kvmhv_vcpu_entry_p9() recovers the guest fault state from the paca exception save area with
vcpu->arch.fault_dsisr = exsave[EX_DSISR/sizeof(u64)];
but EX_DSISR is not a doubleword. It is a 32-bit field at offset 48 that shares a doubleword with EX_CCR at offset 52 and the exception entry in exceptions-64s.S stores it accordingly. For e.g.
stw r10,IAREA+EX_DSISR(r13)
This issue was caught when running kvm selftests on big-endian, where this read would land in the high half and the truncation yields EX_CCR instead - KVM acts on the guest's condition register as if it were HDSISR.
The result is that every HDSI on a big-endian host is evaluated against a bogus fault status i.e. it fails with below error:
KVM: Got radix HV page fault with DSISR=84004840
Let's fix this by reading this field at it's natural width.
--- arch/powerpc/kvm/book3s_hv_p9_entry.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
arch/powerpc/kvm/book3s_hv_p9_entry.c +11 -2
@@ -529,6 +529,15 @@ unsigned long kvmppc_msr_hard_disable_set_facilities(struct kvm_vcpu *vcpu, unsi }EXPORT_SYMBOL_GPL (kvmppc_msr_hard_disable_set_facilities); +/* +* EX_DSISR is a 32-bit field that shares a doubleword with EX_CCR. +* Small helper for reading it back at its natural width. +*/ +static u32 exsave_dsisr (u64 *exsave) +{ +return *(u32 *)((void *)exsave +EX_DSISR ); +} +int kvmhv_vcpu_entry_p9 (struct kvm_vcpu *vcpu,u64 time_limit,unsigned long lpcr,u64 *tb) {struct p9_host_os_sprs host_os_sprs;
@@ -770,7 +779,7 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpcif (unlikely (trap ==BOOK3S_INTERRUPT_MACHINE_CHECK )) { vcpu->arch.fault_dar = exsave[EX_DAR /sizeof (u64)]; - vcpu->arch.fault_dsisr = exsave[EX_DSISR /sizeof (u64)]; + vcpu->arch.fault_dsisr =exsave_dsisr (exsave);kvmppc_realmode_machine_check (vcpu); }else if (unlikely (trap ==BOOK3S_INTERRUPT_HMI )) {
@@ -781,7 +790,7 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc }else if (trap ==BOOK3S_INTERRUPT_H_DATA_STORAGE ) { vcpu->arch.fault_dar = exsave[EX_DAR /sizeof (u64)]; - vcpu->arch.fault_dsisr = exsave[EX_DSISR /sizeof (u64)]; + vcpu->arch.fault_dsisr =exsave_dsisr (exsave); vcpu->arch.fault_gpa =mfspr (SPRN_ASDR ); }else if (trap ==BOOK3S_INTERRUPT_H_INST_STORAGE ) {
Signature · 1 line
2.39.5
Ritesh Harjani (IBM)
reviewerRFC 2/3 KVM: PPC: Book3S: Don't call __ffs() on an empty pending_exceptions bitmap
In reply to Ritesh Harjani (IBM)This problem was caught when running KVM PPC selftests on big-endian. __ffs() doc clearly says:
"Undefined if no bit exists, so code should check against 0 first"
This cause KVM to deliever an arbitary interrupt to the guest and was causing guest to hang up while running these selftests.
--- arch/powerpc/kvm/book3s.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
arch/powerpc/kvm/book3s.c +12 -10
@@ -401,17 +401,19 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)if (vcpu->arch.pending_exceptions)printk (KERN_EMERG "KVM: Check pending: %lx\n" , vcpu->arch.pending_exceptions);#endif - priority =__ffs (*pending); -while (priority <BOOK3S_IRQPRIO_MAX ) { -if (kvmppc_book3s_irqprio_deliver (vcpu, priority) && -clear_irqprio (vcpu, priority)) { -clear_bit (priority, &vcpu->arch.pending_exceptions); -break ; - } +if (*pending) { + priority =__ffs (*pending); +while (priority <BOOK3S_IRQPRIO_MAX ) { +if (kvmppc_book3s_irqprio_deliver (vcpu, priority) && +clear_irqprio (vcpu, priority)) { +clear_bit (priority, &vcpu->arch.pending_exceptions); +break ; + } - priority =find_next_bit (pending, -BITS_PER_BYTE *sizeof (*pending), - priority +1 ); + priority =find_next_bit (pending, +BITS_PER_BYTE *sizeof (*pending), + priority +1 ); + } }/* Tell the guest about our interrupt status */
--
2.39.5
[email protected]
AI reviewRe: [RFC 2/3] KVM: PPC: Book3S: Don't call __ffs() on an empty pending_exceptions bitmap
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] The patch introduces a Time-of-Check to Time-of-Use (TOCTOU) vulnerability by reading *pending twice without READ_ONCE(), failing to fully prevent the undefined behavior it intends to fix.
Pre-existing issues:
- [High] The exact same __ffs(0) undefined behavior bug exists in the booke.c architecture variant but was omitted from this fix. --
commit 2aea5bb81e74db6e15dcd388923821301dd8cce8
Author: Ritesh Harjani (IBM) <[email protected]>
KVM: PPC: Book3S: Don't call __ffs() on an empty pending_exceptions bitmap
This patch adds a check to ensure the pending_exceptions bitmap is not empty before calling __ffs() in the Book3S KVM PPC code. This prevents undefined behavior that previously caused arbitrary interrupt delivery and guest hangs.
Quoted 7 lines
arch/powerpc/kvm/book3s.c +0
-0@@ -401,17 +401,19 @@ int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu)if (vcpu->arch.pending_exceptions)printk (KERN_EMERG "KVM: Check pending: %lx\n" , vcpu->arch.pending_exceptions);
arch/powerpc/kvm/book3s.c int kvmppc_core_prepare_to_enter(struct kvm_vcpu *vcpu) · line 405
Quoted 10 lines
arch/powerpc/kvm/book3s.c +2
-7#endif - priority =__ffs (*pending); -while (priority <BOOK3S_IRQPRIO_MAX ) { -if (kvmppc_book3s_irqprio_deliver (vcpu, priority) && -clear_irqprio (vcpu, priority)) { -clear_bit (priority, &vcpu->arch.pending_exceptions); -break ; - } +if (*pending) { + priority =__ffs (*pending);
[Severity: High]
Does this read *pending twice without READ_ONCE() and introduce a Time-of-Check to Time-of-Use (TOCTOU) vulnerability?
The KVM_INTERRUPT ioctl can be invoked concurrently by a host userspace thread. It calls kvm_arch_vcpu_unlocked_ioctl(), which does not hold vcpu->mutex, and proceeds to kvmppc_core_dequeue_external() where it clears bits in vcpu->arch.pending_exceptions locklessly using the atomic clear_bit().
Since the compiler can emit two separate memory loads for *pending, if the last pending bit is cleared by a concurrent KVM_INTERRUPT ioctl between the if (*pending) check and the __ffs(*pending) evaluation, could the second load read 0 and still trigger the __ffs(0) undefined behavior this patch intends to fix?
[Severity: High]
This isn't a bug introduced by this patch, but does the same __ffs(0) undefined behavior still exist in the BookE architecture variant?
Looking at kvmppc_core_check_exceptions() in arch/powerpc/kvm/booke.c:
arch/powerpc/kvm/booke.c:kvmppc_core_check_exceptions() {
...
priority = __ffs(*pending);
while (priority < BOOKE_IRQPRIO_MAX) {
...
}This is called unconditionally by kvmppc_core_prepare_to_enter() on every VM entry:
arch/powerpc/kvm/booke.c:kvmppc_core_prepare_to_enter() {
...
kvmppc_core_check_exceptions(vcpu);
...
}Since there is no check against 0, it appears __ffs(0) is executed on every guest entry when no exceptions are pending on BookE guests. Should a similar fix be applied to booke.c as well?
Quoted 17 lines
arch/powerpc/kvm/book3s.c +10
-3+while (priority <BOOK3S_IRQPRIO_MAX ) { +if (kvmppc_book3s_irqprio_deliver (vcpu, priority) && +clear_irqprio (vcpu, priority)) { +clear_bit (priority, &vcpu->arch.pending_exceptions); +break ; + } - priority =find_next_bit (pending, -BITS_PER_BYTE *sizeof (*pending), - priority +1 ); + priority =find_next_bit (pending, +BITS_PER_BYTE *sizeof (*pending), + priority +1 ); + } }/* Tell the guest about our interrupt status */
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Ritesh Harjani (IBM)
reviewerRFC 3/3 KVM: PPC: Book3S HV nestedv2: cap max vcpus to PAPR limit
In reply to Ritesh Harjani (IBM)PAPR spec for H_GUEST_CREATE_VCPU says:
"Only 2048 VCPUs can be created, and the specified ID must be in the range of 0
to 2047"
However, KVM reports KVM_CAP_MAX_VCPUS as NR_CPUS and KVM_CAP_MAX_VCPU_ID as 8 * NR_CPUS. It also passes the userspace vCPU id through to the hcall unchanged. On a nestedv2 host (KVM on PowerVM / pseries nested PAPR) built with NR_CPUS > 2048, kvm selftest e.g. kvm_create_max_vcpus will create vCPU 2048 and fail with EINVAL when the hypervisor returns H_P3.
Following error gets reported (with a debug print added to selftest to print failed vcpu_id number):
./kvm_create_max_vcpus
Random seed: 0x6ec3f539
KVM_CAP_MAX_VCPU_ID: 65536
KVM_CAP_MAX_VCPUS: 8192
Testing creating 8192 vCPUs, with IDs 0...8191.
[ 62.655458][ T302] KVM: Create Guest vcpu hcall failed, rc=-56
[ 62.655458][ T302] KVM: Create Guest vcpu hcall failed, rc=-56
Failed KVM_CREATE_VCPU for vcpu_id 2048 with errno 22
==== Test Assertion Failure ====
lib/kvm_util.c:1395: vcpu->fd >= 0
pid=302 tid=302 errno=22 - Invalid argument
1 0x00000000100061d3: __vm_vcpu_add at kvm_util.c:1395 (discriminator 6)
2 0x0000000010001a6b: test_vcpu_creation at kvm_create_max_vcpus.c:32 (discriminator 3)
3 0x000000001000160b: main at kvm_create_max_vcpus.c:59
4 0x000000001001b713: __libc_start_call_main at libc-start.o:?
5 0x000000001001bb47: __libc_start_main_impl at ??:?
KVM_CREATE_VCPU failed, rc: -1 errno: 22 (Invalid argument)
[ 65.316986][ T302] KVM: TLB LPID invalidation hcall failed, rc=-2
[ 65.316986][ T302] KVM: TLB LPID invalidation hcall failed, rc=-2Cap both extensions at 2048 when kvmhv_is_nestedv2() is set, so userspace never requests an id the L0 cannot create. PowerNV and nestedv1 are unchanged. The issue is only seen with nestedv2. While at it, this also fixes the limit for KVM_CAP_NR_VCPUS.
--- arch/powerpc/include/asm/kvm_host.h | 1 + arch/powerpc/kvm/powerpc.c | 6 ++++++ 2 files changed, 7 insertions(+)
arch/powerpc/include/asm/kvm_host.h +1 -0
@@ -37,6 +37,7 @@#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE #include <asm/kvm_book3s_asm.h> /* for MAX_SMT_THREADS */ #define KVM_MAX_VCPU_IDS (MAX_SMT_THREADS * KVM_MAX_VCORES) +#define KVM_MAX_VCPU_IDS_NESTEDv2 2048/* PAPR H_GUEST_CREATE_VCPU vcpuId */ /* * Limit the nested partition table to 4096 entries (because that's what
arch/powerpc/kvm/powerpc.c +6 -0
@@ -663,12 +663,18 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) r =min (num_present_cpus (),KVM_MAX_VCPUS ); else r =min (num_online_cpus (),KVM_MAX_VCPUS ); +if (kvmhv_is_nestedv2 ()) + r =min (r, KVM_MAX_VCPU_IDS_NESTEDv2);break ;case KVM_CAP_MAX_VCPUS : r =KVM_MAX_VCPUS ; +if (kvmhv_is_nestedv2 ()) + r =min (r, KVM_MAX_VCPU_IDS_NESTEDv2);break ;case KVM_CAP_MAX_VCPU_ID : r =KVM_MAX_VCPU_IDS ; +if (kvmhv_is_nestedv2 ()) + r =min (r, KVM_MAX_VCPU_IDS_NESTEDv2);break ;#ifdef CONFIG_PPC_BOOK3S_64 case KVM_CAP_PPC_GET_SMMU_INFO :
--
2.39.5
[email protected]
AI reviewRe: [RFC 3/3] KVM: PPC: Book3S HV nestedv2: cap max vcpus to PAPR limit
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Low] Build failure on non-HV configurations (e.g. BookE or PR-only builds) due to undeclared identifier KVM_MAX_VCPU_IDS_NESTEDv2.
Pre-existing issues:
- [High] The patch correctly caps the reported KVM capabilities for nestedv2 maximum vCPUs and vCPU IDs to 2048, but it fails to enforce this limit during actual vCPU creation. A malicious userspace can bypass the capability check and pass a vcpu_id >= 2048, causing the L0 hypervisor to reject it and triggering an unbounded pr_err() log spam in the host kernel. --
commit 603e4503cd207cc17d84e5e865ff47b744a57762
Author: Ritesh Harjani (IBM) <[email protected]>
KVM: PPC: Book3S HV nestedv2: cap max vcpus to PAPR limit
This commit limits the maximum number of vCPUs and vCPU IDs reported by KVM for nestedv2 guests to 2048, as dictated by the PAPR specification. It updates KVM_CAP_NR_VCPUS, KVM_CAP_MAX_VCPUS, and KVM_CAP_MAX_VCPU_ID when kvmhv_is_nestedv2() is true.
Quoted 17 lines
arch/powerpc/include/asm/kvm_host.h +1
-0@@ -37,6 +37,7 @@#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE #include <asm/kvm_book3s_asm.h> /* for MAX_SMT_THREADS */ #define KVM_MAX_VCPU_IDS (MAX_SMT_THREADS * KVM_MAX_VCORES) +#define KVM_MAX_VCPU_IDS_NESTEDv2 2048/* PAPR H_GUEST_CREATE_VCPU vcpuId */ /* * Limit the nested partition table to 4096 entries (because that's what arch/powerpc/kvm/powerpc.c +0
-0
arch/powerpc/kvm/powerpc.c int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) · line 667
Quoted 5 lines
arch/powerpc/kvm/powerpc.c +2
-0@@ -663,12 +663,18 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) r =min (num_present_cpus (),KVM_MAX_VCPUS ); else r =min (num_online_cpus (),KVM_MAX_VCPUS ); +if (kvmhv_is_nestedv2 ()) + r =min (r, KVM_MAX_VCPU_IDS_NESTEDv2);
[Severity: Low]
Will this cause a build failure on non-HV configurations?
Since KVM_MAX_VCPU_IDS_NESTEDv2 is defined inside an
#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE block in asm/kvm_host.h, but the KVM_CAP_NR_VCPUS case in kvm_vm_ioctl_check_extension() sits outside of that #ifdef block, using it unconditionally here could result in an undeclared identifier error for builds without HV support (like BookE or PR-only).
arch/powerpc/kvm/powerpc.c int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) · line 677
Quoted 10 lines
arch/powerpc/kvm/powerpc.c +4
-0break ;case KVM_CAP_MAX_VCPUS : r =KVM_MAX_VCPUS ; +if (kvmhv_is_nestedv2 ()) + r =min (r, KVM_MAX_VCPU_IDS_NESTEDv2);break ;case KVM_CAP_MAX_VCPU_ID : r =KVM_MAX_VCPU_IDS ; +if (kvmhv_is_nestedv2 ()) + r =min (r, KVM_MAX_VCPU_IDS_NESTEDv2);
[Severity: High]
This is a pre-existing issue, but does capping the reported capability prevent userspace from actually creating vCPUs with IDs >= 2048?
While this patch fixes the capability reporting, a malicious userspace process can still bypass the capability check and pass a vcpu_id >= 2048 to the KVM_CREATE_VCPU ioctl.
If this happens, it appears kvm_arch_vcpu_precreate() in arch/powerpc/kvm/powerpc.c will still allow it:
int kvm_arch_vcpu_precreate(struct kvm *kvm, unsigned int id)
{
return 0;
}This could cause the L0 hypervisor to reject it in kvmhv_nestedv2_vcpu_create() in arch/powerpc/kvm/book3s_hv_nestedv2.c, which contains an unratelimited pr_err() log:
rc = plpar_guest_create_vcpu(0, vcpu->kvm->arch.lpid, vcpu->vcpu_id);
if (rc != H_SUCCESS) {
pr_err("KVM: Create Guest vcpu hcall failed, rc=%ld\n", rc);Could this be used by an unprivileged host user with /dev/kvm access to trigger an unbounded log spam and cause a local Denial of Service (DoS)? Should architecture-specific bounds checking be added to kvm_arch_vcpu_precreate() to reject vcpu_ids >= KVM_MAX_VCPU_IDS_NESTEDv2 before proceeding with vCPU creation?
Quoted 1 line
arch/powerpc/kvm/powerpc.c +0
-0break ;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3