bn-uefi-helper icon indicating copy to clipboard operation
bn-uefi-helper copied to clipboard

UDK EDK2: EFI_BOOT_SERVICES offset mismatch in EFI_SYSTEM_TABLE struct (__packed attribute not set)

Open sylv-io opened this issue 2 years ago • 4 comments

First, thanks for this great plugin. It helps a lot.

While reversing an EFI binary build with UEFI Development Kit I realize there is an offset mismatch of EFI_BOOT_SERVICES in EFI_SYSTEM_TABLE struct. This plugin tells me it should have an offset of 0x70 But in edk2 it is 0x60.

How to reproduce:

Build HelloWorld.efi and compare UefiBootServicesTableLibConstructor() in MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.c

Source code:

EFI_STATUS
EFIAPI
UefiBootServicesTableLibConstructor (
  IN EFI_HANDLE        ImageHandle,
  IN EFI_SYSTEM_TABLE  *SystemTable
  )
{
  //
  // Cache the Image Handle
  //
  gImageHandle = ImageHandle;
  ASSERT (gImageHandle != NULL);

  //
  // Cache pointer to the EFI System Table
  //
  gST = SystemTable;
  ASSERT (gST != NULL);

  //
  // Cache pointer to the EFI Boot Services Table
  //
  gBS = SystemTable->BootServices;
  ASSERT (gBS != NULL);

  return EFI_SUCCESS;
}

HLIL:

000007b9  EFI_STATUS UefiBootServicesTableLibConstructor(EFI_HANDLE ImageHandle, struct EFI_SYSTEM_TABLE* SystemTable)

000007d7      data_46a0 = ImageHandle
000007fa      if (sub_75b() != 0 && data_46a0 == 0)
0000081f          sub_668(0x2d28)  {"/home/sylv/work/…"}
0000082f      data_46a8 = SystemTable
00000852      if (sub_75b() != 0 && data_46a8 == 0)
00000877          sub_668(0x2d28)  {"/home/sylv/work/…"}
0000088b      data_46b0 = SystemTable->StdErr
000008ae      if (sub_75b() != 0 && data_46b0 == 0)
000008d3          sub_668(0x2d28)  {"/home/sylv/work/…"}
000008db      return 0

LLIL snipped:

  20 @ 00000879  rax = [rbp + 0x18 {arg_10}].q
  21 @ 0000087d  rdx = [rax + 0x60 {EFI_SYSTEM_TABLE::StdErr}].q
  22 @ 00000881  rax = 0x46b0
  23 @ 0000088b  [rax {data_46b0}].q = rdx
  24 @ 0000088e  rax = 0x75b
  25 @ 00000898  call(rax)
  26 @ 0000089c  if (al == 0) then 36 @ 0x8d5 else 40 @ 0x89e

Relatet types in binary ninja:

struct EFI_SYSTEM_TABLE
{
    struct EFI_TABLE_HEADER Hdr;
    CHAR16* FirmwareVendor;
    UINT32 FirmwareRevision;
    EFI_HANDLE ConsoleInHandle;
    struct EFI_SIMPLE_TEXT_INPUT_PROTOCOL* ConIn;
    EFI_HANDLE ConsoleOutHandle;
    struct EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL* ConOut;
    EFI_HANDLE StandardErrorHandle;
    struct EFI_SIMPLE_TEXT_OUTPUT_PROTOCOL* StdErr;
    struct EFI_RUNTIME_SERVICES* RuntimeServices;
    struct EFI_BOOT_SERVICES* BootServices;
    UINTN NumberOfTableEntries;
    struct EFI_CONFIGURATION_TABLE* ConfigurationTable;
};

struct EFI_TABLE_HEADER
{
    UINT64 Signature;
    UINT32 Revision;
    // 4 byte empty
    UINT32 HeaderSize;
    // 4 byte empty
    UINT32 CRC32;
    // 4 byte empty
    UINT32 Reserved;
};

It is probably related to EFI_TABLE_HEADER size, but I haven't investigated it yet.

Thanks!

sylv-io avatar Mar 23 '22 21:03 sylv-io

Update:

UEFI Spec 2.9:

typedef struct {
EFI System Table
UINT64 Signature;
UINT32 Revision;
UINT32 HeaderSize;
UINT32 CRC32;
UINT32 Reserved;
} EFI_TABLE_HEADER;

headers/efi_pei_services_64.h

struct EFI_TABLE_HEADER {
    UINT64 Signature;
    UINT32 Revision;
    UINT32 HeaderSize;
    UINT32 CRC32;
    UINT32 Reserved;
};

Look like the header file is fine, but somehow it was not properly imported :sweat_smile:

sylv-io avatar Mar 23 '22 21:03 sylv-io

Workaround:

  1. undefine types EFI_SYSTEM_TABLE and EFI_TABLE_HEADER
  2. import EFI_TABLE_HEADER with attribute __packed
  3. import EFI_SYSTEM_TABLE

Should we apply this to all affected structs ? This should fix this issue.

sylv-io avatar Mar 23 '22 21:03 sylv-io

Thank you! I noticed the same issue you're describing after looking at 64-bit firmware but hadn't gotten around to fixing it upstream. I originally was looking at 32-bit firmware when I wrote the plugin, where the struct members aligned without specifying __packed. I've accepted your PR. And yes, I think the __packed attribute will need applied to all the structs.

zznop avatar Mar 24 '22 13:03 zznop

Indeed. I came back to mark my PR as WIP since there still affected structs :sweat_smile: I'll create a new one, which addresses all structs :+1:

sylv-io avatar Mar 24 '22 16:03 sylv-io