On Thu, Jun 06, 2024, Mathias Krause wrote: > On 06.06.24 00:31, Sean Christopherson wrote: > > On Thu, Jun 06, 2024, Mathias Krause wrote: > >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > >> index 14841acb8b95..9f18fc42f018 100644 > >> --- a/virt/kvm/kvm_main.c > >> +++ b/virt/kvm/kvm_main.c > >> @@ -4200,7 +4200,7 @@ 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) > > > > Hmm, I don't love that KVM subtly relies on the KVM_MAX_VCPU_IDS check to guard > > against truncation when passing @id to kvm_arch_vcpu_precreate(), kvm_vcpu_init(), > > etc. I doubt that it will ever be problematic, but it _looks_ like a bug. > > It's not subtle but very explicit. KVM_MAX_VCPU_IDS is a small positive > number, depending on some arch specific #define, but with x86 allowing > for the largest value of 4 * 4096. That value, for sure, cannot exceed > U32_MAX, so an explicit truncation isn't needed as the upper bits will > already be zero if the limit check passes. > > While subtile integer truncation is the bug that my patch is actually > fixing, it is for the *userland* facing part of it, as in clarifying the > ABI to work on "machine-sized words", i.e. a ulong, and doing the limit > checks on these. > > *In-kernel* APIs truncate / sign extend / mix signed/unsigned values all > the time. The kernel is full of these. Trying to "fix" them all is an > uphill battle not worth fighting, imho. Oh, I'm not worry about something going wrong with the actual truncation. What I don't like is the primary in-kernal API, kvm_vm_ioctl_create_vcpu(), taking an unsigned long, but everything underneath converting that to an unsigned int, without much of anything to give the reader a clue that the truncation is deliberate. Similarly, without the context of the changelog, it's not at all obvious why kvm_vm_ioctl_create_vcpu() takes an unsigned long. E.g. x86 has another potentially more restrictive check on @id, and it looks quite odd to check @id against KVM_MAX_VCPU_IDS as an "unsigned long" in flow flow, but as an "unsigned int" in another. int kvm_arch_vcpu_precreate(struct kvm *kvm, unsigned int id) { if (kvm_check_tsc_unstable() && kvm->created_vcpus) pr_warn_once("SMP vm created on host with unstable TSC; " "guest TSC will not be reliable\n"); if (!kvm->arch.max_vcpu_ids) kvm->arch.max_vcpu_ids = KVM_MAX_VCPU_IDS; if (id >= kvm->arch.max_vcpu_ids) return -EINVAL; > I'd rather suggest to add a build time assert instead, as the existing > runtime check is sufficient (with my u32->ulong change). Something like > this: > > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -4200,12 +4200,13 @@ 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; > > + BUILD_BUG_ON(KVM_MAX_VCPU_IDS > INT_MAX); This should be UINT_MAX, no? Regardless, the "need" for an explicit BUILD_BUG_ON() is another reason I dislike relying on the KVM_MAX_VCPU_IDS check to detect truncation. If @id is checked as a 32-bit value, and we somehow screw up and define KVM_MAX_VCPU_IDS to be a 64-bit value, clang will rightly complain that the check is useless, e.g. given "#define KVM_MAX_VCPU_ID_TEST BIT(32)" arch/x86/kvm/x86.c:12171:9: error: result of comparison of constant 4294967296 with expression of type 'unsigned int' is always false [-Werror,-Wtautological-constant-out-of-range-compare] if (id > KVM_MAX_VCPU_ID_TEST) ~~ ^ ~~~~~~~~~~~~~~~~~~~~ 1 error generated. > if (id >= KVM_MAX_VCPU_IDS) > return -EINVAL; What if we do an explicit check before calling kvm_vm_ioctl_create_vcpu()? That would avoid the weird __id param, and provide a convenient location to document exactly why KVM checks for truncation. We could also move the "if (id >= KVM_MAX_VCPU_IDS)" check to kvm_vm_ioctl(), but I don't love that, because again IMO it makes the code less readable overall, loses clang's tuautological constant check, and the cost of the extra check against UINT_MAX is completely negligible. Though if I had to choose, I'd prefer moving the check to kvm_vm_ioctl() over taking an "unsigned long" in kvm_vm_ioctl_create_vcpu(). diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 4965196cad58..8155146b16cd 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -5083,6 +5083,13 @@ static long kvm_vm_ioctl(struct file *filp, return -EIO; switch (ioctl) { case KVM_CREATE_VCPU: + /* + * KVM tracks vCPU ID as a 32-bit value, be kind to userspace + * and reject too-large values instead of silently truncating. + */ + if (arg > UINT_MAX) + return -EINVAL; + r = kvm_vm_ioctl_create_vcpu(kvm, arg); break; case KVM_ENABLE_CAP: {