Re: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count

["Krzysztof Koch" <krzysztof.koch@arm.com>]

Hi Zhichao,

Please find my comments inline marked as [Krzysztof].

Kind regards,

Krzysztof


-----Original Message-----
From: Gao, Zhichao <zhichao.gao@intel.com> 
Sent: Monday, August 19, 2019 2:19
To: devel@edk2.groups.io; Krzysztof Koch <Krzysztof.Koch@arm.com>
Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>
Subject: RE: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: Validate System Locality count



> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of 
> Krzysztof Koch
> Sent: Thursday, August 15, 2019 9:11 PM
> To: devel@edk2.groups.io
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray 
> <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; 
> Sami.Mujawar@arm.com; Matteo.Carlini@arm.com; nd@arm.com
> Subject: [edk2-devel] [PATCH v1 05/11] ShellPkg: acpiview: SLIT: 
> Validate System Locality count
> 
> 1. Check if the 'Number of System Localities' provided can be 
> represented in the SLIT table. The table 'Length' field is a 32-bit 
> value while the 'Number of System Localities' field is 64-bit long.
> 
> 2. Check if the SLIT matrix fits in the table buffer. If N is the SLIT 
> locality count, then the matrix used to represent the localities is 
> N*N bytes long. The ACPI table length must be big enough to fit the matrix.
> 
> 3. Remove (now) redundant 64x64 bit multiplication.

Why removing? This change is added to fixed the issue build error with IA32 multiplication of two 64 bits data.
The change of #3 should be removed from the patch.
Keeping the variable size as UINT64 wouldn't affect the result.

Thanks,
Zhichao

[Krzysztof] If you look closely, in this patch I have removed the need to 64x64 bit multiplication. As I explain in the commit message, the specification of the SLIT table has an error. The System Locality Count is a 64-bit value while the ACPI table length field is 32-bit long. 

Consequently, after the right checks are implemented (in this patch), it is possible to operate on 32-bit values only. I believe that now MultU64x64() is no longer needed so I reverted back to the '*' multiplication operator.

Please let me know what you think.

> 
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
> 
> Notes:
>     v1:
>     - Validate the 'Number of System Localities' Field [Krzysztof]
>     - Remove redundant 64x64 bit multiplication [Krzysztof]
> 
>
> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser.c 
> |
> 47 +++++++++++++++++---
>  1 file changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser
> .c 
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser
> .c
> index
> 17e2166a09d8615b714e0c51d4d93d293fcdf601..e4625ee8b13907893a9b6990
> ecb956baf91cc3b9 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitParser
> .c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Slit/SlitPa
> +++ rs
> +++ er.c
> @@ -30,7 +30,7 @@ STATIC CONST ACPI_PARSER SlitParser[] = {
>  /**
>    Macro to get the value of a System Locality  **/ -#define 
> SLIT_ELEMENT(Ptr, i, j) *(Ptr + (MultU64x64 (i, LocalityCount)) + j)
> +#define SLIT_ELEMENT(Ptr, i, j) *(Ptr + (i * LocalityCount) + j)
> 
>  /**
>    This function parses the ACPI SLIT table.
> @@ -57,9 +57,9 @@ ParseAcpiSlit (
>    )
>  {
>    UINT32 Offset;
> -  UINT64 Count;
> -  UINT64 Index;
> -  UINT64 LocalityCount;
> +  UINT32 Count;
> +  UINT32 Index;
> +  UINT32 LocalityCount;
>    UINT8* LocalityPtr;
>    CHAR16 Buffer[80];  // Used for AsciiName param of ParseAcpi
> 
> @@ -87,8 +87,45 @@ ParseAcpiSlit (
>      return;
>    }
> 
> +  /*
> +    Despite the 'Number of System Localities' being a 64-bit field in SLIT,
> +    the maximum number of localities that can be represented in SLIT 
> + is
> limited
> +    by the 'Length' field of the ACPI table.
> +
> +    Since the ACPI table length field is 32-bit wide. The maximum number of
> +    localities that can be represented in SLIT can be calculated as:
> +
> +    MaxLocality = sqrt (MAX_UINT32 - sizeof
> (EFI_ACPI_6_3_SYSTEM_LOCALITY_DISTANCE_INFORMATION_TABLE_HEAD
> ER))
> +                = 65535
> +                = MAX_UINT16
> +  */
> +  if (*SlitSystemLocalityCount > MAX_UINT16) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"ERROR: The Number of System Localities provided can't be
> represented " \
> +        L"in the SLIT table. SlitSystemLocalityCount = %ld. " \
> +        L"MaxLocalityCountAllowed = %d.\n",
> +      *SlitSystemLocalityCount,
> +      MAX_UINT16
> +      );
> +    return;
> +  }
> +
> +  LocalityCount = (UINT32)*SlitSystemLocalityCount;
> +
> +  // Make sure system localities fit in the table buffer provided  if 
> + (Offset + (LocalityCount * LocalityCount) > AcpiTableLength) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"ERROR: Invalid Number of System Localities. " \
> +        L"SlitSystemLocalityCount = %ld. AcpiTableLength = %d.\n",
> +      *SlitSystemLocalityCount,
> +      AcpiTableLength
> +      );
> +    return;
> +  }
> +
>    LocalityPtr = Ptr + Offset;
> -  LocalityCount = *SlitSystemLocalityCount;
> 
>    // We only print the Localities if the count is less than 16
>    // If the locality count is more than 16 then refer to the
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 
> 
> 
>