Re: [PATCH v10 05/11] x86/virt/tdx: Handle multiple callers in tdx_pamt_get/put()

[Edgecombe, Rick P]

On Thu, 2026-09-03 at 02:03 +0000, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] tdx_find_dpamt_refcount() uses an unchecked PFN to index the dpamt_refcounts array, leading to out-of-bounds memory access.
> --
> 
> commit 413476a1747fac53ffdaf87030c42cb8b25bc668
> Author: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> 
> x86/virt/tdx: Handle multiple callers in tdx_pamt_get/put()
> 
> This patch refcounts the dynamic PAMT additions for 2MB regions so multiple
> callers can safely share them. It also drops the "__" prefix from the
> helpers and exports them for use by KVM.
> 
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index e8bdeacd989d6..a126e395f5f6a 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -292,7 +292,7 @@ static __init void free_dpamt_refcounts(void)
> >   	dpamt_refcounts = NULL;
> >   }
> >   
> > -static __maybe_unused atomic_t *tdx_find_dpamt_refcount(unsigned long pfn)
> > +static atomic_t *tdx_find_dpamt_refcount(unsigned long pfn)
> >   {
> >   	/* Find which PMD a PFN is in. */
> >   	unsigned long index = pfn >> (PMD_SHIFT - PAGE_SHIFT);
> > 
> >   	return &dpamt_refcounts[index];
> 
> [Severity: High]
> Does this index calculation need bounds checking?
> 
> If memory is hotplugged after boot, increasing the global max_pfn, and KVM
> allocates private guest memory that lands in this hotplugged region (e.g.,
> via guest_memfd), the returned PFN could be greater than the boot-time
> max_pfn used to size the dpamt_refcounts array. This would lead to an
> out-of-bounds pointer being returned here.

TDX's support for memory hotplug is limited. It will reject onlining memory that
was not present when setting up TDX initially to prevent it from being used for
TDX.

> 
> [ ... ]
> 
> > @@ -2150,41 +2144,59 @@ static int __tdx_pamt_get(kvm_pfn_t pfn)
> [ ... ]
> >   out_free:
> > +	spin_unlock(&dpamt_lock);
> >   	free_pamt_array(pamt_pages);
> >   
> >   	return ret;
> >   }
> > +EXPORT_SYMBOL_FOR_KVM(tdx_pamt_get);
> 
> [Severity: High]
> Is it safe to export this function without adding bounds checking to
> tdx_find_dpamt_refcount()?
> 
> Exporting this helper allows external callers like KVM to pass in arbitrary
> or hotplugged PFNs. If a hotplugged PFN is passed, the resulting
> out-of-bounds pointer from tdx_find_dpamt_refcount() will be modified by
> atomic_inc_not_zero() and atomic_set() within this function, potentially
> causing memory corruption or a host kernel panic.

This is not a real functional issue unless something passes an invalid pfn in. 

In the past there was an optimization to only populate the refcounts where there
was actually memory (i.e. leave gaps in the allocation). But now with the
current code, we could more reasonably warn by just checking max_pfn.
Although... I guess it still wouldn't warn about invalid PFNs, just refcount
bounds.

I think I'll leave it alone at this point unless somebody shouts. I don't know
about bounds checking, especially for the put side where there is no return
code. But it could be cheap thing to warn about.