Fix Inconsistent ROP gadget info between detail printing and listing

[MrQuantum1915]

Your checklist for this pull request

• I've read the guidelines for contributing to this repository.
• I made sure to follow the project's coding style.
• I've documented every RZ_API function and struct this PR changes.
• I've added tests that prove my changes are effective (required for changes to RZ_API).
• I've updated the Rizin book with the relevant information (if needed).
• I've used AI tools to generate fully or partially these code changes and I'm sure the changes are not copyrighted by somebody else.

Detailed description

This PR, closes issue #5491 about inconsistant output of Listing gadgets vs Detailed info printing of gadgets .

Tracing the bug

When we run /R' it lists a gadgets upto 0x00002d2cbut in/Rgit only outputs till0x000020e0`. (#5491)
The count of gadgets reduces :

rizin -A test/bins/arm/crackme.arm32.bin
[0x00000000]> /R~Gadget~?
745
[0x00000000]> /Rg~Gadget~?
186

When we run /R and see output at 0x000020e0, it has ret instruction (return)

  0x000020b0             c26090  ret 0x9060
Gadget size: 3

  0x000020e0             ca6003  ret far 0x360
Gadget size: 3

And it correctly get listed in /Rg also.

Gadget 0x20ae
Stack change: 0x9068
Changed registers: rcx rsp 
Register dependencies:
Var Read: rcx

Gadget 0x20b0
Stack change: 0x9068
Changed registers: rsp 
Register dependencies:

Gadget 0x20e0
Stack change: 0x368
Changed registers: rsp 
Register dependencies:

So seeing instruction at 0x00002d2c we see that it has call instruction.

  0x00002d2a               0020  add byte [rax], ah
  0x00002d2c         e8070020f0  call 0xfffffffff0202d38
Gadget size: 7

  0x00002d2c         e8070020f0  call 0xfffffffff0202d38
Gadget size: 5

I confirmed this behavior for other gadgets too.
So the problem is that in /Rg analysis only gadgets having ret instruction are getting listed.

Now tracing the source code:
Rg is handled by rz_cmd_detail_gadget_handler

the call Hierarchy gets us to perform_gadget_analysis

In the perform_gadget_analysis function on line 1408, it explicitly checks for ret gadget!:

	if (!is_ret_gadget(core, hit_last, crop)) {
		return rop_gadget_info;
	}

now /R is handled by rz_cmd_info_gadget_handler, it searches for rop and computes end_gadget_list

In compute_end_gadget_list on line 1519, it checks for end_gadget_list, instead of ret_gadget in /Rg:

