search

Vector35 / binaryninja-api

Public API, examples, documentation and issues for Binary Ninja
https://binary.ninja/
MIT License
940 stars 213 forks source link

Automatic sizing of stack variables #5412

Closed joelreymont closed 6 months ago

joelreymont commented 6 months ago

Version and Platform (required):

Internal binary major dine favor.

Notice how IDA Pro automatically determines the size of payload here

uint64_t __fastcall DjiProductInfo_GetPayloadInfoHandle(struct CommandManager *mgr, struct CommandHeader *header)
{
  __int64 payload[14]; // [xsp+38h] [xbp+38h] BYREF
  __int64 v6; // [xsp+A8h] [xbp+A8h]
  __int64 v7[4]; // [xsp+B0h] [xbp+B0h] BYREF
  uint64_t v8; // [xsp+D0h] [xbp+D0h]
  T_DjiOsalHandler *osal; // [xsp+D8h] [xbp+D8h]

  osal = DjiPlatform_GetOsalHandler();
  memset(payload, 0, sizeof(payload));
  v6 = 0LL;
  memset(v7, 0, sizeof(v7));
  v8 = osal->MutexLock((T_DjiMutexHandle)s_productInfoMutex);
  if ( v8 )
  {
    DjiLogger_Output("core", 0, "[%s:%d) lock mutex error: 0x%08llX.", "DjiProductInfo_GetPayloadInfoHandle", 203LL, v8);
    return v8;
  }
  else
  {
    strncpy((char *)payload + 1, s_productInfo, 0x1FuLL);
    *(__int64 *)((char *)&payload[4] + 1) = *(_QWORD *)&s_productInfo[32];
    *(__int64 *)((char *)&payload[5] + 1) = *(_QWORD *)&s_productInfo[40];
    strncpy((char *)&payload[6] + 1, byte_292970, 0x3FuLL);
    BYTE1(v6) = dword_2929B8;
    WORD1(v6) = dword_2929BC;
    HIDWORD(v6) = *(int *)((char *)&dword_2929BC + 2);
    strncpy((char *)v7, byte_2929C2, 0x1FuLL);
    v8 = osal->MutexUnlock((T_DjiMutexHandle)s_productInfoMutex);
    if ( v8 )
    {
      DjiLogger_Output(
        "core",
        0,
        "[%s:%d) unlock mutex error: 0x%08llX.",
        "DjiProductInfo_GetPayloadInfoHandle",
        221LL,
        v8);
      return v8;
    }
    else
    {
      v8 = DjiCommand_SendAckData(mgr, header, payload, 0x98);
      if ( v8 )
        DjiLogger_Output(
          "core",
          0,
          "[%s:%d) get DJI product information ack error:0x%08llX",
          "DjiProductInfo_GetPayloadInfoHandle",
          228LL,
          v8);
      return v8;
    }
  }
}

Why doesn't BN do the same?

0009ecc4  uint64_t DjiProductInfo_GetPayloadInfoHandle(struct CommandManager* mgr, struct CommandHeader* header)

