uc_context: validate save/restore against content flags and engine#2322
Open
MarkLee131 wants to merge 2 commits into
Open
uc_context: validate save/restore against content flags and engine#2322MarkLee131 wants to merge 2 commits into
MarkLee131 wants to merge 2 commits into
Conversation
Contributor
|
I would prefer to save |
Two failure modes used to abort or corrupt the host process:
1. uc_context_alloc() returned a heap allocation with only fv
explicitly zeroed; snapshot_level / ramblock_freed / last_block
stayed garbage. uc_context_restore() then trusted those fields
and tripped the cpu_asidx_from_attrs() assertion in cpu.h on
the next emu_start().
2. uc_context_restore() trusted fv / last_block from the
user-visible struct without checking they came from a
uc_context_save() on the same engine; cross-engine restores
dereferenced live host pointers belonging to a different
uc_engine.
Three changes here:
- uc_context_alloc() switches to g_malloc0() so untouched header
fields read 0.
- struct uc_context grows a context_content field set by
uc_context_save() to mirror uc->context_content, and an engine
pointer set only when UC_CTL_CONTEXT_MEMORY is included.
uc_context_restore() refuses contexts with context_content == 0
(covers unicorn-engine#2319), with arch/mode that doesn't match the destination
engine, or with an engine pointer mismatch on a memory restore
(covers unicorn-engine#2320). Each restore branch is gated on both sides
advertising the matching content bit, so a CPU-only context
can't be replayed as a memory snapshot.
- uc_context_reg_*() refuses memory-only contexts via
context_content, since their data buffer holds no CPU state.
Adds three regression tests in tests/unit/test_ctl.c:
restore-without-save, cross-engine restore (CPU-only portable,
memory-mode rejected), and reg-read/write on a memory-only snapshot.
Closes part of unicorn-engine#1766. Fixes unicorn-engine#2319. Fixes unicorn-engine#2320.
MarkLee131
force-pushed
the
fix/issue-2319-context-alloc-zero-init
branch
from
db67d17 to
b1badf9
Compare
MarkLee131
changed the title
Author
Comment on lines
+2475
to
+2484
| // Refuse register r/w on memory-only contexts: their data buffer | ||
| // holds no CPU state, so the call would just shuffle zeros into the | ||
| // caller's value (read) or be lost on the next restore (write). | ||
| // Untouched contexts (context_content == 0) are left alone, so the | ||
| // uc_context_alloc + uc_context_reg_write + uc_context_save pattern | ||
| // keeps working. | ||
| static inline uc_err uc_context_check_cpu_state(const uc_context *ctx) | ||
| { | ||
| if ((ctx->context_content & UC_CTL_CONTEXT_MEMORY) && | ||
| !(ctx->context_content & UC_CTL_CONTEXT_CPU)) { |
Contributor
There was a problem hiding this comment.
I don't understand what you want to achieve with this. When we refuse to restore a context which was never saved writing to a register of the context is doing nothing. So why allow this?
Author
There was a problem hiding this comment.
Carve-out was for alloc + reg_write + save, but uc_context_save overwrites the buffer from the engine so the reg_write gets clobbered anyway. Tightened to require UC_CTL_CONTEXT_CPU, symmetric with restore. Test extended.
Previously the check only refused contexts with MEMORY set and CPU unset; never-saved contexts (context_content == 0) were left alone on the assumption that an alloc + reg_write + save pattern might exist. uc_context_save() overwrites the data buffer from the engine though, so any reg_write before save would be clobbered, and there is no real caller for that path. Reject anything without UC_CTL_CONTEXT_CPU set, symmetric with uc_context_restore() refusing context_content == 0. Extends the unicorn-engine#2319 regression test to assert uc_context_reg_read / uc_context_reg_write also reject a never-saved context.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two failure modes in
uc_context_save/uc_context_restorecould abort or corrupt the host process.uc_context_alloc()returned a heap allocation with onlyfvexplicitly zeroed;snapshot_level/ramblock_freed/last_blockstayed garbage. A strayuc_context_restore()before anyuc_context_save()propagated thoseinto the engine and tripped the
cpu_asidx_from_attrs()assertion incpu.hon the nextemu_start().uc_context_restore()trustedfv/last_blockfrom the user-visible struct without checking they came from auc_context_save()on the same engine. Cross-engine restores dereferenced live host pointers belonging to adifferent
uc_engine.Changes:
uc_context_alloc()switches tog_malloc0()so untouched header fields read 0.struct uc_contextgrows acontext_contentfield set byuc_context_save()mirroringuc->context_content, and anenginepointer set only whenUC_CTL_CONTEXT_MEMORYis included.uc_context_restore()refuses contexts withcontext_content == 0(coversuc_context_allocleaves header fields uninitialized; misuse aborts the host #2319), arch/mode mismatching the destination engine, or an engine pointer mismatch on a memory restore (coversuc_context_restorereads back fields from a struct the user owns; three crash classes follow #2320). Each restore branch is gated on both sides advertising the matching content bit, soa CPU-only context cannot be replayed as a memory snapshot.
uc_context_reg_*()refuses memory-only contexts (context_content & MEMORYwithoutCPU), since their data buffer holds no CPU state.Approach follows the review feedback on this PR and on #2320.
Regression tests in
tests/unit/test_ctl.c:UC_ERR_ARG, engine still usableuc_context_reg_read/_reg_writeon memory-only snapshot returnsUC_ERR_ARGCloses part of #1766. Fixes #2319. Fixes #2320. Supersedes #2325.