DynamoRIO / DynamoRIO/dynamorio

Operand disassembly can crash when fragment is incomplete

Open
#1,710 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
C
Stars
3.2k
Forks
629
Avg merge
2d 18h
Merged PRs (30d)
30

Description

With some logging options (I haven't characterized exactly which), print_known_pc_target gets called and will find a target fragment that is a valid fragment but whose linkstubs aren't (yet) set up with the last linkstub marked with LINK_END_OF_LIST.

As a result, the linkstub iterator will find a bogus linkstub (flags==0x0000) and call EXIT_STUB_PC on it. In the flags==0x0000 case this ends up at indirect_linkstub_stub_pc because EXIT_STUB_PC falls through to that case. indirect_linkstub_stub_pc then likely crashes. If you skip the 0x0000 case and carry on iterating you end up at completely uninitialized linkstubs with flags==0xcdcd etc.

I don't know whether it's safe to assume you'll see flags==0x0000 when you run off the end, but it can't hurt to test for it.

The following patch is a tentative suggestion:

```
diff --git a/core/arch/arm/emit_utils.c b/core/arch/arm/emit_utils.c
index 5129cab..bb71c39 100644
--- a/core/arch/arm/emit_utils.c
+++ b/core/arch/arm/emit_utils.c
@@ -494,9 +494,11 @@ link_indirect_exit_arch(dcontext_t *dcontext, fragment_t *f,
cache_pc
indirect_linkstub_stub_pc(dcontext_t *dcontext, fragment_t *f, linkstub_t *l)
{
- cache_pc cti = EXIT_CTI_PC(f, l);
+ cache_pc cti;
cache_pc tgt;
dr_isa_mode_t old_mode;
+ ASSERT(LINKSTUB_INDIRECT(l->flags));
+ cti = EXIT_CTI_PC(f, l);
if (!EXIT_HAS_STUB(l->flags, f->flags))
return NULL;
dr_set_isa_mode(dcontext, FRAG_ISA_MODE(f->flags), &old_mode);
diff --git a/core/arch/disassemble_shared.c b/core/arch/disassemble_shared.c
index 04bc401..8bc3e62 100644
--- a/core/arch/disassemble_shared.c
+++ b/core/arch/disassemble_shared.c
@@ -499,6 +499,14 @@ print_known_pc_target(char *buf, size_t bufsz, size_t *sofar INOUT,
CLIENT_ASSERT(!TEST(FRAG_FAKE, fragment->flags),
"opnd_disassemble: invalid target");
for (ls=FRAGMENT_EXIT_STUBS(fragment); ls; ls=LINKSTUB_NEXT_EXIT(ls)) {
+ if (ls->flags == 0) {
+ /* This is not a valid linkstub. We would see this if
+ * the fragment's linkstub list is in an incomplete state.
+ * Either there are no linkstubs or the last one has not
+ * been marked with LINK_END_OF_LIST.
+ */
+ break;
+ }
if (target == EXIT_STUB_PC(dcontext, fragment, ls)) {
print_to_buffer(buf, bufsz, sofar,
"$"PFX" ",
diff --git a/core/arch/x86/emit_utils.c b/core/arch/x86/emit_utils.c
index 30a5b0e..8b01517 100644
--- a/core/arch/x86/emit_utils.c
+++ b/core/arch/x86/emit_utils.c
@@ -883,9 +883,11 @@ link_indirect_exit_arch(dcontext_t *dcontext, fragment_t *f,
cache_pc
indirect_linkstub_stub_pc(dcontext_t *dcontext, fragment_t *f, linkstub_t *l)
{
- cache_pc cti = EXIT_CTI_PC(f, l);
+ cache_pc cti;
/* decode the cti: it should be a relative jmp to the stub */
cache_pc stub;
+ ASSERT(LINKSTUB_INDIRECT(l->flags));
+ cti = EXIT_CTI_PC(f, l);
if (!EXIT_HAS_STUB(l->flags, f->flags))
return NULL;
/* for x64, or -unsafe_ignore_eflags_trace, a trace may have a jne to the stub */
```

Contributor guide

Open the contributing guide

Research direction

Start in core/arch/disassemble_shared.c at print_known_pc_target and compare the linkstub handling with the ARM and x86 indirect_linkstub_stub_pc implementations in core/arch/*/emit_utils.c. Reproduce or add coverage for an incomplete fragment, then verify operand disassembly no longer follows invalid linkstubs or crashes.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
devtools, reverse-engineering
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.