0009ecd4      int64_t x2
0009ecd4      int64_t var_c8 = x2
0009ecd8      struct T_DjiOsalHandler* osal = DjiPlatform_GetOsalHandler()
0009ece4      int64_t payload = 0
0009ece4      int64_t s
0009ece4      __builtin_memset(s: &s, c: 0, n: 0x90)
0009ed2c      payload.b = 0
0009ed44      uint64_t x0_2 = osal->MutexLock(mutex: s_productInfoMutex)
0009ed54      uint64_t x0_4
0009ed54      if (x0_2 != 0)
0009ed84          DjiLogger_Output(tag: "core", level: 0, fmt: "[%s:%d) lock mutex error: 0x%08l…", "DjiProductInfo_GetPayloadInfoHan…", 0xcb, x0_2)
0009ed88          x0_4 = x0_2
0009ed54      else
0009eda4          strncpy(&payload:1, &PRODUCT_INFO, 0x1f)
0009edb0          void* x0_6 = PRODUCT_INFO.appId[0].q
0009edb8          int64_t var_88 = x0_6
0009edb8          int64_t var_80 = *(x0_6 + 0x28)
0009edd0          int64_t var_78
0009edd0          strncpy(&var_78:1, &PRODUCT_INFO.developerAccount, 0x3f)
0009ede4          int64_t var_38
0009ede4          var_38:1.b = (data_2929b8).b
0009ee00          var_38:2.d = data_2929bc.d
0009ee08          var_38:4.d = data_2929be.d
0009ee20          int64_t var_30
0009ee20          strncpy(&var_30, &data_2929c2, 0x1f)
0009ee38          uint64_t x0_13 = osal->MutexUnlock(mutex: s_productInfoMutex)
0009ee48          if (x0_13 != 0)
0009ee78              DjiLogger_Output(tag: "core", level: 0, fmt: "[%s:%d) unlock mutex error: 0x%0…", "DjiProductInfo_GetPayloadInfoHan…", 0xdd, x0_13)
0009ee7c              x0_4 = x0_13
0009ee48          else
0009ee98              uint64_t x0_16 = DjiCommand_SendAckData(mgr, header, payload: &payload, payload_size: 0x98)
0009eea8              if (x0_16 != 0)
0009eed8                  DjiLogger_Output(tag: "core", level: 0, fmt: "[%s:%d) get DJI product informat…", "DjiProductInfo_GetPayloadInfoHan…", 0xe4, x0_16)
0009eedc              x0_4 = x0_16
0009eee4      return x0_4

I know the size is 0x98 because I know what payload_size is here 

0009ee98              uint64_t x0_16 = DjiCommand_SendAckData(mgr, header, payload: &payload, payload_size: 0x98)

but BN does auto-generate these and should know the size is at least 0x90

0009ece4      __builtin_memset(s: &s, c: 0, n: 0x90)

Regardless, IDA Pro generates the correct sizes and memsets, including for var30

memset(payload, 0, sizeof(payload));
  v6 = 0LL;
  memset(v7, 0, sizeof(v7));

whereas BN misses var30 completely.

joelreymont commented 6 months ago

Changing payload size to 0x98 in IDA Pro does this 

uint64_t __fastcall DjiProductInfo_GetPayloadInfoHandle(struct CommandManager *mgr, struct CommandHeader *header)
{
  char payload[152]; // [xsp+38h] [xbp+38h] BYREF
  uint64_t v6; // [xsp+D0h] [xbp+D0h]
  T_DjiOsalHandler *osal; // [xsp+D8h] [xbp+D8h]

  osal = DjiPlatform_GetOsalHandler();
  memset(payload, 0, sizeof(payload));
  v6 = osal->MutexLock((T_DjiMutexHandle)s_productInfoMutex);
  if ( v6 )
  {
    DjiLogger_Output("core", 0, "[%s:%d) lock mutex error: 0x%08llX.", "DjiProductInfo_GetPayloadInfoHandle", 203LL, v6);
    return v6;
  }
  else
  {
    strncpy(&payload[1], s_productInfo, 0x1FuLL);
    *(_QWORD *)&payload[33] = *(_QWORD *)&s_productInfo[32];
    *(_QWORD *)&payload[41] = *(_QWORD *)&s_productInfo[40];
    strncpy(&payload[49], byte_292970, 0x3FuLL);
    payload[113] = dword_2929B8;
    *(_WORD *)&payload[114] = dword_2929BC;
    *(_DWORD *)&payload[116] = *(int *)((char *)&dword_2929BC + 2);
    strncpy(&payload[120], byte_2929C2, 0x1FuLL);
    v6 = osal->MutexUnlock((T_DjiMutexHandle)s_productInfoMutex);
    if ( v6 )
    {
      DjiLogger_Output(
        "core",
        0,
        "[%s:%d) unlock mutex error: 0x%08llX.",
        "DjiProductInfo_GetPayloadInfoHandle",
        221LL,
        v6);
      return v6;
    }
    else
    {
      v6 = DjiCommand_SendAckData(mgr, header, payload, 0x98);
      if ( v6 )
        DjiLogger_Output(
          "core",
          0,
          "[%s:%d) get DJI product information ack error:0x%08llX",
          "DjiProductInfo_GetPayloadInfoHandle",
          228LL,
          v6);
      return v6;
    }
  }
}

