avast / avast/retdec

[Capstone2LlvmIr] (ARM architecture) Incorrect load instruction handling and unhandled control flow instructions

Open
#737 1 comment 0 reactions 1 assignee Claimed by @PeterMatula View on GitHub
C-capstone2llvmir
Dominant language
C++
Stars
8.6k
Forks
1k
PR merge metrics
No merged PRs in 30d

Description

Dear, Retdec developers.

I checked only ARM Thumb mode instructions.
So, not sure about the other instructions.
Please check whether you miss those bugs in your TODO list.

# Incorrectly handled load instruction (in Capstone2LlvmIr module)
In ARM architecture, the data can be placed between functions.
To load data between two functions, ARM thumb mode utilizes PC-relative instructions to compute the data address.

In the retdec, when load instructions are handled in "Capstone2LlvmIrTranslatorArm_impl::loadOp", it handles "ARM_OP_MEM" type instruction.
Then, it computes such PC-relative data address.
This address computation consists of two steps in both ARM manual and Retdec (reference: https://github.com/avast/retdec/blob/master/src/capstone2llvmir/arm/arm.cpp#L405)

1) Get the "baseR" which is the PC register value
2) Get the offset value (in retdec, it is called "disp(lacement)").
However, during this step, PC register should be computed as follows.
"((PC >> 2) << 2) +4 "
This is because some instruction's address is ended with "2".
However, this computation is wrong.

In the "llvm::Value* Capstone2LlvmIrTranslatorArm_impl::getCurrentPc(cs_insn* i)" function, it is computed with ((i->address + (2*i->size)) >> 2) << 2); where i->address is instruction's addres and i->size is instruction's size.

I think the above implementation only considered instructions whose sizes are 2 - not 4. Therefore, some instruction whose size is 4 will be doubled.
As a result, it references the correct data reference address +4 (because (2*i->size) will be 8 with the original implementation)
I observed this bug with "vldr (capstone inst ID 311)" and "ldr (capstone inst ID 75)" instructions (whose instruction sizes are 4).
In order to rectify it, the following code should be modified in https://github.com/avast/retdec/blob/master/src/capstone2llvmir/arm/arm.cpp#L164
as follows:
((i->address + (2 * i->size)) >> 2) << 2);
-> (((i->address >> 2) << 2) + 4)); // 2 * i->size is replaced with "+ 4"

I think this problem should be prioritized since the data can be placed between functions.
In your design, Retdec is designed to remove such "code misunderstood as data" from the disassembly target. Those misunderstood codes are usually placed at the start or end of the function. Thus, it cause wrong disassembly issues such as the function entry part is not included in the disassembly target.

NOTE: I think it should be correct in general. However, I am not sure the other instructions.

# TBB, TBH
Both instructions are related to branch instructions.
Specifically, they are "switch" statement in C and C++.
However, it is considered as just some instruction as follows:
call void @__asm_tbh(i32 %v4_800fe9a)

Hence, it should be handled to recover the control flows correctly.

Thank you

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.