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

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 <[email protected]>

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/[email protected]?part=1