DynamoRIO / DynamoRIO/dynamorio
Operand disassembly can crash when fragment is incomplete
- 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
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