i#2144 : single step on jump #2295

Simorfo · 2017-03-15T13:02:12Z

Changing exception address when a single step exception occurs when
it happens in the stub.

Fixes i#2144

Simorfo · 2017-03-15T13:05:31Z

The fix is not perfect. I think it should change the exception address to the jump destination and not the address of the jump instruction. But I do not know how to get this value.

derekbruening · 2017-03-16T13:05:38Z

I wonder if you could paste the relevant pieces from loglevel 4 into the issue tracker to make sure we understand what's happening here? I put a request there.

derekbruening · 2017-03-16T13:07:48Z

core/win32/callback.c

+             * i#2144 : We exclude single step cases in a jump.
+             */
+            if (pExcptRec->ExceptionCode == EXCEPTION_SINGLE_STEP &&
+                !in_fcache(pExcptRec->ExceptionAddress)) {


The comment below is now in the wrong place, about calling in_fcache last.

Logically this would be better out in the caller and not here in check_internal_exception which is just supposed to handle a crash in DR itself.

Probably what is needed is a query in_exit_stub().

derekbruening · 2017-03-16T13:08:23Z

core/win32/callback.c

+                !in_fcache(pExcptRec->ExceptionAddress)) {
+                fragment_t wrapper;
+                linkstub_t *l;
+                fragment_t *f = fragment_pclookup(dcontext, dcontext->next_tag, &wrapper);


next_tag is only set on entering the cache. This could have run thousands of blocks since then, so next_tag is irrelevant at this point.

It seems that to handle this we need to decode the stub and extract the linkstub pointer. We have existing stub decoding for coarse stubs today but not for fine.

derekbruening · 2017-03-16T13:08:42Z

core/win32/callback.c

+                fragment_t wrapper;
+                linkstub_t *l;
+                fragment_t *f = fragment_pclookup(dcontext, dcontext->next_tag, &wrapper);
+                l = FRAGMENT_EXIT_STUBS(f);


There's no check for pclookup failure.

derekbruening · 2017-03-16T13:16:03Z

An alternative could be to insert a nop during mangling after everything that could trigger this: just popf and iret?

Simorfo · 2017-03-16T14:18:43Z

Maybe your alternative is good.
But I did not feel confident about the exhaustivity of the conditions which could trigger this.
And even so, it would not be totally transparent about the exception address sent to the exception handler.
I will put some level 4 logs as requested.

derekbruening · 2017-03-16T14:22:34Z

But I did not fell confident about the exhaustivity of the conditions which could trigger this.

You mean, things like execution under a debugger?

And even so, it would not be totally transparent about the exception address sent to the exception handler.

We would arrange to have the translation do the right thing.

Simorfo · 2017-03-16T14:32:59Z

You mean, things like execution under a debugger?

That is a good example.
It just seems hard to me to be sure that we thought about every possible case...

Simorfo · 2017-03-27T06:56:59Z

So, what should be done ?
Look for pop and iret ?
Or add some code to see if we are in a stub ?

First solution seems easier, but maybe not best for transparency (ExceptionAddress)

derekbruening · 2017-03-27T16:12:27Z

It is up to you. Certainly putting a nop after the couple of explicit flag writes is the easiest thing to implement and should cover most cases here, whille implementing stub translation is a bigger project.

Execution under a debugger already hits cases we just can't solve from user space: e.g., on Windows, OutputDebugString can cause us to lose control under a debugger. So bailing on handling this under a debugger for now does not seem terrible.

Something like setting a thread's context to trigger this seems like more of a problem. We could at least add a check for that which complains even if it doesn't handle it.

Simorfo · 2017-03-29T08:01:55Z

Okay.
I will go for popf and iret

Should I dot it in mangle function ? something like

#ifdef FOOL_CPUID
        else if (instr_get_opcode(instr) == OP_cpuid) {
            mangle_cpuid(dcontext, ilist, instr, next_instr);
            continue;
        }
#endif

derekbruening · 2017-03-29T17:20:13Z

Yes, sthg like mangle() asking if (instr_can_set_single_step()) mangle_single_step()?

Simorfo · 2017-03-30T13:31:46Z

For iret, I did not know what to do.
I think that we cannot insert a no after a iret because we go to a different basic block.

derekbruening · 2017-03-30T14:42:52Z

For iret, I did not know what to do.
I think that we cannot insert a no after a iret because we go to a different basic block.

We split iret up into its pieces and could put a nop after the popf.

derekbruening · 2017-03-30T14:44:50Z

For iret, I did not know what to do.
I think that we cannot insert a no after a iret because we go to a different basic block.

We split iret up into its pieces and could put a nop after the popf.

I guess that means we need interactions between iret mangling and this single-step mangling.

derekbruening · 2017-03-31T19:41:25Z

core/arch/x86/mangle.c

+     * FIXME i#2144 : to be absolutely transparent, we should translate the
+     * exception address as if we did not insert this nop.
+     */
+    POST(ilist, instr, INSTR_CREATE_nop(dcontext));


Hmm, the instrlist_set_translation_target() is probably automagically giving this a translation target but it's equal to the popf, which is not what we want, right? Also, when translating the single-step exception that points at the nop, won't translate_walk_track() consider this "unsupported_mangle" and thus translate_walk_good_state() will return false and we'll get an assert?

So we need two things: one, set the translation to the next app pc; and two, recognize the nop during translation.

derekbruening · 2017-03-31T19:42:43Z

suite/tests/security-win32/singlestep.c

+        xor      eax, eax
+/* push flags on the stack */
+#ifdef X64
+        pushfq


We have a cpp layer (see asm_defines.asm) to avoid needing ifdefs: use PUSHF, PTRSZ, and REG_XSP.

derekbruening · 2017-03-31T19:43:05Z

suite/tests/security-win32/singlestep.c

+        inc      eax
+/* popping flags from top of the stack */
+#ifdef X64
+        popfq


Use POPF and no ifdef is needed

Simorfo · 2017-04-03T09:36:28Z

I hope this fix is ok.
There still remains that the ExceptionAddress is not transparent : it should be the jump target, not the jump address.
I do not know if it could be fixed in the translate_walk_track and such process.

Simorfo · 2017-04-03T12:30:22Z

I did not manage to reproduce the appveyor 64bit test failure. The 64 bit test build went fine by me.

Simorfo · 2017-04-03T13:31:15Z

In facts, the 64-bit test works when running with my client, and fails without it (because mangling does not insert the nop)... No idea why...

derekbruening · 2017-04-03T21:11:55Z

Both 32-bit and 64-bit singlestep tests are crashing on a STATUS_SINGLE_STEP exception:

start of test, count = 0
<Application C:\projects\dynamorio\build\build_debug-internal-32\suite\tests\bin\security-win32.singlestep.exe (1716).  DynamoRIO internal crash at PC 0x1a385f94.  Please report this at http://dynamorio.org/issues/.  Program aborted.
0x80000004 0x00000000 0x1a385f94 0x1a385f94 0x00000000 0x00000000
Base: 0x742b0000
Registers: eax=0x00000001 ebx=0x00000000 ecx=0x002f219c edx=0x0071a731
	esi=0x002f1f1f edi=0x002f1f1f esp=0x0042fb04 ebp=0x0042fb08
	eflags=0x0000
version 6.2.17259, custom build
-no_dynamic_options -code_api -probe_api -msgbox_mask 0 -dumpcore_mask 125 -stderr_mask 12 -stack_size 56K -max_elide_jmp 0 -max_elide_call 0 -no_inline_ignored_syscalls -staged -native_exec_default_list '' -no_native_exec_managed_code -no_indcall2direct -no_aslr_dr -pad_jmps_mark_no_trace 
0x0042fb08 0x002f1e5c
0x0042fb50 0x756f7c04
0x0042fb64 0x778eab8f
0x0042fbac 0x778eab5a
0x0042fbbc 0x00000000>

derekbruening · 2017-04-03T21:15:41Z

There still remains that the ExceptionAddress is not transparent : it should be the jump target, not the jump address.

I'm not sure what jump your'e referring to? With the nop, isn't the STATUS_SINGLE_STEP pointing at the nop, which is in_fcache and thus gets translated (hence my prior comments about properly translating the nop)? There's no jump involved, right?

derekbruening · 2017-04-03T21:32:13Z

There still remains that the ExceptionAddress is not transparent : it should be the jump target, not the jump address.

I'm not sure what jump your'e referring to? With the nop, isn't the STATUS_SINGLE_STEP pointing at the nop, which is in_fcache and thus gets translated (hence my prior comments about properly translating the nop)? There's no jump involved, right?

Keep forgetting the details here: ok, so you're saying that even once the mange-inserted nop is translated to the next app instr, when that's a jump, the exception address is supposed to go further and point at the jump target. I guess that weird behavior is limited to STATUS_SINGLE_STEP and we'd handle that in callback.c?

derekbruening · 2017-04-03T21:16:49Z

core/arch/x86/mangle.c

@@ -2403,7 +2405,17 @@ mangle_single_step(dcontext_t *dcontext, instrlist_t *ilist, instr_t *instr)
     * FIXME i#2144 : to be absolutely transparent, we should translate the
     * exception address as if we did not insert this nop.
     */
-    POST(ilist, instr, INSTR_CREATE_nop(dcontext));
+    instr_t* next = instr_get_next_app(instr);


style: t *n

derekbruening · 2017-04-03T21:25:12Z

core/arch/x86/mangle.c

@@ -2403,7 +2405,17 @@ mangle_single_step(dcontext_t *dcontext, instrlist_t *ilist, instr_t *instr)
     * FIXME i#2144 : to be absolutely transparent, we should translate the
     * exception address as if we did not insert this nop.
     */
-    POST(ilist, instr, INSTR_CREATE_nop(dcontext));
+    instr_t* next = instr_get_next_app(instr);
+    /* Assumes popf cannot end a basic block. */


This is not a valid assumption as blocks are truncated prior to a branch for various reasons. Can we just have NULL be another condition.

derekbruening · 2017-04-03T21:27:09Z

core/arch/x86/mangle.c

@@ -2403,7 +2405,17 @@ mangle_single_step(dcontext_t *dcontext, instrlist_t *ilist, instr_t *instr)
     * FIXME i#2144 : to be absolutely transparent, we should translate the
     * exception address as if we did not insert this nop.


Aren't we already doing that?

No, we should get the jump target, not the jump instruction address...

Using the jump target instead of the jump is unique to STATUS_SINGLE_STEP, right? Nothing else does that. So the code to do that shouldn't be in general translate.c but instead in callback.c where single step is handled. IMHO it should be part of this CL: it's just a couple of lines of code and without it the tests fail, don't they?

Using the jump target instead of the jump is unique to STATUS_SINGLE_STEP, right?

Yes to the best of my knowledge.

The tests I added will not fail because my fault handler does not check the exception address (just the exception code)

I will try to put this check before check_internal_exception

derekbruening · 2017-04-03T21:35:06Z

core/arch/x86/mangle.c

+     * because the ExceptionAddress should be the jump target.
+     */
+    if (instr_is_cti(next)) {
+        /* Next app instruction gives a correct translation to this nop. */


But won't instrlist_set_translation be set to instr and not next?

Not in my tests although I am not fully aware of instrlist_set_translation code

mangle_shared.c:865:

app_pc xl8 = get_app_instr_xl8(instr); instrlist_set_translation_target(ilist, xl8);

It doesn't matter where the nop is inserted: its xl8 will be set to ilist->translation_target, which is the xl8 of the instr being mangled, which is the popf.

OK I understand.
But the ExceptionAddress is set to the EIP value after execution of a single instruction.
So, the ExceptionAddress we get is the EIP value after the no we insert, i.e. the jump address.
Considering that, I think it is ok to leave the translation address of the no to the popf

I don't follow -- isn't it like this: we put into the code cache "popf; nop". The popf writes to TF which generates the exception. The exception points at the next instruction from the one that generated it, so it points at the nop. Right? Then we process the exception and we translate it to the popf in the original app code.

The popf writes to TF

yes

which generates the exception.

no : the next instruction after popf does

The exception points at the next instruction from the one that generated it

yes

so it points at the nop. Right?

no, it points at the jump

Then we process the exception and we translate it to the popf in the original app code.

no we translate it to the jump but we should translate it to the address after the jump

Yes. Right. I also looked at the manual and now I believe I finally have all the details. Sorry, I should have thought this through earlier. Unfortunately this nop solution is not going to work for the general case. We can make it work for "popf;jmp" or "popf;jcc" with no client, because we can emulate the jmp or jcc, but the presence of instrumentation means we need a different solution as we can't easily emulate arbitrary sequences of instructions. I believe we have to disable and later forge the single-step exception. I will add something to the issue tracker.

Let's move the discussion to #2144 (comment) where I try to describe the problems (instru or mangling between popf and next app instr) that require a larger solution.

Simorfo · 2017-04-04T07:17:04Z

Keep forgetting the details here: ok, so you're saying that even once the mange-inserted nop is translated to the next app instr, when that's a jump, the exception address is supposed to go further and point at the jump target.

Exactly

Simorfo · 2017-04-04T09:59:41Z

I think that I understand why there is a crash with no client (and success with a client).

It looks like the loop over instructions in mangle function does not take into account the popf instruction because it is somehow not marked as interesting. (with my client, every instruction is marked as interesting)
It looks that this is done in build_bb_ilist for other kinds of instructions.

Did I point out something here ?
Can you tell me more ?

derekbruening · 2017-04-04T13:54:06Z

I think that I understand why there is a crash with no client (and success with a client).

It looks like the loop over instructions in mangle function does not take into account the popf instruction because it is somehow not marked as interesting. (with my client, every instruction is marked as interesting)
It looks that this is done in build_bb_ilist for other kinds of instructions.

With no client, DR uses IR Level 0 bundles of instructions for non-"interesting" instructions: with no client, it doesn't care about non-branches in general. The mangle loop skips instrs with no opcode. You want to update decode_cti() -- probably next to the decode of iret.

Simorfo · 2017-04-04T14:36:42Z

You want to update decode_cti()

Thanks, that was exactly what I was looking for

I moved the FIXME comment to callback.c
I do not know how I should get the next EIP.
In the case of a conditional jump, we should decide which branch to take...

Keeping special exit even after client processing. Restoring default exit reason to self mdoficiation. Going to right return address after iret instead of next one.

derekbruening · 2017-05-05T20:42:32Z

I'm afraid that the mixedmode_late failure is from this CL -- I filed as #1421 but it seems to be consistently happening here and only here. Does that test pass locally for you? (Not sure but it may only be enabled in a full suite run so sthg like "ctest" in one build dir is not sufficient.)

Simorfo · 2017-05-07T07:56:16Z

The mixedmode.exe does not work on my windows 7 (without dynamorio) (I get the popup asking to close this program because of a failure)

Simorfo · 2017-05-10T08:20:57Z

From the appveyor log, I gather that the problem comes from an added instruction by this CL (there is a popf in mixedmode)
I guess that this is the instruction where we set dynamically the exit reason of the special single step basic block.
I just made it the same as other parts of code even if I do not understand the difference implied by the condition DYNAMO_OPTION(private_ib_in_tls) || TEST(FRAG_SHARED, flags)

Simorfo · 2017-05-10T09:13:17Z

Can you provide me a log for the error with mixedmode ? I do not manage to run the test by myself.

Simorfo · 2017-05-15T09:28:46Z

OK, I got the log and here is the problem :
ERROR: Could not find encoding for: popf %rsp (%rsp)[8byte] -> %rsp

Simorfo · 2017-05-15T15:07:52Z

Test now works by me.
The problem was the fast decoding of popf which did not take into account the size of esp register (when running 32 bit program on 64 bit dynamorio)
Maybe this patch can be better written with something like resolve_variable_size_dc for registers

derekbruening · 2017-05-15T18:37:02Z

core/arch/x86/decode_fast.c

-                       resolve_variable_size_dc(dcontext, 0, OPSZ_ret, false)));
-        instr_set_dst(instr, 0, opnd_create_reg(REG_XSP));
+                      (stack_sized_reg, REG_NULL, 0, 0,
+                       resolve_variable_size_dc(dcontext, 0,


We have to pass prefixes here, not 0, to handle a data-size prefix on popf

Like this ? even if we cannot get this far with prefixes > 0 ?

Why do you say "we cannot get this far with prefixes > 0"? Maybe you could add to your test a case with the data size prefix to make sure?

Because before my patched code, there is

if (prefixes > 0) { /* prefixes are rare on ctis * rather than handle them all here, just do full decode * FIXME: if we start to see more and more jcc branch hints we * may change our minds here! This is case 211206/6749. */ if (decode(dcontext, start_pc, instr) == NULL) return NULL; else return (start_pc + sz); }

OK, so it goes to the slow path. It doesn't have to for this case but it's not worth reordering. It still seems safer to pass prefixes here rather than 0 to future-proof. It would also be nice to have a test w/ a size prefix but there is a long list of "nice to have" things in the tests so this seems sufficient.

I will update to master and merge.

derekbruening · 2017-05-16T17:31:11Z

Appveyor failure is api.detach #2246.

derekbruening · 2017-05-17T15:03:03Z

There are two problems with what was checked in: style violations and worse a crash, documented at #2144 (comment)

Simorfo force-pushed the singlestep branch from bd9020f to 78b6a6c Compare March 15, 2017 13:32

derekbruening mentioned this pull request Mar 16, 2017

APP CRASH (telock99-hostname.exe) single step execution #2144

Closed

derekbruening requested changes Mar 16, 2017

View reviewed changes

Simorfo force-pushed the singlestep branch from e9dd3a1 to 3c9f07c Compare March 30, 2017 13:30

derekbruening requested changes Mar 31, 2017

View reviewed changes

derekbruening reviewed Apr 3, 2017

View reviewed changes

Simorfo force-pushed the singlestep branch from 0f3bcee to 0662664 Compare April 4, 2017 14:34

Philippe Antoine added 7 commits May 5, 2017 11:17

i#2144: compatibility with client

ac900b2

i#2144: iret handling even with client.

c349d3a

Keeping special exit even after client processing. Restoring default exit reason to self mdoficiation. Going to right return address after iret instead of next one.

i#2144 : changing code thnaks to review

31c0e41

i#2144 : asserting good fragment

f401a88

i#2144: setting max_bb_instrs to 1 for single step basic block

171b037

i#2144 : good order for instruction tests

dc04fd1

i#2144: sets exit reason with x64 implemented method

892513a

Simorfo mentioned this pull request May 5, 2017

i#2163: debug register breakpoints #2303

Merged

derekbruening approved these changes May 5, 2017

View reviewed changes

i#2144: setting exit reason the same way

9a1826e

Simorfo force-pushed the singlestep branch from eb1561a to 9a1826e Compare May 10, 2017 08:17

Philippe Antoine added 3 commits May 15, 2017 11:31

i#2144 : fixing bug while decoding popf (popping only 4 bytes)

5aad852

i#2144 : setting popf esp size depending on execution

f0b6c88

i#2144 : fast decode of popf uses well sized XSP register

bf456f8

derekbruening requested changes May 15, 2017

View reviewed changes

i#2144: using prefixes on popf fast decode

ac8a5ef

derekbruening approved these changes May 16, 2017

View reviewed changes

Merge branch 'master' into singlestep

2c9bcb7

derekbruening merged commit 47b56c8 into DynamoRIO:master May 16, 2017

Simorfo pushed a commit to Simorfo/dynamorio that referenced this pull request May 17, 2017

i#2163: new patch after review and merge from DynamoRIO#2295

224a350

Simorfo pushed a commit to Simorfo/dynamorio that referenced this pull request May 26, 2017

i#2163: new patch after review and merge from DynamoRIO#2295

58c4c6e

derekbruening mentioned this pull request Aug 13, 2019

Crashing DR client when a process sets Trap flag #3783

Open

		@@ -2403,7 +2405,17 @@ mangle_single_step(dcontext_t dcontext, instrlist_t ilist, instr_t *instr)
		* FIXME i#2144 : to be absolutely transparent, we should translate the
		* exception address as if we did not insert this nop.

i#2144 : single step on jump #2295

i#2144 : single step on jump #2295

Conversation

Simorfo commented Mar 15, 2017

Simorfo commented Mar 15, 2017

derekbruening commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbruening commented Mar 16, 2017

Simorfo commented Mar 16, 2017 • edited Loading

derekbruening commented Mar 16, 2017

Simorfo commented Mar 16, 2017 • edited Loading

Simorfo commented Mar 27, 2017 • edited Loading

derekbruening commented Mar 27, 2017

Simorfo commented Mar 29, 2017

derekbruening commented Mar 29, 2017

Simorfo commented Mar 30, 2017

derekbruening commented Mar 30, 2017

derekbruening commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simorfo commented Apr 3, 2017

Simorfo commented Apr 3, 2017

Simorfo commented Apr 3, 2017 • edited Loading

derekbruening commented Apr 3, 2017 • edited Loading

derekbruening commented Apr 3, 2017

derekbruening commented Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simorfo Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simorfo commented Apr 4, 2017

Simorfo commented Apr 4, 2017

derekbruening commented Apr 4, 2017

Simorfo commented Apr 4, 2017

derekbruening commented May 5, 2017

Simorfo commented May 7, 2017

Simorfo commented May 10, 2017

Simorfo commented May 10, 2017

Simorfo commented May 15, 2017

Simorfo commented May 15, 2017

Choose a reason for hiding this comment

Simorfo May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbruening commented May 16, 2017

derekbruening commented May 17, 2017

Simorfo commented Mar 16, 2017 •

edited

Loading

Simorfo commented Mar 16, 2017 •

edited

Loading

Simorfo commented Mar 27, 2017 •

edited

Loading

Simorfo commented Apr 3, 2017 •

edited

Loading

derekbruening commented Apr 3, 2017 •

edited

Loading

derekbruening commented Apr 3, 2017 •

edited

Loading

Simorfo Apr 4, 2017 •

edited

Loading

Simorfo May 16, 2017 •

edited

Loading