Re: [edk2-devel] [PATCH v2 2/2] ShellPkg/Acpiview: Adds ACPI WSMT Table parse

["Abdul Lateef Attar via groups.io" <AbdulLateef.Attar=amd.com@groups.io>]

Hi Pierre,

     I think the provided patch also serve the same purpose of 
validating the COMM_BUFFER_NESTED_PTR_PROTECTION against FIXED_COMM_BUFFERS.

Do you want me to remove the "ValidateReserved" implementation?

Here is the sample test results.

1)
   Protection Flag                    : 0x5
     FIXED_COMM_BUFFERS               : 0x1
     COMM_BUFFER_NESTED_PTR_PROTECTION  : 0x0
     SYSTEM_RESOURCE_PROTECTION       : 0x1
     Reserved                         : 0x0


Table Statistics:
0 Error(s)
0 Warning(s)
Shell>

2)

   Protection Flag                    : 0x6
     FIXED_COMM_BUFFERS               : 0x0
     COMM_BUFFER_NESTED_PTR_PROTECTION  : 0x1
     SYSTEM_RESOURCE_PROTECTION       : 0x1
     Reserved                         : 0x0
ERROR: COMM_BUFFER_NESTED_PTR_PROTECTION is set but FIXED_COMM_BUFFERS 
is not set.


Table Statistics:
1 Error(s)
0 Warning(s)
Shell>

Thanks

AbduL

