linux-kernel - Re: [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version

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: <aPsds5SCmmoG4IJO@intel.com>
Date: Fri, 24 Oct 2025 14:33:23 +0800
From: Chao Gao <chao.gao@...el.com>
To: Kiryl Shutsemau <kas@...nel.org>
CC: Dave Hansen <dave.hansen@...el.com>, <linux-coco@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>, <x86@...nel.org>, Thomas Gleixner
	<tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov
	<bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, "H. Peter Anvin"
	<hpa@...or.com>, Kai Huang <kai.huang@...el.com>, Paolo Bonzini
	<pbonzini@...hat.com>, Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version

>I don't hate it. Seems more scalable than current approach.
>
>See some comments below.
>
>> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
>> index 128e6ffba736..fa9bb6d47a87 100644
>> --- a/arch/x86/virt/vmx/tdx/tdx.c
>> +++ b/arch/x86/virt/vmx/tdx/tdx.c
>> @@ -226,22 +226,23 @@ static int build_tdx_memlist(struct list_head *tmb_list)
>> 	return ret;
>>  }
>>  
>> -static int read_sys_metadata_field(u64 field_id, u64 *data)
>> +static int read_sys_metadata_field(u64 *field_id, void *ptr)
>
>Keeping the same name for completely different functionality?

how about read_sys_metadata_fields() or read_sys_metadata_all()?

>
>>  {
>> 	struct tdx_module_args args = {};
>> 	int ret;
>>  
>> 	/*
>> -	 * TDH.SYS.RD -- reads one global metadata field
>> -	 *  - RDX (in): the field to read
>> -	 *  - R8 (out): the field data
>> +	 * TDH.SYS.RDALL -- reads all global metadata fields
>> +	 *  - RDX (in): the physical address of the buffer to store
>> +	 *  - R8 (in/out): the initial field ID to read (in) and
>> +	 *		   the next field ID to read (out).
>> 	 */
>> -	args.rdx = field_id;
>> -	ret = seamcall_prerr_ret(TDH_SYS_RD, &args);
>> +	args.rdx = __pa(ptr);
>
>Maybe take virtual address (unsigned long) of the buffer as an argument
>to the function. And use virt_to_phys() here.
>
>This way there's no need in cast on caller side.

Sure. Will do.

>
>> +	args.r8  = *field_id;
>> +	ret = seamcall_prerr_ret(TDH_SYS_RDALL, &args);
>> 	if (ret)
>> 		return ret;
>> -
>> -	*data = args.r8;
>> +	*field_id = args.r8;
>>  
>> 	return 0;
>
>Hm. Isn't it buggy?
>
>Caller expects to see field_id == -1 to exit loop, but you never set it
>in case of an error. It will result in endless loop if error happens not on
>the first iteration.

The caller checks the return value and bails out if there was an error.

>
>Drop the branch and always return ret.

Setting field_id to -1 on error appears unnecessary since callers must check
the return value anyway. And, even if args.r8 were copied to field_id
on error, this wouldn't guarantee that field_id would be set to -1, as
SEAMCALLs may encounter #GP/#UD exceptions where r8 remains unchanged.

Given this, I prefer to leave field_id as an undefined value on error, and
callers should not read/use it when an error occurs.

What do you think?