Re: [PATCH v2] LoongArch: KVM: Add KVM_LOONGARCH_GET/SET_CSR for bulk CSR migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Missing bounds checking on the user-provided CSR index allows arbitrary out-of-bounds kernel memory read and write vulnerabilities via integer signedness conversion.
- [High] The new `KVM_LOONGARCH_GET_CSR` and `KVM_LOONGARCH_SET_CSR` ioctls violate KVM enumeration invariants because they are not discoverable via a KVM capability.
- [Medium] Missing validation of `pad` and `reserved` fields in UAPI structures prevents future extension.
--

--- Patch [1]: [PATCH v2] LoongArch: KVM: Add KVM_LOONGARCH_GET/SET_CSR for bulk CSR migration ---
commit d9e42903e3c1617b11882f82d3b16f57b379c8ac
Author: Tao Cui <cuitao@kylinos.cn>

LoongArch: KVM: Add KVM_LOONGARCH_GET/SET_CSR for bulk CSR migration

This commit introduces bulk get and set ioctls for migrating CSR state in a
single call, replacing the need for many KVM_GET/SET_ONE_REG calls. It adds
the KVM_LOONGARCH_GET_CSR and KVM_LOONGARCH_SET_CSR ioctls and their
associated data structures.

> diff --git a/arch/loongarch/include/uapi/asm/kvm.h b/arch/loongarch/include/uapi/asm/kvm.h
> index cd0b5c11ca9c2..59b3b7efa0397 100644
> --- a/arch/loongarch/include/uapi/asm/kvm.h
> +++ b/arch/loongarch/include/uapi/asm/kvm.h

[ ... ]

> @@ -127,6 +127,19 @@ struct kvm_sync_regs {
>  struct kvm_sregs {
>  };
>  
> +/* bulk CSR entries for VM migration */
> +struct kvm_loongarch_csr_entry {
> +	__u32 index;
> +	__u32 reserved;
> +	__u64 data;
> +};
> +
> +struct kvm_loongarch_csrs {
> +	__u32 ncsrs;	/* number of csr entries */
> +	__u32 pad;
> +	__DECLARE_FLEX_ARRAY(struct kvm_loongarch_csr_entry, entries);
> +};
> +

[Severity: Medium]
Are the pad and reserved fields validated to be zero?

Looking at the kvm_loongarch_csr_io() implementation, the structure is copied
from user space but the padding fields are not checked. If userspace leaves
garbage data in these fields, could it prevent the kernel from safely
repurposing them for future feature flags without breaking ABI compatibility?

>  struct kvm_iocsr_entry {
>  	__u32 addr;
>  	__u32 pad;
> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index 20c207d80e31a..9b411c694718c 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c

[ ... ]

> @@ -1009,6 +1009,73 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs

[ ... ]

> +	for (i = 0; i < csrs.ncsrs; i++) {
> +		unsigned int id = entries[i].index;
> +
> +		if (get_gcsr_flag(id) & INVALID_GCSR) {

[Severity: Critical]
Could an attacker pass a value greater than or equal to 0x80000000 for
entries[i].index?

Since get_gcsr_flag() takes a signed int:

arch/loongarch/kvm/main.c:get_gcsr_flag() {
    if (csr < CSR_MAX_NUMS)
        return gcsr_flag[csr];
    ...
}

Would this unmasked 32-bit unsigned index be implicitly converted to a
negative integer, bypassing the csr < CSR_MAX_NUMS signed comparison?

If the out-of-bounds read of gcsr_flag happens to lack the INVALID_GCSR bit,
could this negative ID be passed to _kvm_setcsr() and eventually to
kvm_write_sw_gcsr(), leading to an arbitrary out-of-bounds 64-bit kernel
memory write?

> +			ret = -EINVAL;
> +			break;
> +		}
> +

[ ... ]

> @@ -1266,6 +1333,12 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  			r = kvm_get_reg(vcpu, &reg);
>  		break;
>  	}
> +	case KVM_LOONGARCH_GET_CSR:
> +		r = kvm_loongarch_csr_io(vcpu, argp, false);
> +		break;
> +	case KVM_LOONGARCH_SET_CSR:
> +		r = kvm_loongarch_csr_io(vcpu, argp, true);
> +		break;

[Severity: High]
Does adding these ioctls without a corresponding KVM capability break KVM
enumeration invariants?

Since KVM_LOONGARCH_GET_CSR and KVM_LOONGARCH_SET_CSR are unconditionally
enabled here without a corresponding KVM_CAP_* constant, userspace tools like
QEMU might not have a programmatic way to discover if the running kernel
supports the new ioctls via KVM_CHECK_EXTENSION. Could this silently break
live migration or capability negotiation?

>  	case KVM_ENABLE_CAP: {
>  		struct kvm_enable_cap cap;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 419011097fa8e..aade65751721d 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h

[ ... ]

> @@ -1670,4 +1670,8 @@ struct kvm_pre_fault_memory {
>  	__u64 padding[5];
>  };
>  
> +/* LoongArch: bulk CSR get/set for migration */
> +#define KVM_LOONGARCH_GET_CSR	_IOWR(KVMIO, 0xd6, struct kvm_loongarch_csrs)
> +#define KVM_LOONGARCH_SET_CSR	_IOW(KVMIO,  0xd7, struct kvm_loongarch_csrs)
> +
>  #endif /* __LINUX_KVM_H */

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