[RFC 0/3] KVM: PPC: BE guest-exit fixes and nestedv2 vCPU limit

[RFC 0/3] KVM: PPC: BE guest-exit fixes and nestedv2 vCPU limit

[Ritesh Harjani (IBM)]

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/cover.1789097569.git.ritesh.list@gmail.com/

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

[RFC 1/3] KVM: PPC: Book3S HV: Read EX_DSISR as a 32-bit field on guest exit

[Ritesh Harjani (IBM)]

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.

Fixes: 89d35b239101 ("KVM: PPC: Book3S HV P9: Implement the rest of the P9 path in C")
Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 arch/powerpc/kvm/book3s_hv_p9_entry.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv_p9_entry.c b/arch/powerpc/kvm/book3s_hv_p9_entry.c
index 34bc0a8a1288..16aa8582b888 100644
--- a/arch/powerpc/kvm/book3s_hv_p9_entry.c
+++ b/arch/powerpc/kvm/book3s_hv_p9_entry.c
@@ -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 lpc
 
 	if (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) {
-- 
2.39.5

[RFC 2/3] KVM: PPC: Book3S: Don't call __ffs() on an empty pending_exceptions bitmap

[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.

Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 arch/powerpc/kvm/book3s.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index 2efbe05caed7..e4152e9a0d96 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -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

Re: [RFC 2/3] KVM: PPC: Book3S: Don't call __ffs() on an empty pending_exceptions bitmap

[sashiko-bot]

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) <ritesh.list@gmail.com>

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.

> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index 2efbe05caed76..e4152e9a0d967 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -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);

[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?

> +		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 */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789099725.git.ritesh.list@gmail.com?part=2

[RFC 3/3] KVM: PPC: Book3S HV nestedv2: cap max vcpus to PAPR limit

[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=-2

Cap 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.

Fixes: 19d31c5f1157 ("KVM: PPC: Add support for nestedv2 guests")
Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 arch/powerpc/include/asm/kvm_host.h | 1 +
 arch/powerpc/kvm/powerpc.c          | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index 2d139c807577..d48232557259 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -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
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 9194cf492d1c..cf620b6534de 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -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

Re: [RFC 3/3] KVM: PPC: Book3S HV nestedv2: cap max vcpus to PAPR limit

[sashiko-bot]

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) <ritesh.list@gmail.com>

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.

> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index 2d139c8075773..d48232557259c 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -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
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 9194cf492d1ca..cf620b6534de6 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -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).

>  		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);

[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?

>  		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789099725.git.ritesh.list@gmail.com?part=3