[v6,12/21] x86/virt/tdx: Add placeholder to construct TDMRs to cover all TDX memory regions

  TDX provides increased levels of memory confidentiality and integrity.
This requires special hardware support for features like memory
encryption and storage of memory integrity checksums.  Not all memory
satisfies these requirements.

As a result, the TDX introduced the concept of a "Convertible Memory
Region" (CMR).  During boot, the firmware builds a list of all of the
memory ranges which can provide the TDX security guarantees.  The list
of these ranges is available to the kernel by querying the TDX module.

The TDX architecture needs additional metadata to record things like
which TD guest "owns" a given page of memory.  This metadata essentially
serves as the 'struct page' for the TDX module.  The space for this
metadata is not reserved by the hardware up front and must be allocated
by the kernel and given to the TDX module.

Since this metadata consumes space, the VMM can choose whether or not to
allocate it for a given area of convertible memory.  If it chooses not
to, the memory cannot receive TDX protections and can not be used by TDX
guests as private memory.

For every memory region that the VMM wants to use as TDX memory, it sets
up a "TD Memory Region" (TDMR).  Each TDMR represents a physically
contiguous convertible range and must also have its own physically
contiguous metadata table, referred to as a Physical Address Metadata
Table (PAMT), to track status for each page in the TDMR range.

Unlike a CMR, each TDMR requires 1G granularity and alignment.  To
support physical RAM areas that don't meet those strict requirements,
each TDMR permits a number of internal "reserved areas" which can be
placed over memory holes.  If PAMT metadata is placed within a TDMR it
must be covered by one of these reserved areas.

Let's summarize the concepts:

 CMR - Firmware-enumerated physical ranges that support TDX.  CMRs are
       4K aligned.
TDMR - Physical address range which is chosen by the kernel to support
       TDX.  1G granularity and alignment required.  Each TDMR has
       reserved areas where TDX memory holes and overlapping PAMTs can
       be put into.
PAMT - Physically contiguous TDX metadata.  One table for each page size
       per TDMR.  Roughly 1/256th of TDMR in size.  256G TDMR = ~1G
       PAMT.

As one step of initializing the TDX module, the kernel configures
TDX-usable memory regions by passing an array of TDMRs to the TDX module.

Constructing the array of TDMRs consists below steps:

1) Create TDMRs to cover all memory regions that the TDX module can use;
2) Allocate and set up PAMT for each TDMR;
3) Set up reserved areas for each TDMR.

Add a placeholder to construct TDMRs to do the above steps after all
TDX memory regions are verified to be truly convertible.  Always free
TDMRs at the end of the initialization (no matter successful or not)
as TDMRs are only used during the initialization.

Reviewed-by: Isaku Yamahata <isaku.yamahata@intel.com>
Signed-off-by: Kai Huang <kai.huang@intel.com>
---

v5 -> v6:
 - construct_tdmrs_memblock() -> construct_tdmrs() as 'tdx_memblock' is
   used instead of memblock.
 - Added Isaku's Reviewed-by.

- v3 -> v5 (no feedback on v4):
 - Moved calculating TDMR size to this patch.
 - Changed to use alloc_pages_exact() to allocate buffer for all TDMRs
   once, instead of allocating each TDMR individually.
 - Removed "crypto protection" in the changelog.
 - -EFAULT -> -EINVAL in couple of places.

---
 arch/x86/virt/vmx/tdx/tdx.c | 72 +++++++++++++++++++++++++++++++++++++
 arch/x86/virt/vmx/tdx/tdx.h | 23 ++++++++++++
 2 files changed, 95 insertions(+)
  

