Re: [PATCH v2 3/4] KVM: Limit check IDs for KVM_SET_BOOT_CPU_ID

[Sean Christopherson <seanjc@xxxxxxxxxx>]

On Wed, Jun 12, 2024, Mathias Krause wrote:
> Do not accept IDs which are definitely invalid by limit checking the
> passed value against KVM_MAX_VCPU_IDS.
> 
> This ensures invalid values, especially on 64-bit systems, don't go
> unnoticed and lead to a valid id by chance when truncated by the final
> assignment.
> 
> Fixes: 73880c80aa9c ("KVM: Break dependency between vcpu index in vcpus array and vcpu_id.")
> Signed-off-by: Mathias Krause <minipli@xxxxxxxxxxxxxx>
> ---
>  arch/x86/kvm/x86.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 082ac6d95a3a..8bc7b8b2dfc5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7220,10 +7220,16 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>  	case KVM_SET_BOOT_CPU_ID:
>  		r = 0;
>  		mutex_lock(&kvm->lock);
> -		if (kvm->created_vcpus)
> +		if (kvm->created_vcpus) {
>  			r = -EBUSY;
> -		else
> -			kvm->arch.bsp_vcpu_id = arg;
> +			goto set_boot_cpu_id_out;
> +		}
> +		if (arg > KVM_MAX_VCPU_IDS) {
> +			r = -EINVAL;
> +			goto set_boot_cpu_id_out;
> +		}
> +		kvm->arch.bsp_vcpu_id = arg;
> +set_boot_cpu_id_out:

Any reason not to check kvm->arch.max_vcpu_ids?  It's not super pretty, but it's
also not super hard.

And rather than use gotos, this can be done with if-elif-else, e.g. with the
below delta get to:

		r = 0;
		mutex_lock(&kvm->lock);
		if (kvm->created_vcpus)
			r = -EBUSY;
		else if (arg > KVM_MAX_VCPU_IDS ||
			 (kvm->arch.max_vcpu_ids && arg > kvm->arch.max_vcpu_ids))
			r = -EINVAL;
		else
			kvm->arch.bsp_vcpu_id = arg;
		mutex_unlock(&kvm->lock);
		break;


diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6e6f3d31cff0..994aa281b07d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6707,7 +6707,9 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
                        break;
 
                mutex_lock(&kvm->lock);
-               if (kvm->arch.max_vcpu_ids == cap->args[0]) {
+               if (kvm->arch.bsp_vcpu_id > cap->args[0]) {
+                       ;
+               } else if (kvm->arch.max_vcpu_ids == cap->args[0]) {
                        r = 0;
                } else if (!kvm->arch.max_vcpu_ids) {
                        kvm->arch.max_vcpu_ids = cap->args[0];
@@ -7226,16 +7228,13 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
        case KVM_SET_BOOT_CPU_ID:
                r = 0;
                mutex_lock(&kvm->lock);
-               if (kvm->created_vcpus) {
+               if (kvm->created_vcpus)
                        r = -EBUSY;
-                       goto set_boot_cpu_id_out;
-               }
-               if (arg > KVM_MAX_VCPU_IDS) {
+               else if (arg > KVM_MAX_VCPU_IDS ||
+                        (kvm->arch.max_vcpu_ids && arg > kvm->arch.max_vcpu_ids))
                        r = -EINVAL;
-                       goto set_boot_cpu_id_out;
-               }
-               kvm->arch.bsp_vcpu_id = arg;
-set_boot_cpu_id_out:
+               else
+                       kvm->arch.bsp_vcpu_id = arg;
                mutex_unlock(&kvm->lock);
                break;
 #ifdef CONFIG_KVM_XEN