[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;
>  	}

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

[Bibo Mao]

On 2026/8/10 下午4:17, Tao Cui wrote:
> 
> 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;
>> +		}
The for loop sentence is a little strange, can we use spinlock method 
rather than atomic cmpxchg method? It is not performance sensitive here.

Regards
Bibo Mao
>>   	default:
>>   		return -ENXIO;
>>   	}

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] KVM corrupts guest memory by unconditionally updating steal time for a disabled feature due to an unhandled state-machine transition.
- [High] LoongArch PV features are silently enabled by default, violating KVM cross-architecture enumeration invariants.
--

--- Patch [1]: [PATCH] LoongArch: KVM: Fix TOCTOU race on pv_features ---
commit 28d84af1392db0a3c512a369b492d598370fb64d
Author: Tao Cui <cuitao@kylinos.cn>

LoongArch: KVM: Fix TOCTOU race on pv_features

This patch fixes a check-then-set race condition in
kvm_loongarch_cpucfg_set_attr() by replacing it with a cmpxchg loop. This
ensures atomic updates of kvm->arch.pv_features when concurrent vCPUs
attempt to set the attributes.

> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index 20c207d80e31a..55030c37cf061 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,
[ ... ]
> -		/* 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;

[Severity: High]
This isn't a bug introduced by this patch, but does this logic allow the first
configuration write to disable features (ON-to-OFF) without clearing previously
initialized dependent states?

Since LOONGARCH_PV_FEAT_UPDATED is not yet set on the first write, an explicit
ON-to-OFF transition is allowed.

If userspace initially configures KVM_LOONGARCH_VCPU_PVTIME_CTRL (because the
feature is enabled by default), and later disables KVM_FEATURE_STEAL_TIME via
CPUCFG_KVM_FEATURE, _kvm_vcpu_load() will still unconditionally request
KVM_REQ_STEAL_UPDATE.

Could this lead to guest memory corruption? When kvm_update_stolen_time()
executes, it relies solely on the cached GPA valid bit:

arch/loongarch/kvm/vcpu.c:kvm_update_stolen_time() {
    ...
    if (!(gpa & KVM_STEAL_PHYS_VALID))
        return;
    ...
}

Should we verify if KVM_FEATURE_STEAL_TIME is still enabled before updating
steal time, or prevent the ON-to-OFF transition if dependent states are
already initialized?

[Severity: High]
This is also a pre-existing issue, but are LoongArch PV features silently
enabled by default?

Looking at kvm_vm_init_features() in arch/loongarch/kvm/vm.c:

arch/loongarch/kvm/vm.c:kvm_vm_init_features() {
    ...
    /* Enable all PV features by default */
    kvm->arch.pv_features |= BIT(KVM_FEATURE_IPI);
    kvm->arch.kvm_features |= BIT(KVM_LOONGARCH_VM_FEAT_PV_IPI);
    ...
}

Does this violate the KVM subsystem architecture rule that new guest-visible
features must be off by default and discoverable? It seems silently enabling
features could break capability negotiation and live migration.

> +			new = val | LOONGARCH_PV_FEAT_UPDATED;
> +			if (cmpxchg(&kvm->arch.pv_features, old, new) == old)
> +				return 0;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810081321.157258-1-cui.tao@linux.dev?part=1