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.

Sign up for GitHub

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

Jump to bottom

Incorrectly added Reverse(Top) Value due to edge case in merge logic #4245

Open
fmagin opened this issue Oct 27, 2023 · 1 comment · May be fixed by #4473
Open

Incorrectly added Reverse(Top) Value due to edge case in merge logic #4245

fmagin opened this issue Oct 27, 2023 · 1 comment · May be fixed by #4473
Labels
bug Something is broken

Comments

@fmagin
Copy link
Contributor

fmagin commented Oct 27, 2023 •
edited

Description

Assuming you have two blocks pred_1 and pred_2 that both precede some block node.
If you are loading a static address for the first time in pred_1 , SimEngineRDXVEX will retrieve the value from project.loader.memory and then store it via state.store:

angr/angr/analyses/reaching_definitions/engine_vex.py

Lines 444 to 468 in cbeace5

else:
addr_v = addr.concrete_value
# Load data from a global region
try:
vs: MultiValues = self.state.memory.load(addr_v, size=size, endness=endness)
defs = set(LiveDefinitions.extract_defs_from_mv(vs))
except SimMemoryMissingError:
try:
val = self.project.loader.memory.unpack_word(addr_v, size=size)
section = self.project.loader.find_section_containing(addr_v)
missing_atom = MemoryLocation(addr_v, size)
missing_def = Definition(missing_atom, self._external_codeloc())
if val == 0 and (not section or section.is_writable):
top = self.state.top(size * self.arch.byte_width)
v = self.state.annotate_with_def(top, missing_def)
else:
v = self.state.annotate_with_def(claripy.BVV(val, size * self.arch.byte_width), missing_def)
vs = MultiValues(v)
if not section or section.is_writable:
self.state.memory.store(addr_v, vs, size=size, endness=endness)
self.state.all_definitions.add(missing_def)
defs = {missing_def}
except KeyError:
continue

But on merging the merge code treats the address from the pred_2 state as an unconstrained_in because it was never accessed in pred_2 and so isn't part of that page in the memory state from pred_2 yet.

angr/angr/storage/memory_mixins/paged_memory/pages/mv_list_page.py

Lines 172 to 182 in cbeace5

# first get a list of all memory objects at that location, and
# all memories that don't have those bytes
for sm, fv in zip(all_pages, merge_conditions):
if sm._contains(b, page_addr):
l.info("... present in %s", fv)
for mo in sm.content_gen(b):
if mo.includes(page_addr + b):
memory_objects.append((mo, fv))
else:
l.info("... not present in %s", fv)
unconstrained_in.append((sm, fv))

This will then later mean that a Top value is generated for this location when merging the state, which is then stored in memory endianess (and thus flipped), resulting in Reverse(Top) being read from that location if this address is accessed inside node, or subsequent blocks.

Steps to reproduce the bug

I can't share the binary and code that triggers this, but I hope the error description is sufficient to understand why this bug is happening.

Environment

No response

Additional context

No response
@fmagin fmagin added bug Something is broken needs-triage Issue has yet to be looked at by a maintainer labels Oct 27, 2023
@fmagin
Copy link
Contributor Author

fmagin commented Oct 28, 2023

The straightforward fix seems to be to remove the code:

                        if not section or section.is_writable:
                            self.state.memory_definitions.store(addr_v, vs, size=size, endness=endness)
                            self.state.all_definitions.add(missing_def)

@ltfish you initially added this in 38c361e could you explain what this solves?

fmagin added a commit to fmagin/angr that referenced this issue Nov 17, 2023
@fmagin
Fix angr#4245
5837474
@ltfish ltfish removed the needs-triage Issue has yet to be looked at by a maintainer label Nov 17, 2023
fmagin added a commit to fmagin/angr that referenced this issue Nov 18, 2023
@fmagin
Fix angr#4245
e6e55e4
fmagin added a commit to fmagin/angr that referenced this issue Nov 22, 2023
@fmagin
Fix angr#4245
b3059f1
fmagin added a commit to fmagin/angr that referenced this issue Feb 1, 2024
@fmagin
Fix angr#4245
7e3732d
fmagin added a commit to fmagin/angr that referenced this issue Feb 28, 2024
@fmagin
Fix angr#4245
91f881c
@fmagin fmagin linked a pull request Feb 28, 2024 that will close this issue
Open
fmagin added a commit to fmagin/angr that referenced this issue Feb 28, 2024
@fmagin
Fix angr#4245
adac54e
fmagin added a commit to fmagin/angr that referenced this issue Mar 20, 2024
@fmagin
Fix angr#4245
63debcd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something is broken
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix #4245 fmagin/angr
2 participants
@ltfish @fmagin