linux-kernel - Re: [PATCHv2 04/12] x86/virt/tdx: Add tdx_alloc/free

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: <scebxrgqi7dgnyv6kjjpbcpijl4juita4lnq76jpw47rs5q2mc@xnbyphvk6vhv>
Date: Fri, 27 Jun 2025 16:00:49 +0300
From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
To: Dave Hansen <dave.hansen@...el.com>
Cc: pbonzini@...hat.com, seanjc@...gle.com, dave.hansen@...ux.intel.com, 
	rick.p.edgecombe@...el.com, isaku.yamahata@...el.com, kai.huang@...el.com, 
	yan.y.zhao@...el.com, chao.gao@...el.com, tglx@...utronix.de, mingo@...hat.com, 
	bp@...en8.de, kvm@...r.kernel.org, x86@...nel.org, linux-coco@...ts.linux.dev, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCHv2 04/12] x86/virt/tdx: Add tdx_alloc/free_page() helpers

On Wed, Jun 25, 2025 at 01:02:38PM -0700, Dave Hansen wrote:
> On 6/9/25 12:13, Kirill A. Shutemov wrote:
> >  arch/x86/include/asm/tdx.h       |   3 +
> >  arch/x86/include/asm/tdx_errno.h |   6 +
> >  arch/x86/virt/vmx/tdx/tdx.c      | 205 +++++++++++++++++++++++++++++++
> >  arch/x86/virt/vmx/tdx/tdx.h      |   2 +
> >  4 files changed, 216 insertions(+)
> 
> Please go through this whole series and add appropriate comments and
> explanations.
> 
> There are 4 lines of comments in the 216 lines of new code.

Will do.

> I'll give some examples:
> 
> > +static int tdx_nr_pamt_pages(void)
> 
> Despite the naming this function does not return the number of TDX
> PAMT pages. It returns the number of pages needed for each *dynamic*
> PAMT granule.

tdx_nr_dpamt_pages_per_2m()? This gets ugly.

> The naming is not consistent with something used only for dynamic PAMT
> support. This kind of comment would help, but is not a replacement for
> good naming:
> 
> /*
>  * How many pages are needed for the TDX
>  * dynamic page metadata for a 2M region?
>  */
> 
> Oh, and what the heck is with the tdx_supports_dynamic_pamt() check?
> Isn't it illegal to call these functions without dynamic PAMT in
> place? Wouldn't the TDH_PHYMEM_PAMT_ADD blow up if you hand it 0's
> in args.rdx?

Returning zero for !tdx_supports_dynamic_pamt() helps to avoid branches in
mmu_topup_memory_caches(). This way we pre-allocate zero pages in PAMPT
page cache.

> > +static int tdx_nr_pamt_pages(void)
> > +{
> > +	if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> > +		return 0;
> > +
> > +	return tdx_sysinfo.tdmr.pamt_4k_entry_size * PTRS_PER_PTE / PAGE_SIZE;
> > +}
> > +
> > +static u64 tdh_phymem_pamt_add(unsigned long hpa,
> > +			       struct list_head *pamt_pages)
> > +{
> > +	struct tdx_module_args args = {
> > +		.rcx = hpa,
> > +	};
> > +	struct page *page;
> > +	u64 *p;
> > +
> > +	WARN_ON_ONCE(!IS_ALIGNED(hpa & PAGE_MASK, PMD_SIZE));
> > +
> > +	p = &args.rdx;
> > +	list_for_each_entry(page, pamt_pages, lru) {
> > +		*p = page_to_phys(page);
> > +		p++;
> > +	}
> 
> This is sheer voodoo. Voodoo on its own is OK. But uncommented voodoo
> is not.
> 
> Imagine what would happen if, for instance, someone got confused and did:
> 
> 	tdx_alloc_pamt_pages(&pamd_pages);
> 	tdx_alloc_pamt_pages(&pamd_pages);
> 	tdx_alloc_pamt_pages(&pamd_pages);

I think tdx_alloc_pamt_pages() has to flag non-empty pamt_pages list.

> It would *work* because the allocation function would just merrily
> shove lots of pages on the list. But when it's consumed you'd run off
> the end of the data structure in this function far, far away from the
> bug site.
> 
> The least you can do here is comment what's going on. Because treating
> a structure like an array is obtuse at best.
> 
> Even better would be to have a check to ensure that the pointer magic
> doesn't run off the end of the struct:
> 
> 	if (p - &args.rcx >= sizeof(args)/sizeof(u64)) {
> 		WARN_ON_ONCE(1);
> 		break;
> 	}
> 
> or some other pointer voodoo.

Will do.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov