Remove deprecated accessed_flags flags field from ZydisDecodedInstruction

[flobernd]

Removes a bunch of deprecated code related to accessed cpu flags.

In the current state we lost fine granular information about FPU flag actions:

• TESTED, TESTED_MODIFIED -> READ
• MODIFIED, TESTED_MODIFIED SET_0, SET_1, UNDEFINED -> WRITTEN

I'm planning to bring back this functionality in a follow up commit.

[athre0z]

Hmm. I seem to remember that we decided that there is precisely zero value in being able to distinguish set 1, set 0 and undefined from write for any use-case that we could think of, unless we also provide full semantic information. What changed?

[flobernd]

The only drawback including the extra info would be a slightly larger binary (maybe 1KiB).

I personally would be fine keeping only the cpu_flags_read/written, but at the same time I'm wondering if any user possibly uses the advanced info for anything.

[ZehMatt]

I personally think having more granular detail would be desired, I was a bit surprised that it got reduced to the two fields only.

[flobernd]

I think there were some instructions for which the affected flags change depending on semantics. But these were only like 1-2 exceptions for which we defined the "worst case" set.

[athre0z]

I personally think having more granular detail would be desired, I was a bit surprised that it got reduced to the two fields only.

Interesting. Could you perhaps provide some insights in how you'd profit from that in any real world project? Note that there are only 81 instructions that use SET_0 and 3 that use SET_1, which are the two cases where I could imagine that someone might build assumptions on it for e.g. an optimizer. For UNDEFINED, any optimizer would always have to assume that the flag might be tainted, which is essentially exactly the same as WRITE.

On the other hand, with the new design, users will now have to inspect (or OR together) the 4 different write flags in order to determine if any sort of write occurred.

Corresponding instruction DB queries

$ jq '.[] | select(.affected_flags and any(.affected_flags[] | values; . == "0")) | .mnemonic' instructions.json | sort | uniq | wc -l
81

$ jq '.[] | select(.affected_flags and any(.affected_flags[] | values; . == "1")) | .mnemonic' instructions.json | sort | uniq | wc -l
1

[flobernd]

On the other hand, with the new design, users will now have to inspect (or OR together) the 4 different write flags in order to determine if any sort of write occurred

With the old design before the flags_read/written masks, it was even worse tho 😃

Maybe we can go for a compromise and keep the flags_read/written while adding the other mask as additional info.

[ZehMatt]

I personally think having more granular detail would be desired, I was a bit surprised that it got reduced to the two fields only.

Interesting. Could you perhaps provide some insights in how you'd profit from that in any real world project? Note that there are only 81 instructions that use SET_0 and 3 that use SET_1, which are the two cases where I could imagine that someone might build assumptions on it for e.g. an optimizer. For UNDEFINED, any optimizer would always have to assume that the flag might be tainted, which is essentially exactly the same as WRITE.

Well I would go with modified, consumed, undefined. As for the undefined flags there are quite interesting uses in obfuscations, so allowing the analysis to spot use of undefined flags without specifically checking for the instruction makes it a bit easier. The interesting thing about those undefined flags is that they are usually concrete per CPU and just undefined in a general sense.

[athre0z]

I started a poll about this on Twitter yesterday and also asked around in some Discord communities. While nobody I talked to seemed to have a particularly strong opinion about it, it seems like quite a few people have previously profited from having this distinction in one way or another. Looking at this data, I'm going to pull an 180 on my previous position and suggest that we just keep things as implemented in this PR. :)

[flobernd]

I started a poll about this on Twitter yesterday and also asked around in some Discord communities.

Thanks for taking care 👍

[athre0z]

LGTM! Sorry for taking so long to review.

[flobernd]

LGTM! Sorry for taking so long to review.

No worries at all! Thanks.