linux-kernel - Re: [PATCH 10/13] x86/virt/tdx: Add SEAMCALL wrappers to remove a TD private page

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] [day] [month] [year] [list]

Message-ID: <Z38yIIhvlaz4NRB/@yzhao56-desk.sh.intel.com>
Date: Thu, 9 Jan 2025 10:19:12 +0800
From: Yan Zhao <yan.y.zhao@...el.com>
To: Dave Hansen <dave.hansen@...el.com>
CC: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "pbonzini@...hat.com" <pbonzini@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"sean.j.christopherson@...el.com" <sean.j.christopherson@...el.com>, "Huang,
 Kai" <kai.huang@...el.com>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "Yamahata, Isaku" <isaku.yamahata@...el.com>
Subject: Re: [PATCH 10/13] x86/virt/tdx: Add SEAMCALL wrappers to remove a TD
 private page

On Wed, Jan 08, 2025 at 08:31:14AM -0800, Dave Hansen wrote:
> On 1/7/25 16:41, Yan Zhao wrote:
> > There is a proposed fix to change the type of KeyID to u16 as shown below (not
> > yet split and sent out). Do you think this change to u16 makes sense?
> 
> I just think that the concept of a KeyID and the current implementation
> on today's hardware are different things. Don't confuse an
> implementation with the _concept_.
> 
> It can also make a lot of sense to pass around a 16-bit value in an
> 'int' in some cases. Think about NUMA nodes. You can't have negative
> NUMA nodes in hardware, but we use 'int' in the kernel everywhere
> because NUMA_NO_NODE gets passed around a lot.
> 
> Anyway, my point is that the underlying hardware types stop having
> meaning at _some_ level of abstraction in the interfaces.
Thanks for explaining the reasoning behind the preference to "int" and pointing
to the example of NUMA nodes.
It helps a lot for me to understand the underlying principle in kernel design!

Regarding the TDX hkid, do we need a similar check for the hkid (as that for
NUMA nodes) to avoid unexpected SEAMCALL error or overflow?

static inline bool numa_valid_node(int nid)
{
        return nid >= 0 && nid < MAX_NUMNODES;
}


> I'd personally probably just keep 'hkid' as an int everywhere until the
> point where it gets shoved into the TDX module ABI.
> 
> Oh, and casts like this:
> 
> >  static inline void tdx_disassociate_vp(struct kvm_vcpu *vcpu)
> > @@ -2354,7 +2354,8 @@ static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params,
> >  	ret = tdx_guest_keyid_alloc();
> >  	if (ret < 0)
> >  		return ret;
> > -	kvm_tdx->hkid = ret;
> > +	kvm_tdx->hkid = (u16)ret;
> > +	kvm_tdx->hkid_assigned = true;
> 
> are a bit silly, don't you think?
Agreed. That's the part I don't like about this fix.