GaloisInc / macaw

Open source binary analysis tools.
BSD 3-Clause "New" or "Revised" License
208 stars 21 forks source link

`macaw-x86`: `call` semantics are wrong when the call target involves the stack pointer #420

Closed RyanGlScott closed 3 months ago

RyanGlScott commented 3 months ago

This C code:

// test.c
typedef struct {
    int foo;
    int (*fn)(void);
} stuff;

int callit(stuff s) {
    return s.fn();
}

int main(void) {
    return 0;
}

When compiled like so:

$ clang -fomit-frame-pointer test.c -o test.exe

Yields an x86-64 binary with the following machine code for the callit function:

0000000000001130 <callit>:
    1130:   48 83 ec 18             sub    $0x18,%rsp
    1134:   89 7c 24 08             mov    %edi,0x8(%rsp)
    1138:   48 89 74 24 10          mov    %rsi,0x10(%rsp)
    113d:   ff 54 24 10             call   *0x10(%rsp)
    1141:   48 83 c4 18             add    $0x18,%rsp
    1145:   c3                      ret    
    1146:   66 2e 0f 1f 84 00 00    cs nopw 0x0(%rax,%rax,1)
    114d:   00 00 00

Note that this calls the s.fn function pointer using the call *0x10(%rsp) instruction (at address 0x113d). This will end up being important later.

Now let's load the callit function into a macaw CFG using the following driver program:

```hs -- Main.hs {-# LANGUAGE DataKinds #-} {-# LANGUAGE GADTs #-} {-# LANGUAGE OverloadedStrings #-} module Main (main) where import Control.Monad (when) import qualified Data.ByteString as BS import qualified Data.ElfEdit as EE import qualified Data.Map as M import Data.Parameterized.Some (Some(..)) import qualified Prettyprinter as PP import qualified Data.Macaw.BinaryLoader.ELF as MBE import qualified Data.Macaw.CFG as MC import qualified Data.Macaw.Discovery as MD import qualified Data.Macaw.Memory as MM import qualified Data.Macaw.Memory.ElfLoader as MME import qualified Data.Macaw.Utils.IncComp as MUI import qualified Data.Macaw.X86 as MX main :: IO () main = do bytes <- BS.readFile "test.exe" case EE.decodeElfHeaderInfo bytes of Left (_off, msg) -> fail msg Right (EE.SomeElf e) -> do EE.ELFCLASS64 <- pure $ EE.headerClass $ EE.header e (warn, mem, _mentry, syms) <- case MME.resolveElfContents options e of Left err -> error err Right r -> pure r when (not (null warn)) $ do error $ "Warnings while loading Elf " ++ show warn let addrSymMap :: M.Map (MM.MemSegmentOff 64) BS.ByteString addrSymMap = M.fromList [ (MME.memSymbolStart sym, MME.memSymbolName sym) | sym <- syms ] -- `callit`'s address Just addrOff <- pure $ MBE.resolveAbsoluteAddress mem 0x1130 Some dfi <- discoverFunction MX.x86_64_linux_info mem addrSymMap addrOff print $ PP.pretty dfi where options = MME.LoadOptions { MME.loadOffset = Just 0 } discoverFunction :: MX.ArchitectureInfo arch -> MM.Memory (MC.ArchAddrWidth arch) -> MD.AddrSymMap (MC.ArchAddrWidth arch) -> MC.ArchSegmentOff arch -> IO (Some (MD.DiscoveryFunInfo arch)) discoverFunction archInf mem symMap addr = let s = MD.emptyDiscoveryState mem symMap archInf in MX.withArchConstraints archInf $ MUI.processIncCompLogs (const $ pure ()) $ MUI.runIncCompM $ do let discoveryOpts = MD.defaultDiscoveryOptions (_, funInfo) <- MUI.liftIncComp id $ MD.discoverFunction discoveryOpts addr MD.UserRequest s [] pure funInfo ```

The resulting macaw CFG is deeply suspicious, however:

