[PATCH] LoongArch: KVM: Fix TOCTOU race on pv_features

[PATCH] LoongArch: KVM: Fix TOCTOU race on pv_features

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

kvm_loongarch_cpucfg_set_attr() validates and writes the VM-wide
pv_features with a lockless check-then-set, so two vCPUs racing it can
both pass the "all-vCPUs-must-match" check and install divergent values.
Make the check-then-set atomic with a cmpxchg loop; the UPDATED bit
already packs the configured state into the same word.

Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 arch/loongarch/kvm/vcpu.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
index 20c207d80e31..55030c37cf06 100644
--- a/arch/loongarch/kvm/vcpu.c
+++ b/arch/loongarch/kvm/vcpu.c
@@ -1164,12 +1164,18 @@ static int kvm_loongarch_cpucfg_set_attr(struct kvm_vcpu *vcpu,
 		if (val & ~valid)
 			return -EINVAL;
 
-		/* All vCPUs need set the same PV features */
-		if ((kvm->arch.pv_features & LOONGARCH_PV_FEAT_UPDATED)
-				&& ((kvm->arch.pv_features & valid) != val))
-			return -EINVAL;
-		kvm->arch.pv_features = val | LOONGARCH_PV_FEAT_UPDATED;
-		return 0;
+		/* Atomically install val; the cmpxchg serializes concurrent setters. */
+		for (;;) {
+			unsigned long old, new;
+
+			old = READ_ONCE(kvm->arch.pv_features);
+			if ((old & LOONGARCH_PV_FEAT_UPDATED) &&
+			    ((old & valid) != val))
+				return -EINVAL;
+			new = val | LOONGARCH_PV_FEAT_UPDATED;
+			if (cmpxchg(&kvm->arch.pv_features, old, new) == old)
+				return 0;
+		}
 	default:
 		return -ENXIO;
 	}
-- 
2.43.0

Re: [PATCH] LoongArch: KVM: Fix TOCTOU race on pv_features

[Tao Cui]

Hi Bibo,
在 2026/8/10 16:13, Tao Cui 写道:
> From: Tao Cui <cuitao@kylinos.cn>
> 
> kvm_loongarch_cpucfg_set_attr() validates and writes the VM-wide
> pv_features with a lockless check-then-set, so two vCPUs racing it can
> both pass the "all-vCPUs-must-match" check and install divergent values.
> Make the check-then-set atomic with a cmpxchg loop; the UPDATED bit
> already packs the configured state into the same word.
> 

I just sent a small patch for the pv_features TOCTOU we discussed. It
turns the check-then-set into a cmpxchg loop on the existing UPDATED
bit, so there's no new field or lock.

While working on it I also looked at two other options and wanted to
mention them here.

One was a spinlock around the check-then-set. It works, but it needs a
new kvm_arch member just for this, which didn't seem worth it given the
UPDATED bit already keeps the value and flag in a single word.

The other is to move pv_features per-vCPU (vcpu->arch). That removes the
shared state altogether: no lock, no latch, no cross-vCPU check, and the
VMM just keeps the vCPUs in sync. It's the cleaner design, but a larger
change, since QEMU would need a matching change too. Today QEMU pushes
pv_features behind a process-wide `static int once` (only the first
vCPU), which relies on the per-VM storage. This is the per-CPU direction
you mentioned earlier [1]; if you're still planning to do it I'm happy to
hold off, otherwise I can put together the kernel + QEMU side.

All three were built and tested locally with a vCPU-attribute test (the
spinlock also came up clean under KCSAN).

Thanks,
Tao

[1] https://lore.kernel.org/all/9696e097-7399-96b2-e0ae-bbb0d2f8c0d7@loongson.cn/

> Signed-off-by: Tao Cui <cuitao@kylinos.cn>
> ---
>  arch/loongarch/kvm/vcpu.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index 20c207d80e31..55030c37cf06 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
> @@ -1164,12 +1164,18 @@ static int kvm_loongarch_cpucfg_set_attr(struct kvm_vcpu *vcpu,
>  		if (val & ~valid)
>  			return -EINVAL;
>  
> -		/* All vCPUs need set the same PV features */
> -		if ((kvm->arch.pv_features & LOONGARCH_PV_FEAT_UPDATED)
> -				&& ((kvm->arch.pv_features & valid) != val))
> -			return -EINVAL;
> -		kvm->arch.pv_features = val | LOONGARCH_PV_FEAT_UPDATED;
> -		return 0;
> +		/* Atomically install val; the cmpxchg serializes concurrent setters. */
> +		for (;;) {
> +			unsigned long old, new;
> +
> +			old = READ_ONCE(kvm->arch.pv_features);
> +			if ((old & LOONGARCH_PV_FEAT_UPDATED) &&
> +			    ((old & valid) != val))
> +				return -EINVAL;
> +			new = val | LOONGARCH_PV_FEAT_UPDATED;
> +			if (cmpxchg(&kvm->arch.pv_features, old, new) == old)
> +				return 0;
> +		}
>  	default:
>  		return -ENXIO;
>  	}