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

Test each and every modr/m value #6

Open
superfury opened this issue Jun 8, 2018 · 6 comments
Open

Test each and every modr/m value #6

superfury opened this issue Jun 8, 2018 · 6 comments

Comments

@superfury
Copy link

superfury commented Jun 8, 2018

I see that not all modr/m registers and r/m abd sib bits are validated? Can it be extended to test each set of bits for validity as well as special combinations with the modr/m and sib bytes(32-bit mode)? so testing all mod+reg, mod+rm, scale, index and all base values for valid results(and exceptions to the default rules on them, like with (e)sp/(e)bp)?

@superfury
Copy link
Author

I've found some problems with the 32-bit ModR/M value, in particular problems with handling and calculating the offset from the 32-bit displacement-only mode using the SIB byte. It was doubling the displacement by applying it at both the index and base calculations, adding them together afterwards. I've also reordered the whole SIB memory operand calculations: the base now uses either a register or 0(for EBP with MOD=0), while the index is followed and added with the displacement, while ESP as an index just gives the displacement instead(not adding anything to the opcode. So it goes from the earlier DISPLACEMENT+0 to DISPLACEMENT(instead of calculating DISPLACEMENT*2 by adding it twice) in the EBP+MOD=0 case, while BASE+DISPLACEMENT is now generated for the pure displacement mode with index ESP, all cases should now work properly).

@superfury
Copy link
Author

superfury commented Jun 12, 2018

548-pinball illusions no longer crashing immediately

It's continuing without crashing now, with that latest modr/m fix! :D

548-pinball illusions loading

@superfury
Copy link
Author

I've found even more ModR/M offset problems: the 16-bit memory writes through the BIU were using the ModR/M parameter object(implicit C object to uint_32 conversion) instead of the proper 16/32-bit address that's stored in a variable within it. The same applies to 16-bit and 32-bit modr/m memory checking(protection checks).

@superfury
Copy link
Author

Another not-so-obvious bug found: EBP as an index(specified using SIB index bits) defaults to DS, not SS. That was even a bug in the capstone disassembler. Maybe even NASM(and it's optimization of EBP+EBP(1) using it incorrectly, which uses SS by default, but the 'optimized' form(EBP2) uses SS instead) still. That will cause trouble with segmented memory where DS!=SS.

@barotto
Copy link
Owner

barotto commented Aug 17, 2018

Thanks for the heads up. I've added a better tester for 32-bit addressing (ModRM + SIB). It's still missing a test for the default segment tho.

@superfury superfury reopened this Jan 19, 2019
@superfury
Copy link
Author

Afaik there's still no default segment checks for the various memory operands? ModR/M seems to be running fine.

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

2 participants