Re: [PATCH 1/4] kvm: cpuid: adjust the returned nent field of kvm_cpuid2 for KVM_GET_SUPPORTED_CPUID and KVM_GET_EMULATED_CPUID

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Emanuele Giuseppe Esposito <eesposit@redhat.com> writes:

> On 31/03/2021 09:56, Vitaly Kuznetsov wrote:
>> Emanuele Giuseppe Esposito <eesposit@redhat.com> writes:
>> 
>>> On 31/03/2021 05:01, Sean Christopherson wrote:
>>>> On Tue, Mar 30, 2021, Emanuele Giuseppe Esposito wrote:
>>>>> Calling the kvm KVM_GET_[SUPPORTED/EMULATED]_CPUID ioctl requires
>>>>> a nent field inside the kvm_cpuid2 struct to be big enough to contain
>>>>> all entries that will be set by kvm.
>>>>> Therefore if the nent field is too high, kvm will adjust it to the
>>>>> right value. If too low, -E2BIG is returned.
>>>>>
>>>>> However, when filling the entries do_cpuid_func() requires an
>>>>> additional entry, so if the right nent is known in advance,
>>>>> giving the exact number of entries won't work because it has to be increased
>>>>> by one.
>>>>>
>>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>>> ---
>>>>>    arch/x86/kvm/cpuid.c | 6 ++++++
>>>>>    1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>>>>> index 6bd2f8b830e4..5412b48b9103 100644
>>>>> --- a/arch/x86/kvm/cpuid.c
>>>>> +++ b/arch/x86/kvm/cpuid.c
>>>>> @@ -975,6 +975,12 @@ int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
>>>>>    
>>>>>    	if (cpuid->nent < 1)
>>>>>    		return -E2BIG;
>>>>> +
>>>>> +	/* if there are X entries, we need to allocate at least X+1
>>>>> +	 * entries but return the actual number of entries
>>>>> +	 */
>>>>> +	cpuid->nent++;
>>>>
>>>> I don't see how this can be correct.
>>>>
>>>> If this bonus entry really is needed, then won't that be reflected in array.nent?
>>>> I.e won't KVM overrun the userspace buffer?
>>>>
>>>> If it's not reflected in array.nent, that would imply there's an off-by-one check
>>>> somewhere, or KVM is creating an entry that it doesn't copy to userspace.  The
>>>> former seems unlikely as there are literally only two checks against maxnent,
>>>> and they both look correct (famous last words...).
>>>>
>>>> KVM does decrement array->nent in one specific case (CPUID.0xD.2..64), i.e. a
>>>> false positive is theoretically possible, but that carries a WARN and requires a
>>>> kernel or CPU bug as well.  And fudging nent for that case would still break
>>>> normal use cases due to the overrun problem.
>>>>
>>>> What am I missing?
>>>
>>> (Maybe I should have put this series as RFC)
>>>
>>> The problem I see and noticed while doing the KVM_GET_EMULATED_CPUID
>>> selftest is the following: assume there are 3 kvm emulated entries, and
>>> the user sets cpuid->nent = 3. This should work because kvm sets 3
>>> array->entries[], and copies them to user space.
>>>
>>> However, when the 3rd entry is populated inside kvm (array->entries[2]),
>>> array->nent is increased once more (do_host_cpuid and
>>> __do_cpuid_func_emulated). At that point, the loop in
>>> kvm_dev_ioctl_get_cpuid and get_cpuid_func can potentially iterate once
>>> more, going into the
>>>
>>> if (array->nent >= array->maxnent)
>>> 	return -E2BIG;
>>>
>>> in __do_cpuid_func_emulated and do_host_cpuid, returning the error. I
>>> agree that we need that check there because the following code tries to
>>> access the array entry at array->nent index, but from what I understand
>>> that access can be potentially useless because it might just jump to the
>>> default entry in the switch statement and not set the entry, leaving
>>> array->nent to 3.
>> 
>> The problem seems to be exclusive to __do_cpuid_func_emulated(),
>> do_host_cpuid() always does
>> 
>> entry = &array->entries[array->nent++];
>> 
>> Something like (completely untested and stupid):
>> 
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 6bd2f8b830e4..54dcabd3abec 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -565,14 +565,22 @@ static struct kvm_cpuid_entry2 *do_host_cpuid(struct kvm_cpuid_array *array,
>>          return entry;
>>   }
>>   
>> +static bool cpuid_func_emulated(u32 func)
>> +{
>> +       return (func == 0) || (func == 1) || (func == 7);
>> +}
>> +
>>   static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func)
>>   {
>>          struct kvm_cpuid_entry2 *entry;
>>   
>> +       if (!cpuid_func_emulated())
>> +               return 0;
>> +
>>          if (array->nent >= array->maxnent)
>>                  return -E2BIG;
>>   
>> -       entry = &array->entries[array->nent];
>> +       entry = &array->entries[array->nent++];
>>          entry->function = func;
>>          entry->index = 0;
>>          entry->flags = 0;
>> @@ -580,18 +588,14 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func)
>>          switch (func) {
>>          case 0:
>>                  entry->eax = 7;
>> -               ++array->nent;
>>                  break;
>>          case 1:
>>                  entry->ecx = F(MOVBE);
>> -               ++array->nent;
>>                  break;
>>          case 7:
>>                  entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
>>                  entry->eax = 0;
>>                  entry->ecx = F(RDPID);
>> -               ++array->nent;
>> -       default:
>>                  break;
>>          }
>> 
>> should do the job, right?
>> 
>> 
>
> Yes, it would work better. Alternatively:
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index ba7437308d28..452b0acd6e9d 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -567,34 +567,37 @@ static struct kvm_cpuid_entry2 
> *do_host_cpuid(struct kvm_cpuid_array *array,
>
>   static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 
> func)
>   {
> -	struct kvm_cpuid_entry2 *entry;
> -
> -	if (array->nent >= array->maxnent)
> -		return -E2BIG;
> +	struct kvm_cpuid_entry2 entry;
> +	bool changed = true;
>
> -	entry = &array->entries[array->nent];
> -	entry->function = func;
> -	entry->index = 0;
> -	entry->flags = 0;
> +	entry.function = func;
> +	entry.index = 0;
> +	entry.flags = 0;
>
>   	switch (func) {
>   	case 0:
> -		entry->eax = 7;
> -		++array->nent;
> +		entry.eax = 7;
>   		break;
>   	case 1:
> -		entry->ecx = F(MOVBE);
> -		++array->nent;
> +		entry.ecx = F(MOVBE);
>   		break;
>   	case 7:
> -		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> -		entry->eax = 0;
> -		entry->ecx = F(RDPID);
> -		++array->nent;
> +		entry.flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> +		entry.eax = 0;
> +		entry.ecx = F(RDPID);
> +		break;
>   	default:
> +		changed = false;
>   		break;
>   	}
>
> +	if (changed) {
> +		if (array->nent >= array->maxnent)
> +			return -E2BIG;
> +
> +		memcpy(&array->entries[array->nent++], &entry, sizeof(entry));
> +	}
> +
>   	return 0;
>   }
>
> pros: avoids hard-coding another function that would check what the 
> switch already does. it will be more flexible if another func has to be 
> added. cons: there is a memcpy for each entry.

Looks good to me,

I'd drop just 'bool changed' and replaced it with 'goto out' in the
'default' case.

memcpy() here is not a problem I believe, this path is not that
performace critical.

-- 
Vitaly