On 06.06.24 00:31, Sean Christopherson wrote:
> On Thu, Jun 06, 2024, Mathias Krause wrote:
>> 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@xxxxxxxxx>
>> Cc: PaX Team <pageexec@xxxxxxxxxxx>
>> Signed-off-by: Mathias Krause <minipli@xxxxxxxxxxxxxx>
>> ---
>> virt/kvm/kvm_main.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> 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.
>
> If we really care enough to fix this, my vote is for something like so:
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 4965196cad58..08adfdb2817e 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4200,13 +4200,14 @@ 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;
> + u32 id = __id;
>
> - if (id >= KVM_MAX_VCPU_IDS)
> + if (id != __id || id >= KVM_MAX_VCPU_IDS)
> return -EINVAL;
>
> mutex_lock(&kvm->lock);
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);
if (id >= KVM_MAX_VCPU_IDS)
return -EINVAL;
Thanks,
Mathias
