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

Lifting causes spurious conditionals #4

Open
toshipiazza opened this issue Aug 16, 2021 · 5 comments
Open

Lifting causes spurious conditionals #4

toshipiazza opened this issue Aug 16, 2021 · 5 comments

Comments

@toshipiazza
Copy link

The autogenerated HLIL from this plugin generates lots of spurious code for conditional jumps, to the point where it is distracting. As I understand it, this is done for correctness, since jump targets must be resolved at the end of a packet.

Example code (test_dualjump_cond_jump() in bn_llil_test_app):

00000084  014101f3           { R1 = add(R1,R1)
00000088  06d0005c             if (P0) jump:t data_90 }
0000008c  c03f0048           { R0 = #0x0; jumpr LR }
00000090  c03f1048           { R0 = #0x1; jumpr LR }

Resulting HLIL:

00000084      char temp211 = 0
00000084      if (arg1)
00000084          temp211 = 1
00000090      if (temp211 == 1)
00000090          return 1
0000008c      return 0

I'd expect output more like

00000090      if (arg1)
00000090          return 1
0000008c      return 0

I guess this lifted IL is so situational that it doesn't make sense to ask the binja devs to optimize this particular construct... However it should be straightforward to fix it up manually in simple cases using the new Workflows API.

@toshipiazza
Copy link
Author

Out of curiosity, I converted the LLIL to pcode using a script and chucked it into Ghidra to see if Ghidra could optimize this construct, and it seems that it can

undefined4 test_dualjump_cond_jump(void) {
  bool in_P0;
  if (!in_P0) {
    return 0;
  }
  return 1;
}

So maybe it is worth sending this to the binja devs

@galenbwill
Copy link

I've commented at Vector35/binaryninja-api#2596 (comment)

@cfircohen
Copy link
Contributor

Thanks for reporting this (and for your interest), Toshi.
You're correct that the LLIL conditional jumps are needed for correctness. I haven't heard of the new BN workflow API, what's that?

@cfircohen
Copy link
Contributor

I watched BN livestream on Workflows, and, indeed, rewriting the IL post analysis is very promising.

@galenbwill
Copy link

One point of note: Workflows are only available with a commercial license of Binary Ninja.

(I have great interest in Hexagon, and was working on an unreleased Hexagon plugin for Binary Ninja before I was hired at Vector 35. This problem in particular intrigues me, as it illustrates the peculiarities of Hexagon (both the register assignment and the branching semantics) and it suggests a potential point of improvement for the Binary Ninja analysis.)

While I agree Workflows present a promising direction for "fixing" this case, I'd much rather see an improvement to the MLIL -> HLIL optimization phase that could handle it more generally.

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

3 participants