> +/* Calculate the actual TDMR_INFO size */
> +static inline int cal_tdmr_size(void)
> +{
> +	int tdmr_sz;
> +
> +	/*
> +	 * The actual size of TDMR_INFO depends on the maximum number
> +	 * of reserved areas.
> +	 */
> +	tdmr_sz = sizeof(struct tdmr_info);
> +	tdmr_sz += sizeof(struct tdmr_reserved_area) *
> +		   tdx_sysinfo.max_reserved_per_tdmr;


would seem safer to have a overflow check here.
  

On Thu, 2022-10-27 at 08:31 -0700, Andi Kleen wrote:
> > +/* Calculate the actual TDMR_INFO size */
> > +static inline int cal_tdmr_size(void)
> > +{
> > +	int tdmr_sz;
> > +
> > +	/*
> > +	 * The actual size of TDMR_INFO depends on the maximum number
> > +	 * of reserved areas.
> > +	 */
> > +	tdmr_sz = sizeof(struct tdmr_info);
> > +	tdmr_sz += sizeof(struct tdmr_reserved_area) *
> > +		   tdx_sysinfo.max_reserved_per_tdmr;
> 
> 
> would seem safer to have a overflow check here.
> 
> 

How about below?

--- a/arch/x86/virt/vmx/tdx/tdx.c
+++ b/arch/x86/virt/vmx/tdx/tdx.c
@@ -614,6 +614,14 @@ static inline int cal_tdmr_size(void)
        tdmr_sz += sizeof(struct tdmr_reserved_area) *
                   tdx_sysinfo.max_reserved_per_tdmr;
 
+       /*
+        * Do simple check against overflow, and return 0 (invalid)
+        * TDMR_INFO size if it happened.  Also WARN() as it should
+        * should never happen in reality.
+        */
+       if (WARN_ON_ONCE(tdmr_sz < 0))
+               return 0;
+
        /*
         * TDX requires each TDMR_INFO to be 512-byte aligned.  Always
         * round up TDMR_INFO size to the 512-byte boundary.
@@ -623,19 +631,27 @@ static inline int cal_tdmr_size(void)
 
 static struct tdmr_info *alloc_tdmr_array(int *array_sz)
 {
+       int sz;
+
        /*
         * TDX requires each TDMR_INFO to be 512-byte aligned.
         * Use alloc_pages_exact() to allocate all TDMRs at once.
         * Each TDMR_INFO will still be 512-byte aligned since
         * cal_tdmr_size() always return 512-byte aligned size.
         */
-       *array_sz = cal_tdmr_size() * tdx_sysinfo.max_tdmrs;
+       sz = cal_tdmr_size() * tdx_sysinfo.max_tdmrs;
+
+       /* Overflow */
+       if (!sz || WARN_ON_ONCE(sz < 0))
+               return NULL;
+
+       *array_sz = sz;
 
        /*
         * Zero the buffer so 'struct tdmr_info::size' can be
         * used to determine whether a TDMR is valid.
         */
-       return alloc_pages_exact(*array_sz, GFP_KERNEL | __GFP_ZERO);
+       return alloc_pages_exact(sz, GFP_KERNEL | __GFP_ZERO);
 }


Btw, should I use alloc_contig_pages() instead of alloc_pages_exact() as IIUC
the latter should fail if the size is larger than 4MB?  In reality, the entire
array only takes dozens of KBs, though.
  

On Fri, 2022-10-28 at 02:21 +0000, Huang, Kai wrote:
> On Thu, 2022-10-27 at 08:31 -0700, Andi Kleen wrote:
> > > +/* Calculate the actual TDMR_INFO size */
> > > +static inline int cal_tdmr_size(void)
> > > +{
> > > +	int tdmr_sz;
> > > +
> > > +	/*
> > > +	 * The actual size of TDMR_INFO depends on the maximum number
> > > +	 * of reserved areas.
> > > +	 */
> > > +	tdmr_sz = sizeof(struct tdmr_info);
> > > +	tdmr_sz += sizeof(struct tdmr_reserved_area) *
> > > +		   tdx_sysinfo.max_reserved_per_tdmr;
> > 
> > 
> > would seem safer to have a overflow check here.
> > 
> > 
> 
> How about below?
> 
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -614,6 +614,14 @@ static inline int cal_tdmr_size(void)
>         tdmr_sz += sizeof(struct tdmr_reserved_area) *
>                    tdx_sysinfo.max_reserved_per_tdmr;
>  
> +       /*
> +        * Do simple check against overflow, and return 0 (invalid)
> +        * TDMR_INFO size if it happened.  Also WARN() as it should
> +        * should never happen in reality.
> +        */
> +       if (WARN_ON_ONCE(tdmr_sz < 0))
> +               return 0;
> +
>         /*
>          * TDX requires each TDMR_INFO to be 512-byte aligned.  Always
>          * round up TDMR_INFO size to the 512-byte boundary.
> @@ -623,19 +631,27 @@ static inline int cal_tdmr_size(void)
>  
>  static struct tdmr_info *alloc_tdmr_array(int *array_sz)
>  {
> +       int sz;
> +
>         /*
>          * TDX requires each TDMR_INFO to be 512-byte aligned.
>          * Use alloc_pages_exact() to allocate all TDMRs at once.
>          * Each TDMR_INFO will still be 512-byte aligned since
>          * cal_tdmr_size() always return 512-byte aligned size.
>          */
> -       *array_sz = cal_tdmr_size() * tdx_sysinfo.max_tdmrs;
> +       sz = cal_tdmr_size() * tdx_sysinfo.max_tdmrs;
> +
> +       /* Overflow */
> +       if (!sz || WARN_ON_ONCE(sz < 0))
> +               return NULL;
> +
> +       *array_sz = sz;
>  
>         /*
>          * Zero the buffer so 'struct tdmr_info::size' can be
>          * used to determine whether a TDMR is valid.
>          */
> -       return alloc_pages_exact(*array_sz, GFP_KERNEL | __GFP_ZERO);
> +       return alloc_pages_exact(sz, GFP_KERNEL | __GFP_ZERO);
>  }
> 
> 
> Btw, should I use alloc_contig_pages() instead of alloc_pages_exact() as IIUC
> the latter should fail if the size is larger than 4MB?  In reality, the entire
> array only takes dozens of KBs, though.

Hi Andi,

Could you take a look whether this is OK?

Also could you take a look my replies to your other comments?

Thanks!
  

On 10/27/22 08:31, Andi Kleen wrote:
> 
>> +/* Calculate the actual TDMR_INFO size */
>> +static inline int cal_tdmr_size(void)
>> +{
>> +    int tdmr_sz;
>> +
>> +    /*
>> +     * The actual size of TDMR_INFO depends on the maximum number
>> +     * of reserved areas.
>> +     */
>> +    tdmr_sz = sizeof(struct tdmr_info);
>> +    tdmr_sz += sizeof(struct tdmr_reserved_area) *
>> +           tdx_sysinfo.max_reserved_per_tdmr;
> 
> would seem safer to have a overflow check here.

tdmr_reserved_area is 16 bytes.  To overflow a signed int, tdmr_sz would
need to be for an allocation >2GB.  alloc_pages_exact() tops out at
supplying 4MB allocations.

So, sure, this breaks at max_reserved_per_tdmr>2^27, but it actually
breaks *EARLIER* at max_reserved_per_tdmr>2^18 because the page
allocator is borked.

Plus, max_reserved_per_tdmr is barely in double digits today.  It's a
*LOOOOOOOOONG* way from either of those limits.  If you want to add a
warning here, then go for it and enforce a sane value on
max_reserved_per_tdmr.

But, the overflow is *LITERALLY* an order of magnitude more obscure than
overwhelming the page allocator.  Let's not clutter up the code with
silly checks like that.
  

On Thu, 2022-11-03 at 08:05 -0700, Hansen, Dave wrote:
> Plus, max_reserved_per_tdmr is barely in double digits today.  It's a
> *LOOOOOOOOONG* way from either of those limits.  If you want to add a
> warning here, then go for it and enforce a sane value on
> max_reserved_per_tdmr.

Hi Dave,

Thanks. By "enforce a sane value on max_reserved_per_tdmr' could you be more
specific? Did you mean if we find its value is insanely big, we can change it to
a reasonable smaller value?

But I don't think we can as the TDMR_INFO is used by the TDX module, so reducing
max_reserved_per_tdmr by the kernel doesn't actually work?

Perhaps for now we can make the kernel to assume TDMR_INFO won't exceed a
reasonable value (i.e. 4K/8K/16K?) and max_tdmrs (which is 64 currently) won't
exceed a reasonable value either (i.e. 1K/512/256?), so that we can just use
alloc_pages_exact() to allocate  the entire TDMR array? If kernel found either
is too big, then kernel could just fail to initialize the TDX module.