linux-kernel - Re: [PATCH v2 24/25] KVM: x86: Introduce KVM_TDX_GET

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7574968a-f0e2-49d5-b740-2454a0f70bb6@intel.com>
Date: Fri, 10 Jan 2025 12:29:32 +0800
From: Xiaoyao Li <xiaoyao.li@...el.com>
To: Francesco Lavra <francescolavra.fl@...il.com>, rick.p.edgecombe@...el.com
Cc: isaku.yamahata@...il.com, kai.huang@...el.com, kvm@...r.kernel.org,
 linux-kernel@...r.kernel.org, pbonzini@...hat.com,
 reinette.chatre@...el.com, seanjc@...gle.com, tony.lindgren@...ux.intel.com,
 yan.y.zhao@...el.com
Subject: Re: [PATCH v2 24/25] KVM: x86: Introduce KVM_TDX_GET_CPUID

On 1/9/2025 7:07 PM, Francesco Lavra wrote:
> On 2024-10-30 at 19:00, Rick Edgecombe wrote:
>> @@ -1055,6 +1144,81 @@ static int tdx_td_vcpu_init(struct kvm_vcpu
>> *vcpu, u64 vcpu_rcx)
>>   	return ret;
>>   }
>>   
>> +/* Sometimes reads multipple subleafs. Return how many enties were
>> written. */
>> +static int tdx_vcpu_get_cpuid_leaf(struct kvm_vcpu *vcpu, u32 leaf,
>> int max_cnt,
>> +				   struct kvm_cpuid_entry2
>> *output_e)
>> +{
>> +	int i;
>> +
>> +	if (!max_cnt)
>> +		return 0;
>> +
>> +	/* First try without a subleaf */
>> +	if (!tdx_read_cpuid(vcpu, leaf, 0, false, output_e))
>> +		return 1;
>> +
>> +	/*
>> +	 * If the try without a subleaf failed, try reading subleafs
>> until
>> +	 * failure. The TDX module only supports 6 bits of subleaf
>> index.
> 
> It actually supports 7 bits, i.e. bits 6:0, so the limit below should
> be 0b1111111.

Nice catch!

>> +	 */
>> +	for (i = 0; i < 0b111111; i++) {
>> +		if (i > max_cnt)
>> +			goto out;
> 
> This will make this function return (max_cnt + 1) instead of max_cnt.
> I think the code would be simpler if max_cnt was initialized to
> min(max_cnt, 0x80) (because 0x7f is a supported subleaf index, as far
> as I can tell), and the for() condition was changed to `i < max_cnt`.

Looks better.

>> +		/* Keep reading subleafs until there is a failure.
>> */
>> +		if (tdx_read_cpuid(vcpu, leaf, i, true, output_e))
>> +			return i;
>> +
>> +		output_e++;

here the output_e++ can overflow the buffer.

>> +	}
>> +
>> +out:
>> +	return i;
>> +}
>> +
>> +static int tdx_vcpu_get_cpuid(struct kvm_vcpu *vcpu, struct
>> kvm_tdx_cmd *cmd)
>> +{
>> +	struct kvm_cpuid2 __user *output, *td_cpuid;
>> +	struct kvm_cpuid_entry2 *output_e;
>> +	int r = 0, i = 0, leaf;
>> +
>> +	output = u64_to_user_ptr(cmd->data);
>> +	td_cpuid = kzalloc(sizeof(*td_cpuid) +
>> +			sizeof(output->entries[0]) *
>> KVM_MAX_CPUID_ENTRIES,
>> +			GFP_KERNEL);
>> +	if (!td_cpuid)
>> +		return -ENOMEM;
>> +
>> +	for (leaf = 0; leaf <= 0x1f; leaf++) {
>> +		output_e = &td_cpuid->entries[i];
>> +		i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
>> +					     KVM_MAX_CPUID_ENTRIES -
>> i - 1,
> 
> This should be KVM_MAX_CPUID_ENTRIES - i.

Nice catch!

>> +					     output_e);
>> +	}
>> +
>> +	for (leaf = 0x80000000; leaf <= 0x80000008; leaf++) {
>> +		output_e = &td_cpuid->entries[i];
>> +		i += tdx_vcpu_get_cpuid_leaf(vcpu, leaf,
>> +					     KVM_MAX_CPUID_ENTRIES -
>> i - 1,
> 
> Same here.
>