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

[TriCore] Replace one- and sign-extend with MathExtra.h functions. #2212

Merged
merged 2 commits into from
Dec 3, 2023

Conversation

Rot127
Copy link
Collaborator

@Rot127 Rot127 commented Nov 30, 2023

This fixes incorrect sign extensions and reachable aborts().

@imbillow So sorry for my sloppy review last time! Please take a look at this.

Also be so kind and run the fuzzing again with this patch:

diff --git a/cstool/cstool.c b/cstool/cstool.c
index 2240ce132..4b6b62887 100644
--- a/cstool/cstool.c
+++ b/cstool/cstool.c
@@ -607,6 +607,19 @@ int main(int argc, char **argv)
 		cs_option(handle, CS_OPT_DETAIL, CS_OPT_DETAIL_REAL);
 	}
 
+	uint32_t bytes = 0xffffffff;
+	while (address == 0xffffffff) {
+		printf("\r0x%08x\t\t", bytes);
+		fflush(stdout);
+		count = cs_disasm(handle, (uint8_t*)&bytes, 4, address, 0, &insn);
+		if (insn && insn->detail)
+			free(insn->detail);
+		free(insn);
+		bytes++;
+		if (bytes == 0xffffffff)
+			return 0;
+	}
+
 	count = cs_disasm(handle, assembly, size, address, 0, &insn);
 	if (count > 0) {
 		size_t i;

You can fuzz then with

./cstool -d tc162 0 0xffffffff

There are definitely some aborts left.

This fixes incorrect sign extensions and reachable
aborts().
@imbillow
Copy link
Contributor

imbillow commented Dec 2, 2023

Good job, I also found some abort, because I tested it with a fixed address and not 0xffffffff.

@Rot127
Copy link
Collaborator Author

Rot127 commented Dec 2, 2023

Nice! Would you cherry-pick those commits into your fixing PR?

@imbillow
Copy link
Contributor

imbillow commented Dec 2, 2023

Nice! Would you cherry-pick those commits into your fixing PR?

Yes.

The problem now is that there will obviously be some target addresses that are negative when address=0, and some target addresses > U32_MAX when address=U32_MAX. How to deal with these two cases is reasonable, do you have any comments?

@Rot127
Copy link
Collaborator Author

Rot127 commented Dec 2, 2023

If the ISA doesn't say anything about those cases, I would do a wrap around. This can be seen more easily. If you have an addr + offset jump at address 0xffffffff to 0x10 it is kinda obvious that the offset was wrapped around.

The alternative cases, rounding to 0x0/0xffffffff or decoding as invalid, are way harder to detect by the user.

@imbillow
Copy link
Contributor

imbillow commented Dec 2, 2023

@Rot127

I added the fix here,
https://github.com/imbillow/capstone/tree/tricore-fix
maybe you want cherry-pick it, and then this is the simple program I used for testing.
https://github.com/imbillow/capstone-sys-git/blob/master/capstone-sys/src/bin/cs_test_tricore.rs

@XVilka
Copy link
Contributor

XVilka commented Dec 2, 2023

I second the wraparound strategy.

@Rot127 Rot127 marked this pull request as ready for review December 2, 2023 22:57
@Rot127
Copy link
Collaborator Author

Rot127 commented Dec 2, 2023

@kabeor Could you take a look at this quickly? Without it TriCore disassembly is pretty much broken.

@XVilka
Copy link
Contributor

XVilka commented Dec 3, 2023

@imbillow do you approve this PR in its current state?

@XVilka
Copy link
Contributor

XVilka commented Dec 3, 2023

@kabeor, please merge this one.

@kabeor
Copy link
Member

kabeor commented Dec 3, 2023

Merged.

@kabeor kabeor merged commit 2fa9f60 into capstone-engine:next Dec 3, 2023
11 checks passed
@Rot127 Rot127 mentioned this pull request Dec 19, 2023
@Rot127 Rot127 deleted the tricore-abort branch October 6, 2024 14:35
Rot127 added a commit to Rot127/rizin that referenced this pull request Oct 6, 2024
Rot127 added a commit to Rot127/rizin that referenced this pull request Oct 6, 2024
Rot127 added a commit to Rot127/rizin that referenced this pull request Oct 23, 2024
Rot127 added a commit to Rot127/rizin that referenced this pull request Oct 24, 2024
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

Successfully merging this pull request may close these issues.

4 participants