On 08-03-2024 14:50, PierreGondois via groups.io wrote:
> Caution: This message originated from an External Source. Use proper 
> caution when opening attachments, clicking links, or responding.
>
>
> Hello Abdul,
>
> On 3/8/24 08:22, Abdul Lateef Attar wrote:
>> From: Abdul Lateef Attar <AbdulLateef.Attar@amd.com>
>>
>> Adds WSMT parse to the UefiShellAcpiViewCommandLib library.
>>
>> Cc: Zhichao Gao <zhichao.gao@intel.com>
>> Cc: Pierre Gondois  <pierre.gondois@arm.com>
>> Signed-off-by: Abdul Lateef Attar <AbdulLateef.Attar@amd.com>
>> Reviewed-by: Pierre Gondois  <pierre.gondois@arm.com>
>> ---
>>   .../UefiShellAcpiViewCommandLib/AcpiParser.h  |  17 ++
>>   .../Parsers/Wsmt/WsmtParser.c                 | 147 ++++++++++++++++++
>>   .../UefiShellAcpiViewCommandLib.c             |   1 +
>>   .../UefiShellAcpiViewCommandLib.inf           |   1 +
>>   4 files changed, 166 insertions(+)
>>   create mode 100644 
>> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Wsmt/WsmtParser.c
>>
>> diff --git 
>> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h 
>> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
>> index ba3364f2c2..6468fe5d8c 100644
>> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
>> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
>> @@ -985,6 +985,23 @@ ParseAcpiSsdt (
>>     IN UINT8    AcpiTableRevision
>>     );
>>
>> +/**
>> +  This function parses the ACPI WSMT table.
>> +
>> +  @param [in] Trace              If TRUE, trace the ACPI fields.
>> +  @param [in] Ptr                Pointer to the start of the buffer.
>> +  @param [in] AcpiTableLength    Length of the ACPI table.
>> +  @param [in] AcpiTableRevision  Revision of the ACPI table.
>> +**/
>> +VOID
>> +EFIAPI
>> +ParseAcpiWsmt (
>> +  IN BOOLEAN  Trace,
>> +  IN UINT8    *Ptr,
>> +  IN UINT32   AcpiTableLength,
>> +  IN UINT8    AcpiTableRevision
>> +  );
>> +
>>   /**
>>     This function parses the ACPI XSDT table
>>     and optionally traces the ACPI table fields.
>> diff --git 
>> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Wsmt/WsmtParser.c 
>> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Wsmt/WsmtParser.c
>> new file mode 100644
>> index 0000000000..3c7252b0bf
>> --- /dev/null
>> +++ 
>> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Wsmt/WsmtParser.c
>> @@ -0,0 +1,147 @@
>> +/** @file
>> +  WSMT table parser
>> +
>> +  Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved.
>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +
>> +  @par Reference(s):
>> +    - Windows SMM Security Mitigation Table spec, version 1.0
>> +**/
>> +
>> +#include <Library/UefiLib.h>
>> +#include <IndustryStandard/WindowsSmmSecurityMitigationTable.h>
>> +#include "AcpiParser.h"
>> +
>> +STATIC ACPI_DESCRIPTION_HEADER_INFO  AcpiHdrInfo;
>> +
>> +/**
>> +  This function validates the WSMT Protection flag.
>> +
>> +  @param [in] Ptr  Pointer to the start of the buffer.
>> +  @param [in] Context Pointer to context specific information e.g. this
>> +                      could be a pointer to the ACPI table header.
>> +
>> +**/
>> +STATIC
>> +VOID
>> +EFIAPI
>> +ValidateWsmtProtectionFlag (
>> +  IN UINT8  *Ptr,
>> +  IN VOID   *Context
>> +  )
>> +{
>> +  UINT32  ProtectionFlag;
>> +
>> +  ProtectionFlag = *(UINT32 *)Ptr;
>> +
>> +  if ((ProtectionFlag & 
>> EFI_WSMT_PROTECTION_FLAGS_COMM_BUFFER_NESTED_PTR_PROTECTION) \
>> +      == EFI_WSMT_PROTECTION_FLAGS_COMM_BUFFER_NESTED_PTR_PROTECTION)
>> +  {
>> +    if ((ProtectionFlag & 
>> EFI_WSMT_PROTECTION_FLAGS_FIXED_COMM_BUFFERS) \
>> +        != EFI_WSMT_PROTECTION_FLAGS_FIXED_COMM_BUFFERS)
>> +    {
>> +      IncrementErrorCount ();
>> +      Print (L"ERROR: COMM_BUFFER_NESTED_PTR_PROTECTION is set but 
>> FIXED_COMM_BUFFERS is not set.\n");
>> +    }
>> +  }
>> +}
>> +
>> +/**
>> +  This function validates the reserved bits in the WSMT Protection 
>> flag.
>> +
>> +  @param [in] Ptr  Pointer to the start of the buffer.
>> +  @param [in] Context Pointer to context specific information e.g. this
>> +                      could be a pointer to the ACPI table header.
>> +**/
>> +STATIC
>> +VOID
>> +EFIAPI
>> +ValidateReserved (
>> +  IN UINT8  *Ptr,
>> +  IN VOID   *Context
>> +  )
>> +{
>> +  UINT32  ProtectionFlag;
>> +
>> +  ProtectionFlag = *(UINT32 *)Ptr;
>> +
>> +  if ((ProtectionFlag & 0xFFFFFFF8) != 0) {
>> +    IncrementErrorCount ();
>> +    Print (L"ERROR: Reserved bits are not zero.\n");
>> +  }
>> +}
>> +
>> +/**
>> +  An ACPI_PARSER array describing the WSMT Protection flag .
>> +**/
>> +STATIC CONST ACPI_PARSER  WsmtProtectionFlagParser[] = {
>> +  { L"FIXED_COMM_BUFFERS ",                1,  0, L"0x%x", NULL, 
>> NULL, NULL,             NULL },
>> +  { L"COMM_BUFFER_NESTED_PTR_PROTECTION ", 1,  1, L"0x%x", NULL, 
>> NULL, NULL,             NULL },
>> +  { L"SYSTEM_RESOURCE_PROTECTION ",        1,  2, L"0x%x", NULL, 
>> NULL, NULL,             NULL },
>> +  { L"Reserved ",                          29, 3, L"0x%x", NULL, 
>> NULL, ValidateReserved, NULL },
>
> I think we misunderstood each other here.
> We should check that if COMM_BUFFER_NESTED_PTR_PROTECTION,
> then FIXED_COMM_BUFFERS is also set.
>
> So I think we need to:
> - store the value of FIXED_COMM_BUFFERS (cf. ACPI_PARSER::ItemPtr in 
> other parsers)
> - add a validate to COMM_BUFFER_NESTED_PTR_PROTECTION to check the above
>
> I don't think it is necessary to check the reserved bits,
>
> Regards,
> Pierre
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116505): https://edk2.groups.io/g/devel/message/116505
Mute This Topic: https://groups.io/mt/104804849/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-