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: {
