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!
