| Message ID | 20240614202859.3597745-2-minipli@grsecurity.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Reject vCPU IDs above 2^32 | expand |
On Fri, Jun 14, 2024, Mathias Krause wrote: > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 14841acb8b95..b04e87f6568f 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -4200,12 +4200,20 @@ static void kvm_create_vcpu_debugfs(struct kvm_vcpu *vcpu) > /* > * Creates some virtual cpus. Good luck creating more than one. > */ > -static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id) > +static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id) > { > int r; > struct kvm_vcpu *vcpu; > struct page *page; > > + /* > + * KVM tracks vCPU IDs as 'int', be kind to userspace and reject > + * too-large values instead of silently truncating. > + * > + * Also ensure we're not breaking this assumption by accidentally > + * pushing KVM_MAX_VCPU_IDS above INT_MAX. I tweaked this slightly because it's not just accidental changes we need to guard against, and to "hint" that vcpu_id really should be an "unsigned int". /* * KVM tracks vCPU IDs as 'int', be kind to userspace and reject * too-large values instead of silently truncating. * * Ensure KVM_MAX_VCPU_IDS isn't pushed above INT_MAX without first * changing the storage type (at the very least, IDs should be tracked * as unsigned ints). */ > + */ > + BUILD_BUG_ON(KVM_MAX_VCPU_IDS > INT_MAX); > if (id >= KVM_MAX_VCPU_IDS) > return -EINVAL; > > -- > 2.30.2 >
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 14841acb8b95..b04e87f6568f 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -4200,12 +4200,20 @@ static void kvm_create_vcpu_debugfs(struct kvm_vcpu *vcpu) /* * Creates some virtual cpus. Good luck creating more than one. */ -static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id) +static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id) { int r; struct kvm_vcpu *vcpu; struct page *page; + /* + * KVM tracks vCPU IDs as 'int', be kind to userspace and reject + * too-large values instead of silently truncating. + * + * Also ensure we're not breaking this assumption by accidentally + * pushing KVM_MAX_VCPU_IDS above INT_MAX. + */ + BUILD_BUG_ON(KVM_MAX_VCPU_IDS > INT_MAX); if (id >= KVM_MAX_VCPU_IDS) return -EINVAL;
If, on a 64 bit system, a vCPU ID is provided that has the upper 32 bits set to a non-zero value, it may get accepted if the truncated to 32 bits integer value is below KVM_MAX_VCPU_IDS and 'max_vcpus'. This feels very wrong and triggered the reporting logic of PaX's SIZE_OVERFLOW plugin. Instead of silently truncating and accepting such values, pass the full value to kvm_vm_ioctl_create_vcpu() and make the existing limit checks return an error. Even if this is a userland ABI breaking change, no sane userland could have ever relied on that behaviour. Reported-by: PaX's SIZE_OVERFLOW plugin running on grsecurity's syzkaller Fixes: 6aa8b732ca01 ("[PATCH] kvm: userspace interface") Cc: Emese Revfy <re.emese@gmail.com> Cc: PaX Team <pageexec@freemail.hu> Signed-off-by: Mathias Krause <minipli@grsecurity.net> --- virt/kvm/kvm_main.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)