Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LR and FP aren't eliminated in HLIL #3

Open
toshipiazza opened this issue Aug 15, 2021 · 3 comments
Open

LR and FP aren't eliminated in HLIL #3

toshipiazza opened this issue Aug 15, 2021 · 3 comments

Comments

@toshipiazza
Copy link

toshipiazza commented Aug 15, 2021

The autogenerated LLIL for e.g. allocframe() and dealloc_return() manipulate LLIL_SPLIT_REG(LR, FP), and those refs don't seem to be elided in HLIL.

Example code (test_allocframe() in bn_llil_test_app):

00000020  01c09da0           { allocframe(SP,#0x8):raw } {var_8} {arg_0} {var_10}
00000024  00e00078           { R0 = #0x100 }
00000028  1ec01e96           { LR:FP = dealloc_return(FP):raw } {var_8}

LLIL:

// allocframe
   0 @ 00000020  temp29.d = SP {arg_0}
   1 @ 00000020  temp100.d = temp29.d - 8
   2 @ 00000020  [temp100.d {var_8}].q = LR:FP
   3 @ 00000020  FP = temp100.d
   4 @ 00000020  temp29.d = temp100.d - 8 {var_10}
   5 @ 00000020  SP = temp29.d
// r0 = 0x100
   6 @ 00000024  temp0.d = 0x100
   7 @ 00000024  R0 = temp0.d
// deallocframe
   8 @ 00000028  temp100.d = FP {var_8}
   9 @ 00000028  temp101.q = [temp100.d {var_8}].q
  10 @ 00000028  temp30.q = temp101.q
  11 @ 00000028  SP = temp100.d + 8
  12 @ 00000028  LR:FP = temp30.q
  13 @ 00000028  <return> jump(LR)

Resulting HLIL:

00000028      int32_t FP
00000028      int32_t FP_1
00000028      int32_t LR
00000028      int32_t LR_1
00000028      LR_1:FP_1 = LR:FP
00000028      return 0x100

I'd expect output more like

00000028      return 0x100

Looking at a different x86 binary, it seems that RBP and all callee-saved registers are eliminated somewhere between LLIL and MLIL.

@toshipiazza
Copy link
Author

Another curious thing is that, when I open bn_llil_test_app for the first time in binja, test_allocframe() infers FP as the first argument (I changed that to void in the snippets above). Specifically, binja infers

int32_t test_allocframe(int32_t arg1 @ FP)

This happens even when I add FP to GetCalleeSavedRegisters in arch_hexagon.cc

@toshipiazza
Copy link
Author

toshipiazza commented Aug 17, 2021

I'm pretty sure the issue lies with Binja's analysis. Either it expects callee-saved regs and LR/FP to be manipulated with LLIL push and pops, or it cannot handle LLIL_SPLIT_REG and LLIL_SET_SPLIT_REG.

To be sure, I modified lift_L4_return() and lift_S2_allocframe() to use straightforward push and pop's, and produced the following IL's:

LLIL:

   0 @ 00000020  push(LR)
   1 @ 00000020  push(FP)
   2 @ 00000020  FP = SP {__saved_FP}
   3 @ 00000020  SP = SP - 8
   4 @ 00000024  temp0.d = 0x100
   5 @ 00000024  R0 = temp0.d
   6 @ 00000028  SP = FP
   7 @ 00000028  FP = pop
   8 @ 00000028  LR = pop
   9 @ 00000028  <return> jump(LR)

HLIL:

00000028      return 0x100

FP is no longer inferred as an implicit argument to the function, as well.

@cfircohen
Copy link
Contributor

Yeah, I also encountered optimization problems with LLIL_SPLIT_REG.

TempReg::CopyFromTemp in plugin/packet_context.cc also uses split registers, so I experimented a bit, and changed the split reg computation to two SetRegisters with a logical shift:

void TempReg::CopyFromTemp(BinaryNinja::LowLevelILFunction &il) {
  ExprId expr;
  if (size_ == 1) {
    expr = il.SetRegister(1, reg_, il.Register(1, LLIL_TEMP(reg_)));
    il.AddInstruction(expr);
  } else if (size_ == 4) {
    expr = il.SetRegister(4, reg_, il.Register(4, LLIL_TEMP(reg_)));
    il.AddInstruction(expr);
  } else {
    CHECK_EQ(size_, 8);

    il.AddInstruction(il.SetRegister(
        4, reg_, il.LowPart(4, il.Register(8, LLIL_TEMP(reg_)))));
    il.AddInstruction(il.SetRegister(
        4, reg_ + 1,
        il.LowPart(4, il.LogicalShiftRight(8, il.Register(8, LLIL_TEMP(reg_)),
                                           il.Const(1, 32)))));
  }
}

However, I wasn't too happy with the results, so I didn't push the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants