| Message ID | 20240612215415.3450952-4-minipli@grsecurity.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Reject vCPU IDs above 2^32 | expand |
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@grsecurity.net> > --- > 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
On 14.06.24 18:35, Sean Christopherson wrote: > On Wed, Jun 12, 2024, Mathias Krause wrote: >> 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. I explicitly excluded it as there's no strict requirement for calling KVM_ENABLE_CAP(KVM_CAP_MAX_VCPU_ID) before KVM_SET_BOOT_CPU_ID, so kvm->arch.max_vcpu_ids would not yet be set. But yeah, checking if it was already set prior to narrowing the compare to it is a neat way to solve that. Good idea! > 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; Heh, I had the if-else ladder before but went for the goto version after looking around, attempting not to deviate from the "style" used for all the other cases . :| > > 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]; This is a separate fix, imho -- not allowing to set kvm->arch.max_vcpu_ids to a value that would exclude kvm->arch.bsp_vcpu_id. So I'll put that a separate commit. However, this still doesn't prevent creating VMs that have no BSP as the actual vCPU ID assignment only happens later, when vCPUs are created. But, I guess, that's no real issue. If userland insists on not having a BSP, so be it. > @@ -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 Thanks, looking good! Will prepare a v3. Mathias
On Fri, Jun 14, 2024, Mathias Krause wrote: > On 14.06.24 18:35, Sean Christopherson wrote: > However, this still doesn't prevent creating VMs that have no BSP as the > actual vCPU ID assignment only happens later, when vCPUs are created. > > But, I guess, that's no real issue. If userland insists on not having a > BSP, so be it. "struct kvm" is zero-allocated, so the BSP will default to vCPU0. I wouldn't be at all surprised if VMMs rely on that (after looking, that does appear to be the case for our VMM).
On 14.06.24 22:36, Sean Christopherson wrote: > On Fri, Jun 14, 2024, Mathias Krause wrote: >> On 14.06.24 18:35, Sean Christopherson wrote: >> However, this still doesn't prevent creating VMs that have no BSP as the >> actual vCPU ID assignment only happens later, when vCPUs are created. >> >> But, I guess, that's no real issue. If userland insists on not having a >> BSP, so be it. > > "struct kvm" is zero-allocated, so the BSP will default to vCPU0. I wouldn't be > at all surprised if VMMs rely on that (after looking, that does appear to be the > case for our VMM). Sure, zero-initialized by default makes a lot of sense. I too would assume that CPU0 is the BSP. However, I was thinking more along the lines: --- a/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c +++ b/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c @@ -136,6 +136,7 @@ int main(int argc, char *argv[]) run_vm_bsp(0); run_vm_bsp(1); run_vm_bsp(0); + run_vm_bsp(42); check_set_bsp_busy(); } As in: having two vCPUs with IDs 0 and 1 but the BSP with an ID outside of that range, e.g. 42 in this case. That's still a working setup but without any dedicated BSP, so may cause some hickups for real operating systems. But, again, if a user does this on purpose and things break, well, can keep the pieces. Thanks, Mathias
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: mutex_unlock(&kvm->lock); break; #ifdef CONFIG_KVM_XEN
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@grsecurity.net> --- arch/x86/kvm/x86.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)