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: <mvwzymoykhb6symwvwcc5liws4nb5cnxbyibh25gt67kufxhy2@vnqb3npptx5z>
Date: Fri, 24 Oct 2025 10:35:48 +0100
From: Kiryl Shutsemau <kas@...nel.org>
To: Chao Gao <chao.gao@...el.com>
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

On Fri, Oct 24, 2025 at 02:33:23PM +0800, Chao Gao wrote:
> >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()?

Looks good.

> 
> >
> >>  {
> >> 	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.

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.

> 
> What do you think?

-- 
  Kiryl Shutsemau / Kirill A. Shutemov