Re: [PATCH v2] LoongArch: KVM: Advertise already-supported capabilities

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

Pre-existing issues:
- [High] The unsafe user-access macros (`unsafe_put_user()` and `unsafe_get_user()`) are used without the required `user_access_begin()` and `user_access_end()` guards, violating the kernel API contract.
- [High] A lockless read-modify-write on the shared VM state `kvm->arch.pv_features` allows userspace to bypass the validation check that requires all vCPUs to share identical PV features.
--

--- Patch [1]: [PATCH v2] LoongArch: KVM: Advertise already-supported capabilities ---
commit 91e3758510b96f271c2589373d1340c246fab45a
Author: Tao Cui <[email protected]>

LoongArch: KVM: Advertise already-supported capabilities
    
This commit advertises KVM_CAP_STEAL_TIME and KVM_CAP_VCPU_ATTRIBUTES on
LoongArch, making existing features discoverable to userspace.

> diff --git a/arch/loongarch/kvm/vm.c b/arch/loongarch/kvm/vm.c
> index 1317c718f896a..0a51931d6f6e0 100644
> --- a/arch/loongarch/kvm/vm.c
> +++ b/arch/loongarch/kvm/vm.c
> @@ -122,6 +122,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_IOEVENTFD:
>  	case KVM_CAP_MP_STATE:
>  	case KVM_CAP_SET_GUEST_DEBUG:
> +	case KVM_CAP_VCPU_ATTRIBUTES:

[Severity: High]
This is a pre-existing issue, but does advertising KVM_CAP_VCPU_ATTRIBUTES
expose a data race in kvm_loongarch_cpucfg_set_attr()?

Looking at arch/loongarch/kvm/vcpu.c, there is a lockless read-modify-write
sequence on the shared VM state:

arch/loongarch/kvm/vcpu.c:kvm_loongarch_cpucfg_set_attr() {
...
	/* 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;
...
}

If multiple threads concurrently call KVM_SET_DEVICE_ATTR on different vCPUs,
can they bypass this validation check by both reading before either writes,
resulting in divergent configurations being silently applied globally instead
of returning -EINVAL?

>  		r = 1;
>  		break;
>  	case KVM_CAP_NR_VCPUS:

[ ... ]

> @@ -136,6 +137,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_NR_MEMSLOTS:
>  		r = KVM_USER_MEM_SLOTS;
>  		break;
> +	case KVM_CAP_STEAL_TIME:

[Severity: High]
This isn't a bug introduced by this patch, but does advertising
KVM_CAP_STEAL_TIME expose an issue with unsafe memory accesses?

In arch/loongarch/kvm/vcpu.c, unsafe_put_user() is used without the required
user_access_begin() and user_access_end() guards:

arch/loongarch/kvm/vcpu.c:kvm_update_stolen_time() {
...
	unsafe_put_user(0, &st->preempted, out);
...
}

And similarly in kvm_vcpu_set_pv_preempted():

arch/loongarch/kvm/vcpu.c:kvm_vcpu_set_pv_preempted() {
...
	unsafe_put_user(KVM_VCPU_PREEMPTED, &st->preempted, out);
...
}

Since omitting user_access_begin() skips the access_ok() check at the access
site, could this cause a kernel fault if hardware-enforced user access
protections are introduced?

> +		r = kvm_pvtime_supported();
> +		break;
>  	default:
>  		r = 0;
>  		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1