Re: [edk2-devel] [PATCH v1 2/3] ShellPkg: UefiShellDebug1CommandsLib: Image Execution Table in Dmem.c

["Stuart Yoder" <stuart.yoder@arm.com>]

Hi Sam,

See inline comments...

On 1/24/24 2:56 PM, Sam Kaynor wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4352
> 
> Implemented dumping of the Image Execution Table using Dmem.c
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Signed-off-by: Sam Kaynor <Sam.Kaynor@arm.com>
> ---
>   ShellPkg/Library/UefiShellDebug1CommandsLib/Dmem.c                         | 139 ++++++++++++++++++++
>   ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni |   3 +
>   2 files changed, 142 insertions(+)
> 
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Dmem.c b/ShellPkg/Library/UefiShellDebug1CommandsLib/Dmem.c
> index 1ae7b1f3d85c..5b0730b75268 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Dmem.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Dmem.c
> @@ -138,6 +138,142 @@ DisplayRtProperties (
>     return (ShellStatus);
>   }
>   
> +/**
> +  Retrieve the ImageExecutionTable Entry ImageName from Device Path
> +
> +  @param[in] Address    The pointer to the ImageExecutionTable.
> +**/
> +EFI_STATUS
> +GetBaseName (
> +  IN  CHAR16          *FileName,
> +  OUT CHAR16          **BaseName
> +  )
> +{
> +  UINT32              StrLen;
> +  CHAR16              *StrTail;
> +
> +  StrLen = StrSize(FileName);
> +
> +  for (StrTail = FileName + StrLen - 1; StrTail != FileName && *StrTail != L'\\'; StrTail--) {
> +  }
> +
> +  if (StrTail == FileName) {
> +    return EFI_NOT_FOUND;
> +  }
> +  *BaseName = StrTail+1;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Retrieve the ImageExecutionTable entries
> +
> +  @param[in] Address    The pointer to the ImageExecutionTable.
> +**/
> +EFI_STATUS
> +GetImageExecutionInfo (
> +  IN UINT64 Address
> +  )
> +{
> +  EFI_STATUS Status;
> +  EFI_IMAGE_EXECUTION_INFO       *InfoPtr;
> +  VOID                           *ptr;
> +  CHAR16                         *ImagePath;
> +  CHAR16                         *ImageName;
> +  UINTN                          *NumberOfImages;
> +  CHAR16                         *ActionType;
> +
> +  Status = EfiGetSystemConfigurationTable (&gEfiImageSecurityDatabaseGuid, &ptr);
> +
> +  NumberOfImages = ptr;
> +
> +  ptr += sizeof(NumberOfImages);

The arithmetic to advance to the table entries isn't super clear as
written.  I think you need to define a variable for the table
header EFI_IMAGE_EXECUTION_INFO_TABLE and use that.

The Memory Attribute Table is similar.  Take a look at how they
handled the pointer arithmetic here:

https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c#L197

> +  for (int Image = 0; Image < *NumberOfImages; Image++) {
> +    InfoPtr = ptr;
> +    ImagePath = ptr + sizeof(EFI_IMAGE_EXECUTION_INFO);
> +
> +    GetBaseName(ImagePath,&ImageName);
> +
> +    switch(InfoPtr->Action) {
> +      case EFI_IMAGE_EXECUTION_AUTHENTICATION:
> +        ActionType = L"AUTHENTICATION";
> +        break;
> +      case EFI_IMAGE_EXECUTION_AUTH_UNTESTED:
> +        ActionType = L"AUTHENTICATION UNTESTED";
> +        break;
> +      case EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED:
> +        ActionType = L"AUTHENTICATION SIGNATURE FAILED";
> +        break;
> +      case EFI_IMAGE_EXECUTION_AUTH_SIG_PASSED:
> +        ActionType = L"AUTHENTICATION SIGNATURE PASSED";
> +        break;
> +      case EFI_IMAGE_EXECUTION_AUTH_SIG_NOT_FOUND:
> +        ActionType = L"AUTHENTICATION SIGNATURE NOT FOUND";
> +        break;
> +      case EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND:
> +        ActionType = L"AUTHENTICATION SIGNATURE FOUND";
> +        break;
> +      case EFI_IMAGE_EXECUTION_POLICY_FAILED:
> +        ActionType = L"POILCY FAILED";
> +        break;
> +      case EFI_IMAGE_EXECUTION_INITIALIZED:
> +        ActionType = L"INITIALIZED";
> +        break;
> +      default:
> +        ActionType = L"invalid action";
> +    }
> +
> +    ShellPrintHiiEx(
> +      -1,
> +      -1,
> +      NULL,
> +      STRING_TOKEN (STR_DMEM_IMG_EXE_ENTRY),
> +      gShellDebug1HiiHandle,
> +      ImageName,
> +      ActionType
> +    );

When running this code I noticed that the column of action type
strings were not aligned.  Can you set a field width to align
them?

> +
> +    ptr += InfoPtr->InfoSize;

Move the above to the part of the for loop statement that increments
for each iteration of the loop.

> +  }
> +
> +  return Status;
> +}
> +
> +/**
> +  Display the ImageExecutionTable entries
> +
> +  @param[in] Address    The pointer to the ImageExecutionTable.
> +**/
> +SHELL_STATUS
> +DisplayImageExecutionEntries (
> +  IN UINT64 Address
> +  )
> +{
> +  EFI_IMAGE_EXECUTION_INFO_TABLE *ImageExecutionTable;
> +  SHELL_STATUS    ShellStatus;
> +  EFI_STATUS      Status;
> +
> +  ShellStatus = SHELL_SUCCESS;
> +
> +  if (Address != 0) {
> +    Status = EfiGetSystemConfigurationTable (&gEfiImageSecurityDatabaseGuid, (VOID **)&ImageExecutionTable);
> +    if (EFI_ERROR (Status)) {
> +      ShellStatus = SHELL_NOT_FOUND;
> +      ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_DMEM_ERR_GET_FAIL), gShellDebug1HiiHandle, L"ImageExecutionTable");
> +    } else {
> +      ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_DMEM_IMG_EXE_TABLE), gShellDebug1HiiHandle);
> +      Status = GetImageExecutionInfo(Address);
> +    }
> +  } else {
> +    ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_DMEM_ERR_NOT_FOUND), gShellDebug1HiiHandle, L"ImageExecutionTable");
> +  }
> +
> +  return (ShellStatus);
> +}
> +
> +
> +
>   STATIC CONST SHELL_PARAM_ITEM  ParamList[] = {
>     { L"-mmio", TypeFlag },
>     { L"-verbose", TypeFlag },
> @@ -368,6 +504,9 @@ ShellCommandRunDmem (
>             if (ShellStatus == SHELL_SUCCESS) {
>               ShellStatus = DisplayRtProperties (RtPropertiesTableAddress);
>             }
> +          if (ShellStatus == SHELL_SUCCESS) {
> +            ShellStatus = DisplayImageExecutionEntries (ImageExecutionTableAddress);
> +          }
>           }
>   
>         } else {
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni
> index 299b0ba44f31..eee9384e3ffb 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.uni
> @@ -144,6 +144,9 @@
>                                                     "  UPDATE_CAPSULE                   %d\r\n"
>                                                     "  QUERY_CAPSULE_CAPABILITIES       %d\r\n"
>                                                     "  QUERY_VARIABLE_INFO              %d\r\n"
> +#string STR_DMEM_IMG_EXE_TABLE    #language en-US "\r\nImage Execution Table\r\n"
> +                                                  "----------------------------------------\r\n"
> +#string STR_DMEM_IMG_EXE_ENTRY    #language en-US "%s: %s\r\n"
>   #string STR_DMEM_ERR_NOT_FOUND    #language en-US "\r\n%H%s%N: Table address not found.\r\n"
>   #string STR_DMEM_ERR_GET_FAIL     #language en-US "\r\n%H%s%N: Unable to get table information.\r\n"
>   


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