Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

i#6417: restore registers before syscall. #6475

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

i#6417: restore registers before syscall. #6475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
925d9e6
i#6417: restore output register for syscall.
ivankyluk Nov 27, 2023
b463ae2
Merge branch 'master' into i6417-restore-output-register-before-syscall
ivankyluk Dec 5, 2023
9bba603
Incorporate review comments.
ivankyluk Dec 5, 2023
c6cf84b
Fix CI test breakages.
ivankyluk Dec 5, 2023
185cd63
Rename x86 templatex files, and fix Aarch64 syscall number.
ivankyluk Dec 5, 2023
d0f8a79
Merge branch 'master' into i6417-restore-output-register-before-syscall
ivankyluk Dec 5, 2023
438bca2
Merge branch 'master' into i6417-restore-output-register-before-syscall
ivankyluk Dec 15, 2023
94073fe
Merge branch 'master' into i6417-restore-output-register-before-syscall
ivankyluk Mar 25, 2024
732be3c
Merge branch 'master' into i6417-restore-output-register-before-syscall
ivankyluk May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions clients/drcachesim/tests/offline-syscall-mmap-aarch64.templatex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
Calling mmap\(0, 0x8765, 0x7, 0x22, -1, 0\)
Output format:
<--record#-> <--instr#->: <---tid---> <record details>
------------------------------------------------------------
.*
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function==syscall #222>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0x0>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0x8765>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0x7>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0x22>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0xffffffffffffffff>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0x0>
.*
13 changes: 13 additions & 0 deletions clients/drcachesim/tests/offline-syscall-mmap-x86-32.templatex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
Calling mmap\(0, 0x8765, 0x7, 0x22, -1, 0\)
Output format:
<--record#-> <--instr#->: <---tid---> <record details>
------------------------------------------------------------
.*
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function==syscall #192>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0x0>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0x8765>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0x7>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0x22>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0xffffffff>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0x0>
.*
13 changes: 13 additions & 0 deletions clients/drcachesim/tests/offline-syscall-mmap-x86-64.templatex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
Calling mmap\(0, 0x8765, 0x7, 0x22, -1, 0\)
Output format:
<--record#-> <--instr#->: <---tid---> <record details>
------------------------------------------------------------
.*
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function==syscall #9>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0x0>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0x8765>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0x7>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0x22>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0xffffffff>
+[0-9]+ +[0-9]+: +[0-9]+ <marker: function argument 0x0>
.*
18 changes: 17 additions & 1 deletion ext/drreg/drreg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,11 @@ drreg_insert_restore_all(void *drcontext, instrlist_t *bb, instr_t *inst,
(instr_is_label(inst) &&
((ptr_uint_t)instr_get_note(inst) == DR_NOTE_ANNOTATION ||
(ptr_uint_t)instr_get_note(inst) == DR_NOTE_REG_BARRIER)) ||
/* i#6417: in case of a syscall, restore the values of registers since
* they may be used as input parameters for the kernel. For example,
* ECX is used to store the length for mmap2.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this code is restoring the flags. I don't think they are an input to any syscall on any platform (they are an output on Mac).

*/
instr_is_syscall(inst) ||
/* DR slots are not guaranteed across app instrs */
(pt->aflags.slot != MAX_SPILLS &&
pt->aflags.slot >= (int)ops.num_spill_slots))) {
Expand Down Expand Up @@ -531,6 +536,11 @@ drreg_insert_restore_all(void *drcontext, instrlist_t *bb, instr_t *inst,
(instr_is_label(inst) &&
((ptr_uint_t)instr_get_note(inst) == DR_NOTE_ANNOTATION ||
(ptr_uint_t)instr_get_note(inst) == DR_NOTE_REG_BARRIER)) ||
/* i#6417: in case of a syscall, restore the values of registers since
* they may be used as input parameters for the kernel. For example,
* ECX is used to store the length for mmap2.
*/
instr_is_syscall(inst) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good for pre-syscall. But after the syscall we need to update the spilled values with whatever the kernel wrote: so we need to also check for instr_is_syscall in the loop labeled /* After each app write, update spilled app values: */. Otherwise, a restore at the end of the block will bring in the old pre-syscall value.

To test, add an asm sequence to drreg-test (ifdef LINUX I guess) with a single block with a syscall in the middle and a pattern the test client can find (see existing DRREG_TEST_38_ASM e.g.) and the test client for that block can then reserve eax and ecx and clobber both register both before and after the syscall.
Verify the mmap succeeded and that the app's eax is correctly restored at the end of the block (maybe have the app check the syscall return value in the next block or sthg).

/* Treat a partial write as a read, to restore rest of reg */
(instr_writes_to_reg(inst, reg, DR_QUERY_INCLUDE_ALL) &&
!instr_writes_to_exact_reg(inst, reg, DR_QUERY_INCLUDE_ALL)) ||
Expand Down Expand Up @@ -1346,7 +1356,13 @@ static drreg_status_t
drreg_restore_reg_now(void *drcontext, instrlist_t *ilist, instr_t *inst,
per_thread_t *pt, reg_id_t reg)
{
if (pt->reg[GPR_IDX(reg)].ever_spilled) {
/* i#6417: in case of a syscall, restore the values of registers since
* they may be used as input parameters for the kernel. For example,
* ECX is used to store the length for mmap2. Register used to store the
* output of the syscall is marked as not spilled, so we need to check for
* syscall as well.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if drreg treated ecx as dead, it wouldn't have preserved the original app value, so it won't have that value to restore. (So why did the tests pass? Did drmemtrace happen to not clobber ecx b/c it picked other scratch regs?). I think we want to keep your original change to not treat a syscall dest reg as dead, and then we'll spill it and it will be available for restores. (And we won't then want this check below for instr_is_syscall).

*/
if (pt->reg[GPR_IDX(reg)].ever_spilled || instr_is_syscall(inst)) {
if (pt->reg[GPR_IDX(reg)].xchg != DR_REG_NULL) {
/* XXX i#511: NYI */
return DRREG_ERROR_FEATURE_NOT_AVAILABLE;
Expand Down
19 changes: 19 additions & 0 deletions suite/tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2171,6 +2171,10 @@ if (X86) # FIXME i#1551, i#1569: port asm to ARM and AArch64
endif (X86)
tobuild(common.getretaddr common/getretaddr.c)

if (UNIX)
tobuild(common.syscall-mmap common/syscall-mmap.c)
endif ()

if (X86) # FIXME i#1551, i#1569: port asm to ARM and AArch64
# XXX i#1287: implement for MacOS
# We do not support -no_early_inject on Android (i#1873).
Expand Down Expand Up @@ -4885,6 +4889,21 @@ if (BUILD_CLIENTS)
"@-tool@invariant_checker" "")
endif ()

if (UNIX)
if (X86)
if (X64)
torunonly_drcacheoff(syscall-mmap-x86-64 common.syscall-mmap
"-record_syscall 9|6" "@-simulator_type@view" "")
else ()
torunonly_drcacheoff(syscall-mmap-x86-32 common.syscall-mmap
"-record_syscall 192|6" "@-simulator_type@view" "")
endif ()
elseif (AARCH64)
torunonly_drcacheoff(syscall-mmap-aarch64 common.syscall-mmap
"-record_syscall 222|6" "@-simulator_type@view" "")
endif ()
endif ()

###########################################################################
# drcpusim tests

Expand Down
49 changes: 49 additions & 0 deletions suite/tests/common/syscall-mmap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/* **********************************************************
* Copyright (c) 2023 Google, Inc. All rights reserved.
* **********************************************************/

/*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
*
* * Redistributions of source code must retain the above copyright notice,
* this list of conditions and the following disclaimer.
*
* * Redistributions in binary form must reproduce the above copyright notice,
* this list of conditions and the following disclaimer in the documentation
* and/or other materials provided with the distribution.
*
* * Neither the name of VMware, Inc. nor the names of its contributors may be
* used to endorse or promote products derived from this software without
* specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL VMWARE, INC. OR CONTRIBUTORS BE LIABLE
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
* SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
* CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
* DAMAGE.
*/

#include <sys/mman.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this file should be in suite/tests/linux/ as mmap is unix-specific (that's the directory that has the mmap.c test as well).

#include <stdio.h>

int
main()
{
void *p;
const size_t size = 0x00008765;
printf("Calling mmap(0, 0x%x, 0x%x, 0x%x, -1, 0)\n", (unsigned int)size,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally we include tools.h and use print for reliable interleaving with client/DR output. Ditto below.

PROT_EXEC | PROT_READ | PROT_WRITE, MAP_ANON | MAP_PRIVATE);
p = mmap(0, size, PROT_EXEC | PROT_READ | PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
if (p == MAP_FAILED) {
printf("mmap ERROR 0x%p\n", p);
return 1;
}
return 0;
}
1 change: 1 addition & 0 deletions suite/tests/common/syscall-mmap.expect
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Calling mmap(0, 0x8765, 0x7, 0x22, -1, 0)
Loading