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

[AARCH32] Incorrect Implementation of Carry Flag Behavior #7459

Open
f-raZ0R opened this issue Feb 8, 2025 · 10 comments
Open

[AARCH32] Incorrect Implementation of Carry Flag Behavior #7459

f-raZ0R opened this issue Feb 8, 2025 · 10 comments
Assignees
Labels
Status: Waiting on customer Waiting for customer feedback

Comments

@f-raZ0R
Copy link

f-raZ0R commented Feb 8, 2025

Describe the bug
ARM has unintuitive behavior involving the carry flag, where it behaves differently with subtraction operators. This behavior is not replicated by the decompiler, at least for ARMv6, causing incorrect output.

To Reproduce
Steps to reproduce the behavior:

  1. Go find some instance of a cmp directly followed by a bcs, for example.
  2. Notice that the condition is inverted. Wuh-oh, this shouldn't happen...

Expected behavior
Per https://developer.arm.com/documentation/dui0801/l/Condition-Codes/Carry-flag?lang=en , the behavior of the carry flag is inverted on subtraction, being set to 1 if the operation didn't underflow.

Environment (please complete the following information):

  • OS:Windows 10
  • Java Version: N/A
  • Ghidra Version: 11.3
  • Ghidra Origin: Github

Additional context
I am not sure what other ARM versions this is applicable to, but I know that this behavior is incorrectly implemented for ARMv6.
This is a pretty major issue, seeing how the carry flag is used in a lot of operations, causing a lot of code to be incorrectly decompiled, often times resulting in code being mistakenly marked as "unreachable".

@ghidracadabra ghidracadabra self-assigned this Feb 11, 2025
@ghidracadabra ghidracadabra added the Status: Triage Information is being gathered label Feb 11, 2025
@ghidracadabra
Copy link
Contributor

There are several variants of those instructions. Can you find a problematic instance, right-click on each instruction and select Instruction Info, then paste the contents of the Constructor Line #'s section?

@ghidracadabra ghidracadabra added Status: Waiting on customer Waiting for customer feedback and removed Status: Triage Information is being gathered labels Feb 11, 2025
@f-raZ0R
Copy link
Author

f-raZ0R commented Feb 12, 2025

 mov(ARMinstructions.sinc:3481), COND(ARMinstructions.sinc:658), 
 SBIT_CZNO(ARMinstructions.sinc:666), shift1(ARMinstructions.sinc:697)

 instruction(ARMinstructions.sinc:1533), 
 cpy(ARMinstructions.sinc:2583), COND(ARMinstructions.sinc:658), 
 rm(ARMinstructions.sinc:637)

 instruction(ARMinstructions.sinc:1533), 
 cmp(ARMinstructions.sinc:2543), COND(ARMinstructions.sinc:658), 
 rn(ARMinstructions.sinc:634), shift2(ARMinstructions.sinc:816), 
 rm(ARMinstructions.sinc:637)

 instruction(ARMinstructions.sinc:1533), 
 b(ARMinstructions.sinc:2173), cc(ARMinstructions.sinc:644), 
 Addr24(ARMinstructions.sinc:671) 

the decompiler's interpretation of this seems to be inverted, compared to the docs. The decompiler thinks this is an if(false), when it's actually essentially an if(true), to my understanding?

@f-raZ0R
Copy link
Author

f-raZ0R commented Feb 12, 2025

the r0 register is the input parameter here, it is what is being compared

@f-raZ0R
Copy link
Author

f-raZ0R commented Feb 12, 2025

mov r6,#0x0
cpy r1,r6
cmp param_1,r1
bcs LAB_[EXACT POINTER IS NOT RELEVANT HERE]
here’s what said instructions are shown as

@GhidorahRex
Copy link
Collaborator

Can you include the bytes too, please?

@f-raZ0R
Copy link
Author

f-raZ0R commented Feb 12, 2025

00 60 a0 e3
06 10 a0 e1
01 00 50 e1
05 00 00 2a

@ghidracadabra
Copy link
Contributor

(assuming you meant 01 00 50 e1 for the bytes of the third instruction).

I think this might be related to how the code for the cmp instruction was implemented. You can see the details here

Alternatively, in the Listing, you can edit the fields (click on the icon in the toolbar that looks like a brick wall over a down arrow) and enable the "Pcode" field.

The gist is that the carry flag is set based on r1 <= r0 for the subtraction r0 - r1. Assuming again that you've turned off "Eliminate unreachable code" in the decompiler options, any "true" or "false" you see is based an inequality whose inputs are flipped from their order in the subtraction. So the decompiled code might be a bit confusing but should accurately reflect the assembly. Let us know if it doesn't.

@f-raZ0R
Copy link
Author

f-raZ0R commented Feb 13, 2025

Here r1 is set to 0 though, so shouldn't r0 >=r1 always be true? how come it's making an if(false) instead?

@f-raZ0R
Copy link
Author

f-raZ0R commented Feb 13, 2025

Image

Image
screenshot of decompiled code, compared to the bytes, the decompiled output feels very nonsensical to me

@f-raZ0R
Copy link
Author

f-raZ0R commented Feb 13, 2025

Oh wait, hmm. Seems like it.. would in fact never trigger, as it'd always jump?
Is this a bug with the code i'm analyzing itself, then?

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

No branches or pull requests

3 participants