i#2140 : mixing syscall methods by Simorfo · Pull Request #2476 · DynamoRIO/dynamorio

Simorfo · 2017-06-16T15:14:03Z

Adds a simple test using int 2e for syscall, even if another method is used.

What is the best fix ?
Allowing multiple syscall methods ?

Adds a simple test using int 2e for syscall, even if another method is used.

derekbruening · 2017-06-21T16:47:15Z

        }

+#ifdef X86
+        if (my_dcontext != NULL && instr_is_syscall(bb->instr) &&


Why do you check "my_dcontext"? The other code here does not do that. If it is necessary please add a comment explaining why.

I forgot to remove it after trying another solution.
I am removing it now

derekbruening · 2017-06-21T16:49:48Z


+#ifdef X86
+        if (my_dcontext != NULL && instr_is_syscall(bb->instr) &&
+            instr_get_opcode(bb->instr) == OP_int &&


We'd prefer to avoid adding code to the main routine here: either this should be in mangling, if it's something that should happen after a client sees the unchanged code, or it should be in one of the syscall/interrupt handler routines already called during bb building.

Well, there is a good question that relates to my broader question of how to fix this case.

Should we relax the constraint in arch.c :

/* we assume only single method; else need multiple do_syscalls */ ASSERT(new_method == get_syscall_method());

You can see that the UNIX case right above what you're quoting does relax it and has do_int_syscall vs the sysenter do_syscall. We can do something similar for Windows.

I will look into it.
Do you know why this was done for Unix and not for Windows ?

It looks like I should compute both do_syscall (int and other) method at initialization since the do_syscall method is written in the .data section, so it cannot be updated on Windows

.data can be written later by temporarily unprotecting the target page (while trying to avoid races) -- there are some helpers for this like SELF_UNPROTECT_DATASEC

derekbruening · 2017-06-21T16:51:21Z

+#else
+        pExceptionInfo->ContextRecord->Rip++;
+#endif
+#if 0


This "#if 0" is accidentally left here? If this is deliberate, better to be under something descriptive like #ifdef VERBOSE

Cool, I am changing it

derekbruening · 2017-06-21T16:51:40Z

+        pExceptionInfo->ContextRecord->Rip++;
+#endif
+#if 0
+        //prints information about exception


nit: space after //

derekbruening · 2017-06-21T19:19:41Z

+            get_syscall_method() != SYSCALL_METHOD_INT) {
+            instr_t * i2 = INSTR_CREATE_nop(dcontext);
+            SYSLOG_INTERNAL(SYSLOG_WARNING,
+                            "Changing interruption to nop at "PFX"\n", bb->instr_start);


I do not understand: why would we turn it into a nop?

That was the first POC I could find.
But then, it is clearly not transparent to the application.

derekbruening · 2017-06-21T19:21:13Z

+    if (pExceptionInfo->ExceptionRecord->ExceptionCode == EXCEPTION_ACCESS_VIOLATION) {
+        /* Go to next instruction. */
+#ifndef X64
+        pExceptionInfo->ContextRecord->Eip++;


You're assuming the instruction is a single byte? Please explain in a comment.

Well, it looks like (using the verbose output of the program) EIP got already incremented by one before the call of error handler.

I get exception code=c0000005 address=d21175 eip=d21176, info = 0, ffffffff

An access violation will point directly at the faulting instruction. Something is off here, right?

I do not fully understand what is going on, but it looks like the printed EIP from the context in exception handler does not point to the faulting instruction, but rather at the address+1. I run the program directly in this case (no dynamorio).
This look like with an int 2d, except EIP got incremented instead of ExceptionAddress.

Can you reproduce this behavior by running security-win32.except-int2e.exe ?

Simorfo · 2017-07-11T11:45:46Z

After testing, it looks like the right solution is to prepare multiple routines for the different sys calls cases.

…od is used

Simorfo · 2017-07-12T11:47:17Z

Here is another patch.
Instead of trying to do syscalls with multiple methods or updating do_syscall, I consider int 2e not as a syscall when another syscall method is used.
It works on the simple test, but I do not know if it covers all the cases.

fhahn · 2017-07-12T12:31:07Z

run aarch64 tests

fhahn · 2017-07-12T13:27:23Z

run aarch64 tests

Simorfo · 2017-07-12T13:46:20Z

I do not have an aarch64 platform to run tests.
This patch is only about X86, behavior for ARM should remain unchanged.

fhahn · 2017-07-12T13:51:59Z

Ah sorry for the confusion. I've added a machine to run the precommit suite on AArch64 for pull requests, and a build is triggered by commenting run aarch64 tests . For some reason this PR does not get picked up, maybe because it was created before the machine started operating or because the branch is out-of-date.

I only tried to trigger a build, because of 91cf77d#diff-e1b5b4d92663976c0db6c7ccc36d578fR110 which might have an impact on ARM/AArch64?

derekbruening · 2017-07-12T13:59:04Z

@fhahn -- it's probably best to document that "run aarch64 tests" and other info in the wiki

Simorfo · 2017-07-12T14:12:15Z

@fhahn ok I understand.
If I got it correct, 91cf77d#diff-e1b5b4d92663976c0db6c7ccc36d578fR110 should have no impact on ARM.

derekbruening · 2017-07-14T02:36:11Z

Haven't looked at the code, but this confuses me:

I consider int 2e not as a syscall

We can't just ignore int 2e syscalls by the app, right, so I'm not sure what you mean?

Simorfo · 2017-07-19T06:39:41Z

We can't just ignore int 2e syscalls by the app, right, so I'm not sure what you mean?

We consider them as interruptions, not syscalls.
I am not fully aware of how sys calls are handled by dynamorio.
But so far, this patch seems to work on the tests cases, including the one I added about int 2e that did not work previously.

The code simply replaces in the good spots instr_is_syscall(i) by

instr_is_syscall(i) && 
(get_syscall_method() == SYSCALL_METHOD_INT || instr_get_opcode(i) != OP_int)

derekbruening · 2017-07-20T02:48:17Z

We can't just ignore int 2e syscalls by the app, right, so I'm not sure what you mean?

We consider them as interruptions, not syscalls.
I am not fully aware of how sys calls are handled by dynamorio.

DR must see certain syscalls (control or memory related) for correct operation. Not monitoring some syscalls without knowing the syscall number and whether DR cares about it is unsafe. DR promises that clients will see all syscalls. Something like Dr. Memory or any taint tracker will have false positives and false negatives if it misses syscalls.

Simorfo · 2017-07-20T06:40:41Z

Ok, so the test case is not complete enough and the patch is not good enough.
Can we translate an "int 2e" syscall into another type of sys call such as wow64 ?

derekbruening · 2017-07-20T15:00:23Z

I would think we should leave the app syscalls using their original gateway methods. DR does have support for multiple app syscall gateways on Linux: e.g., see get_do_int81_syscall_entry and EXIT_REASON_NI_SYSCALL_INT_0x81 and ditto for 82, and using both sysenter and int 0x80. Presumably we can do the same thing on Windows.

The messy part is what to do when DR needs to fabricate a syscall that wasn't there in the app code. That's where there needs to be a single "primary" syscall method that DR will use. This is the syscall_method global state stuff. We just need to relax the checks there and pick one, like we do on Linux.

derekbruening · 2017-07-20T15:00:54Z

I would think we should leave the app syscalls using their original gateway methods. DR does have support for multiple app syscall gateways on Linux: e.g., see get_do_int81_syscall_entry and EXIT_REASON_NI_SYSCALL_INT_0x81 and ditto for 82, and using both sysenter and int 0x80. Presumably we can do the same thing on Windows.

The messy part is what to do when DR needs to fabricate a syscall that wasn't there in the app code. That's where there needs to be a single "primary" syscall method that DR will use. This is the syscall_method global state stuff. We just need to relax the checks there and pick one, like we do on Linux.

Copied code from int 81 handling on Linux. Checks syscall param_base readability before logging. Translates exception address in case exception is caused by int 2e. Fixes issue 2140

Simorfo · 2017-08-22T13:51:12Z

 #else
-    /* we assume only single method; else need multiple do_syscalls */
-    ASSERT(new_method == get_syscall_method());
+    /* There is usually a single method. */


Check needed to be relaxed

Simorfo · 2017-08-22T13:51:31Z

        }
    }
 #ifdef WINDOWS
+    if (instr_get_opcode(bb->instr) == OP_int) {


Maybe it is not the best style

Simorfo · 2017-08-22T13:52:46Z

            cxt->CXT_XIP = (ptr_uint_t) dcontext->asynch_target;
            /* now handle the fault just like RaiseException */
            DODEBUG({ known_source = true; });
+        } else if ((app_pc) pExcptRec->ExceptionAddress ==


Syscall can cause exception (the test case does this).
So we need to translate the address from the do_syscall method to the program address where the sys call happens

I don't follow: the syscall itself should not cause an exception: it would just return a failure return value, not throw an exception. Could you elaborate on why an exception would happen whose PC is the entry to our generated int 0x2e?

Well, I do not know what it should make, but the syscall does throw an exception, see the test case.
I guess that this happens because the syscall parameters are invalid (0xdeadc0de)

This doesn't make sense to me: this has never happened before. Invalid parameters result in the kernel returning an appropriate error code, not throwing an exception into user mode. Is this limited to WOW64 and this happens during the syscall argument marshalling in (64-bit) user mode and that's why there's an exception, because it's not yet in the kernel?

But you're saying this exception is on an int 0x2e, not a WOW64 "syscall" gateway of the call* to the WOW64 library layer.

Simorfo · 2017-08-22T13:54:31Z

-        sys_param(dcontext, param_base, 7));
-    LOG(THREAD, LOG_SYSCALLS, 3, "\tparam 8: "PFX"\n",
-        sys_param(dcontext, param_base, 8));
+    /* Needs to check readability before logging. */


Check is needed because the program can set param_base to an invalid value (unreadable address in memory) and that happens with the test case

Sure, though this method is racy as the memory could become unreadable concurrently with this code. The more robust method is a safe_read or try/except, but since this is just logging code this is ok if it makes the code simpler: but please use a comment stating we're aware of the race.

Simorfo · 2017-08-22T13:56:47Z

So, here is a new patch, inspired by int 81 handling on Linux.
There was more to patch to get the test case working : check syscall param_base readability, and translate exception address caused by int 2e syscall.

derekbruening

Looks pretty good, just a few requests/suggestions

derekbruening · 2017-09-06T03:08:21Z

-    /* we assume only single method; else need multiple do_syscalls */
-    ASSERT(new_method == get_syscall_method());
+    /* There is usually a single method. */
+    ASSERT_CURIOSITY(new_method == get_syscall_method());


Since we handle it maybe there's no reason to print out a curiosity. But we still only handle combining syscall or sysenter with int 2e: we won't handle both syscall and sysenter (seems really unlikely of course), so maybe this should be a check that either the new or the old is int 2e, right?

derekbruening · 2017-09-06T03:13:06Z

    /* FIXME i#1551: maybe we should union int80 and svc as both are inline syscall? */
+
+#ifdef WINDOWS
+    /* Like int 81 on Linux. */


81 is on Mac, not Linux

derekbruening · 2017-09-06T03:20:15Z

 #ifdef WINDOWS
+    if (instr_get_opcode(bb->instr) == OP_int) {
+        if (instr_get_interrupt_number(bb->instr) == 0x2e) {
+            /* Special handling of int 2e when it is not the syscall method. */


So the assumption is that syscall is seen first, and is the primary method? So that should be checked in check_syscall_method, right? B/c we won't handle the other order (right?)

yes.
To be more precise, "int" is the second method. Wow64 can be the first method as well.

derekbruening · 2017-09-06T03:23:36Z

+                PRE(ilist, instr,
+                    instr_create_save_immed16_to_dcontext(dcontext, reason,
+                                                          EXIT_REASON_OFFSET));
+            }


I see several instances of this if..else sequence now for writing the exit reason: how about putting it into a local helper function and calling that from all the cases in this file?

ok done with mangle_special_exit

derekbruening · 2017-09-06T03:24:00Z

+                    instr_create_save_immed16_to_dcontext(dcontext, reason,
+                                                          EXIT_REASON_OFFSET));
+            }
+        }