$ runghc Main.hs 
function callit @ 0x1130
0x1130:
  ;  rsp = stack_frame + 0
  # 0x1130 sub    rsp,0x18
  r15 := (ssbb_overflows rsp_0 (0x18 :: [64]) false)
  r16 := (trunc rsp_0 4)
  r17 := (bv_ult r16 (0x8 :: [4]))
  r18 := (bv_ult rsp_0 (0x18 :: [64]))
  r19 := (bv_add rsp_0 (0xffffffffffffffe8 :: [64]))
  r20 := (bv_slt r19 (0x0 :: [64]))
  r21 := (eq rsp_0 (0x18 :: [64]))
  r22 := (trunc r19 8)
  r23 := (even_parity r22)
  # 0x1130: {rip => 0x1134
            ;rsp => r19
            ;cf => r18
            ;pf => r23
            ;af => r17
            ;zf => r21
            ;sf => r20
            ;of => r15}
  # 0x1134 mov    DWORD PTR [rsp+0x8],edi
  r24 := (trunc rdi_0 32)
  r25 := (bv_add rsp_0 (0xfffffffffffffff0 :: [64]))
  write_mem r25 r24
  # 0x1134: {rip => 0x1138}
  # 0x1138 mov    QWORD PTR [rsp+0x10],rsi
  r26 := (bv_add rsp_0 (0xfffffffffffffff8 :: [64]))
  write_mem r26 rsi_0
  # 0x1138: {rip => 0x113d}
  # 0x113d call   QWORD PTR [rsp+0x10]
  r27 := (bv_add rsp_0 (0xffffffffffffffe0 :: [64]))
  write_mem r27 0x1141
  r28 := (bv_add rsp_0 (0xfffffffffffffff0 :: [64]))
  r29 := read_mem r28 (bvle8)
  # 0x113d: {rip => r29;rsp => r27}
  call and return to 0x1141
    { rip = r29
    , rsp = r27
    , cf = r18
    , pf = r23
    , af = r17
    , zf = r21
    , sf = r20
    , df = false
    , of = r15
    , x87top = 0x7 :: [3]
    }
0x1141:
  ;  rsp = stack_frame - 24
  # 0x1141 add    rsp,0x18
  r41 := (sadc_overflows rsp_0 (0x18 :: [64]) false)
  r42 := (trunc rsp_0 4)
  r43 := (uadc_overflows r42 (0x8 :: [4]) false)
  r44 := (uadc_overflows rsp_0 (0x18 :: [64]) false)
  r45 := (bv_add rsp_0 (0x18 :: [64]))
  r46 := (bv_slt r45 (0x0 :: [64]))
  r47 := (eq rsp_0 (0xffffffffffffffe8 :: [64]))
  r48 := (trunc r45 8)
  r49 := (even_parity r48)
  # 0x1141: {rip => 0x1145
            ;rsp => r45
            ;cf => r44
            ;pf => r49
            ;af => r43
            ;zf => r47
            ;sf => r46
            ;of => r41}
  # 0x1145 ret
  r50 := read_mem r45 (bvle8)
  r51 := (bv_add rsp_0 (0x20 :: [64]))
  # 0x1145: {rip => r50;rsp => r51}
  return
    { rip = r50
    , rsp = r51
    , cf = r44
    , pf = r49
    , af = r43
    , zf = r47
    , sf = r46
    , df = false
    , of = r41
    , x87top = 0x7 :: [3]
    }

In particular, pay attention to the parts of the CFG leading up to address 0x113d. We would expect that just before the call and return to ... bit, the value of rip should be an address derived from %rsi (which we move to 0x10(%rsp) just before calling it). That's not what happens in the macaw CFG, however: instead, the value of rip just before the call is r29, which corresponds to a value derived from %rdi instead! This is quite wrong, and in a downstream application of mine that depends on macaw-x86, this results in macaw-symbolic jumping to a non-sensical address.

The root cause is a bug in macaw-x86's semantics. Currently, we have:

def_call :: InstructionDef
def_call = defUnary "call" $ \_ v -> do
  -- Push value of next instruction
  old_pc <- getReg R.X86_IP
  push addrRepr old_pc
  -- Set IP
  tgt <- getCallTarget v
  rip .= tgt

Note that we push the next instruction to the top of the stack before we get the call target. In this case, however, the call target is 0x10(%rsp), which means that pushing the next instruction will change the call target! That seems like a problem, and some searching reveals that radare2 used to suffer from the same bug.

I believe the following patch would fix this:

diff --git a/x86/src/Data/Macaw/X86/Semantics.hs b/x86/src/Data/Macaw/X86/Semantics.hs
index 370567a3..e923cd38 100644
--- a/x86/src/Data/Macaw/X86/Semantics.hs
+++ b/x86/src/Data/Macaw/X86/Semantics.hs
@@ -1376,11 +1376,12 @@ def_set_list =

 def_call :: InstructionDef
 def_call = defUnary "call" $ \_ v -> do
+  -- Get the address to branch to
+  tgt <- getCallTarget v
   -- Push value of next instruction
   old_pc <- getReg R.X86_IP
   push addrRepr old_pc
   -- Set IP
-  tgt <- getCallTarget v
   rip .= tgt

 -- | Conditional jumps

This ensures that we retrieve the call target before pushing the next instruction, which may influence the data that %rsp points to.