if (is_end_gadget(&end_gadget, context->crop)) {

NOw in is_end_gadget function, it includes many more instructions along with ret.

Hence /R shows more gadgets compared to /Rg because the /Rg drops majority of gadgets in analysis phase.

---

/Rg should analyse all the gadgets in detail instead of filtering out the ones without ret instructio, because after all the result of /R and /Rg should converge to a single truth.

---

Firstly I tried to replace is_ret_gadget function by calling is_end_gadget in is_end_gadget_hit` wrapper function :

static bool is_end_gadget_hit(const RzCore *core, const RzCoreAsmHit *hit, const ut8 crop) {
	rz_return_val_if_fail(core && core->analysis && hit, false);
	bool status = false;
	RzAnalysisOp aop = { 0 };
	ut8 *buf = malloc(hit->len);
	if (rz_io_nread_at(core->io, hit->addr, buf, hit->len) < 0) {
		free(buf);
		return status;
	}
	rz_analysis_op_init(&aop);
	if (rz_analysis_op(core->analysis, &aop, hit->addr, buf, hit->len, RZ_ANALYSIS_OP_MASK_DISASM) < 0) {
		free(buf);
		return status;
	}
	status = is_end_gadget(&aop, crop);
	rz_analysis_op_fini(&aop);
	free(buf);
	return status;
}

For ARM binary the result is OK,

rizin -A test/bins/arm/crackme.arm32.bin
[0x00000000]> /Rg~Gadget~?
738
[0x00000000]> /R~Gadget~?
738

NOTE: that running detailed analysis reduced gadgets from 745 to 738 because initially aaa pipeline is does not emulate every path, whereas /Rg runs IL emulation on every gadget, and hence it discards some gadgets due to unreachanble code etc..

But for MIPS binary it still breaks:

rizin -A test/bins/elf/analysis/mips64r2-busybox
[0x120004440]> /Rg~Gadget~?
104
[0x120004440]> /R~Gadget~?
68198

Running /R shows that many gadgets end with nop no operation (delay slot) , which is not covered in is_end_gadget check:

❯ rizin -A test/bins/elf/analysis/mips64r2-busybox
[0x120004440]> /R | tail -n 20
  0x1200ed414           00200300  sll a0, v1, 0
Gadget size: 12

  0x1200ed410           08000000  jr zero
  0x1200ed414           00200300  sll a0, v1, 0
Gadget size: 8

  0x1200ee0dc           00000000  nop
  0x1200ee0e0           805b0b20  addi t3, zero, 0x5b80
  0x1200ee0e4           01000000  movf zero, zero, fcc0
  0x1200ee0e8           08000000  jr zero
  0x1200ee0ec           00000000  nop
Gadget size: 20

  0x1200ee0e0           805b0b20  addi t3, zero, 0x5b80
  0x1200ee0e4           01000000  movf zero, zero, fcc0
  0x1200ee0e8           08000000  jr zero
  0x1200ee0ec           00000000  nop
Gadget size: 16

But if you check compute_end_gadget_list function at line 1522, it clearly handles the delay slot case and hence /R shows that gadgets , whereas our fix of calling only is_end_gadget does not handle delay slot explicitly.

Actually doing is_end_gadget check is redundant.

Because the hitlist passed to perform_gadget_analysis functions is generated by construct_rop_gadget function. And this function uses the end_list (pre-computed by compute_end_gadget_list) which identifies all valid terminators (RET, CALL, JMP, etc) and correctly handles delay slots as seen above.

The valid flag only becomes true when the gadget ends exactly at one of these pre-validated terminators.

So doing is_end_gadget() check inside perform_gadget_analysis function is REDUNDANT.

FINAL FIX

The most minimal and correct fix is to remove the check entirely. We trust the search engine to only deliver valid gadgets for analysis. This unifies the logic and ensures that /Rg, /Rk, /Rs, and /Rl all work consistently across all architectures. And as seen in Test plan section below, the outputs of /R and /Rg converges to a single truth!

NOTE: Also as now /Rg lists so many more gadgets that were ignored previously, I have to change the expected outputs for many tests in rop regression test cmd_rop using -i interactive mode.

...

Test plan

After this PR, the values of /R and /Rg would converge instead of showing different outputs as in #5491

rizin -A test/bins/arm/crackme.arm32.bin
[0x00000000]> /Rg~Gadget~?
738
[0x00000000]> /R~Gadget~?
738

NOTE: As now /Rg has to emulate many more gadgets this test will take some time to finish, because they are 68192 gadgets!!

rizin -A test/bins/elf/analysis/mips64r2-busybox
[0x120004440]> /Rg~Gadget~?
68192
[0x120004440]> /R~Gadget~?
68192
[0x120004440]> 

...

Closing issues

closes #5491
...

[wargio]

i'm kinda sure that calls are not ret.

For me these changes are wrong.

I would expect for rop generator to sequences where you have return or jump registers, but not call types

@notxvilka

[notxvilka]

i'm kinda sure that calls are not ret. For me these changes are wrong.

I would expect for rop generator to sequences where you have return or jump registers, but not call types

@notxvilka

Yes, you are right

[MrQuantum1915]

i'm kinda sure that calls are not ret. For me these changes are wrong.

I would expect for rop generator to sequences where you have return or jump registers, but not call types

@notxvilka

@wargio @notxvilka

yeah.
ROP sequence dont use call instructions. Its used in COP (call oriented).

So this means current rop search engine which accepts several call types should be more restrictive, it should not return true for COP related instructions:

static bool is_end_gadget(const RzAnalysisOp *aop, const ut8 crop) {
	if (aop->family == RZ_ANALYSIS_OP_FAMILY_SECURITY) {
		return false;
	}
	switch (aop->type) {
	case RZ_ANALYSIS_OP_TYPE_TRAP:
	case RZ_ANALYSIS_OP_TYPE_RET:
	case RZ_ANALYSIS_OP_TYPE_UCALL:
	case RZ_ANALYSIS_OP_TYPE_RCALL:
	case RZ_ANALYSIS_OP_TYPE_ICALL:
	case RZ_ANALYSIS_OP_TYPE_IRCALL:
	case RZ_ANALYSIS_OP_TYPE_UJMP:
	case RZ_ANALYSIS_OP_TYPE_RJMP:
	case RZ_ANALYSIS_OP_TYPE_IJMP:
	case RZ_ANALYSIS_OP_TYPE_IRJMP:
	case RZ_ANALYSIS_OP_TYPE_JMP:
	case RZ_ANALYSIS_OP_TYPE_CALL:
		if (crop) {
			return is_cond_end_gadget(aop);
		}
		return true;
	default:
		return false;
	}
}

ANd hence both /R and /Rg consider call as valid terminators.

To fix this properly, I suggest we should fix the root of the problem in the search engine itself.
I suggest to still keep the removal of the is_ret_gadget check in /Rg in my PR, because otherwise it will also accidentally blocks valid jump gadgets like MIPS jr $ra

what do you prefer ?

1. remove direct call and direct jump types (direct address) from is_end_gadget(). The engine will now search for return and jump registers. we keep indirect ones.
2. make config: remove them by default, but leave an option to enable them for COP?

[Rot127]

accidentally blocks valid jump gadgets like MIPS jr $ra

Shouldn't this be fixed in analysis_mips.c then?

[MrQuantum1915]

accidentally blocks valid jump gadgets like MIPS jr $ra

Shouldn't this be fixed in analysis_mips.c then?

@Rot127

thanks!!!
for pointing out that! it helped me find root cause and some other issues in that file

the gadget with jr $ra instruction were shown in /R but not in /Rg.
and as you pointed out, this should have been promoted to ret by the analysis phase, but it is NOT.

I checked the type of one of instruction:

the type is rjmp instead of ret

---

i checked analysis_mips_cs.c file, in the analyse function:

		// register is $ra, so jmp is a return
		if (insn->detail->mips.operands[0].reg == MIPS_REG_RA) {
			op->type = RZ_ANALYSIS_OP_TYPE_RET;
			ctx->t9_pre = UT64_MAX;
		}

so the reason is that the enum is not matching.
I confirmed it , the value of MIPS_REG_RA is 22, while our binary has 331 (just added fprintf):

And 331 belongs to MIPS_REG_RA_64 in Capstone's mips.h file

Our test binary was mips64r2-busybox : 64 bit!

22 is for 32 bit

adding a check for MIPS_REG_RA_64 solved the bug of jr $ra not being promoted to ret.

---

This is a mistake because for Capstone version >=6 (which split 32bit and 64 bit reg classes), we should compare it with 64 bit's enum not 32 bit.

Actually I analysed whole file to see any other such mistake, i found a few other blind spots where we are missing these modern Capstone checks. (NOTE: in somecases we correctly check for 64bit like line 391, but in other we are missing the check, which can actually break analyses if we test for 64bit mips binary):

List:

1. Current $ra check
2. Line 468 : ADDIU
3. Line 471 : ADDIU for stack tracking
4. Line 546: ANDI for stack alignment

Also Capstone version 6 introduced nanoMIPS, so I think we should add _NM checks too for CS_NEXT_VERSION>=6, like MIPS_REG_GP_NM=12, etc...

---

If you agree, i would like to open a NEW PR to fix mips analysis file with this fixes. This will automatically fix issue of jr $ra being not promoted to ret.

Also how would you prefer I handle the Capstone v6 enums?

1. Just append the _64 and _NM checks using explicit || logic wrapped in #if CS_NEXT_VERSION >= 6 at the places they are missing.

2. abstract it by adding centralized macros (like RZ_MIPS_IS_RA(reg)) to hide the Capstone versioning checks? Though the only benefit would be avoiding future such mistakes of missing _64 _Nm checks i guess.

[MrQuantum1915]

@Rot127 @wargio @notxvilka

[Ignoring my changes in PR]

Regardless of my previous comment on analysis_mips_cs.c issue, the /Rg will still fail for MIPS because of this line in perform_gadget _analysis functin rop.c:

	const RzCoreAsmHit *hit_last = (RzCoreAsmHit *)rz_list_last_val(hitlist);
	if (!is_ret_gadget(core, hit_last, crop)) {
		return rop_gadget_info;
	}

MIPS gadget has jr $ra as SECOND LAST instruciton followed by delay slot like nop, daddiu sp, sp, 0x90, etc...

But currently we are only checking if LAST instruction is ret or not. We should also check whether LAST SECOND is ret or not like this :

	const RzCoreAsmHit *hit_last = (RzCoreAsmHit *)rz_list_last_val(hitlist);
	const RzCoreAsmHit *hit_prev = (RzCoreAsmHit *)rz_list_get_n(hitlist, rz_list_length(hitlist) - 2);
	if (!is_ret_gadget(core, hit_last, crop)) {
		if (!hit_prev || !is_ret_gadget(core, hit_prev, crop)) {
			return rop_gadget_info;
		}
	}

NOTE: this is just temp fix, like using rz_list_get_n will be O(N) in linked list, we can optimise that part, and also we can only trigger second check only when there is delay slot , to avoid false positives.

After this change /Rg correctly identifies and analyse MIPS gadgets correctly.

---

Also we need to decide now whether /R showing more gadgets then /Rg by including COP related instructions, is EXPECTED behaviour or not.

• If we want /R to list more gadgets which also have call instructions, then the Original Issue of inconsistent rop info becomes Invalid. And this would mean /R is intended to be a broader raw search than the strict /Rg analysis.

• If this is NOT expected than we should remove call type from valid end_gadget check as discussed in my prior comment.

[notxvilka]

A good point. JOP and COP are sometimes useful but quite niche, thus calls should not be a part of the default ROP chain listing. I suggest moving them into a separate command probably. Maybe part of the new /J command?

[MrQuantum1915]

A good point. JOP and COP are sometimes useful but quite niche, thus calls should not be a part of the default ROP chain listing. I suggest moving them into a separate command probably. Maybe part of the new /J command?

@notxvilka

hmm...makes sense.

So for ROP we should remove JMP and CALL types from checks.

static bool is_end_gadget(const RzAnalysisOp *aop, const ut8 crop) {
	if (aop->family == RZ_ANALYSIS_OP_FAMILY_SECURITY) {
		return false;
	}
	switch (aop->type) {
	case RZ_ANALYSIS_OP_TYPE_TRAP:
	case RZ_ANALYSIS_OP_TYPE_RET:
	case RZ_ANALYSIS_OP_TYPE_UCALL:
	case RZ_ANALYSIS_OP_TYPE_RCALL:
	case RZ_ANALYSIS_OP_TYPE_ICALL:
	case RZ_ANALYSIS_OP_TYPE_IRCALL:
	case RZ_ANALYSIS_OP_TYPE_UJMP:
	case RZ_ANALYSIS_OP_TYPE_RJMP:
	case RZ_ANALYSIS_OP_TYPE_IJMP:
	case RZ_ANALYSIS_OP_TYPE_IRJMP:
	case RZ_ANALYSIS_OP_TYPE_JMP:
	case RZ_ANALYSIS_OP_TYPE_CALL:
		if (crop) {
			return is_cond_end_gadget(aop);
		}
		return true;
	default:
		return false;
	}
}

For ROP we should keep:

case RZ_ANALYSIS_OP_TYPE_TRAP:
case RZ_ANALYSIS_OP_TYPE_RET:

For COP:

case RZ_ANALYSIS_OP_TYPE_UCALL:
case RZ_ANALYSIS_OP_TYPE_RCALL:
case RZ_ANALYSIS_OP_TYPE_ICALL:
case RZ_ANALYSIS_OP_TYPE_IRCALL:
case RZ_ANALYSIS_OP_TYPE_CALL:

For JOP:

case RZ_ANALYSIS_OP_TYPE_UJMP:
case RZ_ANALYSIS_OP_TYPE_RJMP:
case RZ_ANALYSIS_OP_TYPE_IJMP:
case RZ_ANALYSIS_OP_TYPE_IRJMP:
case RZ_ANALYSIS_OP_TYPE_JMP:

We can then use them in their respective check

IMP: By doing so I am assuming that specific architecture analysis plugins correctly promote their specific return idioms (like MIPS jr $ra or ARM bx lr) to RZ_ANALYSIS_OP_TYPE_RET. As I discussed in earlier comment to @Rot127 ...

We can implement this in a new jop.c:

/J : List all indirect branch gadgets (both JOP and COP).

/Jj: List only pure JOP gadgets (ends in jmp).

/Jc: List only COP gadgets (ends in call).

---

• So for THIS PR: i will remove JOP and COP related checks from is_end_gadget function (fixing inconsistency between /R and /Rg), and also FIX to also check the second last instruction for delay slot arch (fixing /Rg for MIPS) as discussed above...

• And I will open a NEW PR, for implementing new /J command for JOP and COP.

Does this make sense? Should I proceed?

[notxvilka]

• So for THIS PR: i will remove JOP and COP related checks from is_end_gadget function (fixing inconsistency between /R and /Rg), and also FIX to also check the second last instruction for delay slot arch (fixing /Rg for MIPS) as discussed above...

• And I will open a NEW PR, for implementing new /J command for JOP and COP.

Yes, exactly. Be sure to mimic /J output and subcommands to the corresponding /R output and subcommands. Maybe some code can be shared between two (printing functions, etc).