which looks right. The stack layout according to IDA Pro is 

  char payload[152]; // [xsp+38h] [xbp+38h] BYREF
  uint64_t v6; // [xsp+D0h] [xbp+D0h]
  T_DjiOsalHandler *osal; // [xsp+D8h] [xbp+D8h]

but BN generates phantom unused variables x2 (passed in register X2) and var_c8 (at stack offset -0xc8).

0009ecc4  uint64_t DjiProductInfo_GetPayloadInfoHandle(struct CommandManager* mgr, struct CommandHeader* header)

0009ecd4      int64_t x2
0009ecd4      int64_t var_c8 = x2
0009ecd8      struct T_DjiOsalHandler* osal = DjiPlatform_GetOsalHandler()
0009ece4      char payload[0x98]
0009ece4      payload[0].q = 0
0009ece4      payload[8].q = 0
0009ecec      payload[0x10].q = 0
0009ecec      payload[0x18].q = 0
0009ecf4      payload[0x20].q = 0
0009ecf4      payload[0x28].q = 0
0009ecfc      payload[0x30].q = 0
0009ecfc      payload[0x38].q = 0
0009ed04      payload[0x40].q = 0
0009ed04      payload[0x48].q = 0
0009ed0c      payload[0x50].q = 0
0009ed0c      payload[0x58].q = 0
0009ed14      payload[0x60].q = 0
0009ed14      payload[0x68].q = 0
0009ed1c      payload[0x70].q = 0
0009ed1c      payload[0x78].q = 0
0009ed24      payload[0x80].q = 0
0009ed24      payload[0x88].q = 0
0009ed28      payload[0x90].q = 0
0009ed2c      payload[0] = 0
0009ed44      uint64_t x0_2 = osal->MutexLock(mutex: s_productInfoMutex)
0009ed54      uint64_t x0_4
0009ed54      if (x0_2 != 0)
0009ed84          DjiLogger_Output(tag: "core", level: 0, fmt: "[%s:%d) lock mutex error: 0x%08l…", "DjiProductInfo_GetPayloadInfoHan…", 0xcb, x0_2)
0009ed88          x0_4 = x0_2
0009ed54      else
0009eda4          strncpy(&payload[1], &PRODUCT_INFO, 0x1f)
0009edb0          void* x0_6 = PRODUCT_INFO.appId[0].q
0009edb8          payload[0x21].q = x0_6
0009edb8          payload[0x29].q = *(x0_6 + 0x28)
0009edd0          strncpy(&payload[0x31], &PRODUCT_INFO.developerAccount, 0x3f)
0009ede4          payload[0x71] = (data_2929b8).b
0009ee00          payload[0x72].d = data_2929bc.d
0009ee08          payload[0x74].d = data_2929be.d
0009ee20          strncpy(&payload[0x78], &data_2929c2, 0x1f)
0009ee38          uint64_t x0_13 = osal->MutexUnlock(mutex: s_productInfoMutex)
0009ee48          if (x0_13 != 0)
0009ee78              DjiLogger_Output(tag: "core", level: 0, fmt: "[%s:%d) unlock mutex error: 0x%0…", "DjiProductInfo_GetPayloadInfoHan…", 0xdd, x0_13)
0009ee7c              x0_4 = x0_13
0009ee48          else
0009ee98              uint64_t x0_16 = DjiCommand_SendAckData(mgr, header, payload: &payload, payload_size: 0x98)
0009eea8              if (x0_16 != 0)
0009eed8                  DjiLogger_Output(tag: "core", level: 0, fmt: "[%s:%d) get DJI product informat…", "DjiProductInfo_GetPayloadInfoHan…", 0xe4, x0_16)
0009eedc              x0_4 = x0_16
0009eee4      return x0_4
joelreymont commented 6 months ago

Related to #5395.

plafosse commented 6 months ago

Duplicate of #2570

joelreymont commented 6 months ago

Do I correctly understand that this bug has been open for... ugh... close to 3 years now?