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

Reworked absolute address handling (Fixes #471) #473

Merged
merged 2 commits into from
Feb 4, 2024

Conversation

mappzor
Copy link
Contributor

@mappzor mappzor commented Jan 12, 2024

This PR fixes all known issues around treating signed displacements as absolute addresses. Address size hint is now used for disambiguation where needed (rare cases that essentially force address override prefix). This also lead to a simplification of the fuzzer as verification of memory operands was always quite messy due to multiple issues that it had to deal with (signedness, 16-bit truncation, MIB, etc.). Now it's finally replaced with simpler and more robust code.

Fixes #471

@mappzor mappzor marked this pull request as draft January 12, 2024 18:34
@mappzor mappzor marked this pull request as ready for review January 16, 2024 15:15
@mappzor
Copy link
Contributor Author

mappzor commented Jan 16, 2024

In the latest set of changes I've finally dealt with the source of all address-related problems. It's this piece of code from ZydisEncoderDecodedInstructionToEncoderRequest:

if (dec_op->encoding == ZYDIS_OPERAND_ENCODING_DISP16_32_64)
{
	ZydisCalcAbsoluteAddress(instruction, dec_op, 0,
		(ZyanU64 *)&enc_op->mem.displacement);
}
else
{
	enc_op->mem.displacement = dec_op->mem.disp.has_displacement ?
		dec_op->mem.disp.value : 0;
}

It opened the gates of hell because it introduces special treatment for absolute addresses coming from moffs variants of mov instructions. So mov [address], eax could result in different encoder requests depending on which variant of mov it was ("normal" vs moffs). Dealing with this on the other end of the story (the encoder itself) resulted in convoluted processing of displacement-only and moffs operands. At some later point 16-bit workarounds were added to the mix.

Now things are done The Right Way ™️. All displacements are the same and they are handled just like in other parts of Zydis (decoder and utils like ZydisCalcAbsoluteAddress) i.e. they are 64-bit signed and interpreted in the presence of external size information. For encoder this information is now the address size hint.

This took way more time and effort than I've originally anticipated for something that initially seemed like a minor oversight but I think the changes are worth it. New behaviors are truly uniform and shield the user from all the ugly quirks of mov. Handling of none hint should be more aligned with the expectations of a typical user as well (when encoding from scratch).

Copy link
Member

@flobernd flobernd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! This looks way better from a usability perspective 🙂 Thanks a lot!

@flobernd flobernd requested a review from athre0z January 26, 2024 08:32
Copy link
Member

@athre0z athre0z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from a coarse glance! Apologies for the delay: I didn't find the time to review this properly. Realistically you know this piece of code better than both Florian and me anyway, so let's not delay this any further. :)

@athre0z athre0z merged commit df34a98 into zyantific:master Feb 4, 2024
13 checks passed
@mappzor mappzor deleted the abs_addr branch February 4, 2024 22:05
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.

Encoder mishandles 16-bit address truncation behavior
3 participants