assert otherwise (i.e., if num is not 0x2e)

derekbruening · 2017-09-06T03:27:18Z

            cxt->CXT_XIP = (ptr_uint_t) dcontext->asynch_target;
            /* now handle the fault just like RaiseException */
            DODEBUG({ known_source = true; });
+        } else if ((app_pc) pExcptRec->ExceptionAddress ==


I don't follow: the syscall itself should not cause an exception: it would just return a failure return value, not throw an exception. Could you elaborate on why an exception would happen whose PC is the entry to our generated int 0x2e?

derekbruening · 2017-09-06T03:29:11Z

-        sys_param(dcontext, param_base, 7));
-    LOG(THREAD, LOG_SYSCALLS, 3, "\tparam 8: "PFX"\n",
-        sys_param(dcontext, param_base, 8));
+    /* Needs to check readability before logging. */


Sure, though this method is racy as the memory could become unreadable concurrently with this code. The more robust method is a safe_read or try/except, but since this is just logging code this is ok if it makes the code simpler: but please use a comment stating we're aware of the race.

derekbruening · 2017-09-06T03:29:35Z

-    LOG(THREAD, LOG_SYSCALLS, 3, "\tparam 8: "PFX"\n",
-        sys_param(dcontext, param_base, 8));
+    /* Needs to check readability before logging. */
+    if (is_readable_without_exception((byte *) param_base, 8 * sizeof(app_pc))) {


The check should be inside a DOLOG to avoid waste when logging is turned off

Simorfo · 2017-09-13T13:59:38Z

Here comes a new patch.

The important point is that syscall in the test case generates an exception (read access violation). So dynamoRIO indeed needs to translate the address when it catches an exception caused by a syscall routine.

derekbruening · 2017-09-25T18:45:02Z

Sorry for the delay, but Travis did an upgrade and broke a lot of our tests (#2632, #2634, #2641) and we're still trying to fix them all. Also I am curious about this exception and plan to run it myself to see it firsthand when I get a chance.

Simorfo · 2017-10-02T14:14:45Z

Maybe this exception has something to do with code in syscall.c
IF_X64(ASSERT_TRUNCATE(sysnum, int, mc->xax));

Philippe Antoine added 6 commits June 16, 2017 17:11

i#2140 : mixing syscall methods

645aaa6

Adds a simple test using int 2e for syscall, even if another method is used.

i#2140 : compile fix

28f6dde

i#2140: remove trailing space

d5145b4

i#2140: remove too long line

ca342b0

i#2140 : restriction to x86

c0f38a9

i#2140 : good identation for ifdef

271f910

derekbruening requested changes Jun 21, 2017

View reviewed changes

i#2140: review corrections

e9eb561

i#2140 : Not considering int 2e as syscalls when another syscall meth…

7323af7

…od is used

i#2140: ARM compatibility

91cf77d

i#2140: code style

1d3b94a

i#2140: new patch like int 81 on Linux

9d97ecf

Copied code from int 81 handling on Linux. Checks syscall param_base readability before logging. Translates exception address in case exception is caused by int 2e. Fixes issue 2140

Simorfo commented Aug 22, 2017

View reviewed changes

i#2140: code style

059833c

derekbruening reviewed Sep 6, 2017

View reviewed changes

i#2140: code review taken into account

82eff53

Conversation

Simorfo commented Jun 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simorfo commented Jul 11, 2017

Uh oh!

Simorfo commented Jul 12, 2017

Uh oh!

fhahn commented Jul 12, 2017

Uh oh!

fhahn commented Jul 12, 2017

Uh oh!

Simorfo commented Jul 12, 2017

Uh oh!

fhahn commented Jul 12, 2017

Uh oh!

derekbruening commented Jul 12, 2017

Uh oh!

Simorfo commented Jul 12, 2017

Uh oh!

derekbruening commented Jul 14, 2017

Uh oh!

Simorfo commented Jul 19, 2017

Uh oh!

derekbruening commented Jul 20, 2017

Uh oh!

Simorfo commented Jul 20, 2017

Uh oh!

derekbruening commented Jul 20, 2017

Uh oh!

derekbruening commented Jul 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Simorfo Sep 13, 2017 •

edited

Loading