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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <aPtPKP7d9dbgcL6w@intel.com>
Date: Fri, 24 Oct 2025 18:04:24 +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

>> >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.
>
>I misread it. Missed the break.
>
>> >
>> >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.
>
>It is not undefined. TDX module sets R8 to -1 in case of error.

Yes, I saw it. That's TDX module ABI. It doesn't necessarily have to be the
semantics of this kernel API. So, we have two choices:

1. Follow the TDX module, i.e., set field_id to -1 on error. Then we should do:

	if (ret)
		*field_id = -1;
	else
		*field_id = args.r8;

 to cover #GP/#UD cases.

2. Leave field_id undefined on error, as in this patch.

I don't see the value of setting field_id to -1 on error if callers are
expected to check the return value.