Skip to content

Commit

Permalink
Fix debugging inside proot
Browse files Browse the repository at this point in the history 
Fix killing of tracees that haven't issued any syscall yet,
previously we've used last_restart_how which was 0,
which happened to be PTRACE_TRACEME which caused hang

Suppress seccomp SIGSYS also when it happens on ptraced process
and don't deliver SIGSYS to ptracer

Improve logging, show detected syscall order when we detect seccomp
  • Loading branch information
@michalbednarski
michalbednarski committed Apr 29, 2018
1 parent 47138da commit 88b915e
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 7 deletions.
25 changes: 21 additions & 4 deletions src/ptrace/wait.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ int translate_wait_exit(Tracee *ptracer)
bool handle_ptracee_event(Tracee *ptracee, int event)
{
bool handled_by_proot_first = false;
bool handled_by_proot_first_may_suppress = false;
Tracee *ptracer = PTRACEE.ptracer;
bool keep_stopped;

Expand Down Expand Up @@ -287,6 +288,11 @@ bool handle_ptracee_event(Tracee *ptracee, int event)
* ptrace emulation. */
return false;

case SIGSYS:
handled_by_proot_first = true;
handled_by_proot_first_may_suppress = true;
break;

default:
PTRACEE.tracing_started = true;
break;
Expand Down Expand Up @@ -314,10 +320,21 @@ bool handle_ptracee_event(Tracee *ptracee, int event)
signal = handle_tracee_event(ptracee, PTRACEE.event4.proot.value);
PTRACEE.event4.proot.value = signal;

/* The computed signal is always 0 since we can come
* in this block only on sysexit and special events
* (as for now). */
assert(signal == 0);
if (handled_by_proot_first_may_suppress) {
/* If we've decided to suppress signal
* (e.g. because seccomp policy blocked syscall
* but we emulate that syscall),
* don't notify ptracer and let ptracee resume. */
if (signal == 0) {
restart_tracee(ptracee, 0);
return true;
}
} else {
/* The computed signal is always 0 since we can come
* in this block only on sysexit and special events
* (as for now). */
assert(signal == 0);
}
}

/* Remember what the new event is, this will be required by
Expand Down
2 changes: 1 addition & 1 deletion src/syscall/syscall.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,10 +190,10 @@ void translate_syscall(Tracee *tracee)
int push_regs_status = push_specific_regs(tracee, override_sysnum);

/* Handle inability to change syscall number */
print_current_regs(tracee, 4, "pre_push");
if (push_regs_status < 0 && override_sysnum) {
word_t orig_sysnum = peek_reg(tracee, ORIGINAL, SYSARG_NUM);
word_t current_sysnum = peek_reg(tracee, CURRENT, SYSARG_NUM);
print_current_regs(tracee, 4, "pre_push");
if (orig_sysnum != current_sysnum) {
/* Restart current syscall as chained */
if (current_sysnum != SYSCALL_AVOIDER) {
Expand Down
10 changes: 8 additions & 2 deletions src/tracee/event.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -540,10 +540,11 @@ int handle_tracee_event(Tracee *tracee, int tracee_status)
signal = 0;

if (!seccomp_detected) {
VERBOSE(tracee, 1, "ptrace acceleration (seccomp mode 2) enabled");
tracee->seccomp = ENABLED;
seccomp_detected = true;
seccomp_after_ptrace_enter = !IS_IN_SYSENTER(tracee);
VERBOSE(tracee, 1, "ptrace acceleration (seccomp mode 2, %s syscall order) enabled",
seccomp_after_ptrace_enter ? "new" : "old");
}

tracee->skip_next_seccomp_signal = false;
Expand Down Expand Up @@ -609,7 +610,9 @@ int handle_tracee_event(Tracee *tracee, int tracee_status)
case SIGTRAP | PTRACE_EVENT_EXEC << 8:
case SIGTRAP | PTRACE_EVENT_EXIT << 8:
signal = 0;
tracee->restart_how = tracee->last_restart_how;
if (tracee->last_restart_how) {
tracee->restart_how = tracee->last_restart_how;
}
break;

case SIGSTOP:
Expand All @@ -633,6 +636,8 @@ int handle_tracee_event(Tracee *tracee, int tracee_status)
ptrace(PTRACE_GETSIGINFO, tracee->pid, NULL, &siginfo);
if (siginfo.si_code == SYS_SECCOMP) {
if (tracee->skip_next_seccomp_signal) {
VERBOSE(tracee, 4, "suppressed SIGSYS after void syscall");
tracee->skip_next_seccomp_signal = false;
signal = 0;
} else {
signal = handle_seccomp_event(tracee);
Expand Down Expand Up @@ -678,6 +683,7 @@ bool restart_tracee(Tracee *tracee, int signal)

/* Restart the tracee and stop it at the next instruction, or
* at the next entry or exit of a system call. */
assert(tracee->restart_how != 0);
status = ptrace(tracee->restart_how, tracee->pid, NULL, signal);
if (status < 0)
return false; /* The process likely died in a syscall. */
Expand Down

10 comments on commit 88b915e

@corbinlc
Copy link
Contributor

Choose a reason for hiding this comment

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

Michal,

I am a trying to understand something with regards to the Oreo 8.0+ issue. I understand most of what you have done, but I am curious what is the correct way to fully emulate a system call that has caused a SIGSYS because of the blocking of certain system calls. Specifically, I am working on one example... PR_symlink, but you can pick any one... like PR_chmod. I will later make it do the right thing, but for right now, I want to have restart the system call (done), do something in an extension at SYSENTER_END to emulate the behavior (done) and then I want and then change the sysnum to PR_void (done). All is behaving as I would expect, but PR_void is blocked when it is run and things are terminated. I want PR_void to be allowed to progress, like what you are doing for the use of PR_void when originally it was PR_ptrace. Can you describe the general approach to handling this for some other case than PR_ptrace?

Thanks,
Corbin

P.S. I sent an email to you about PRoot development. Tell me if you didn't get it. I just pulled your email from some info off github and I am not sure it is correct.

@michalbednarski
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PR_ptrace is in normal case (translate_syscall_enter/_exit). SIGSYS is unlike normal syscalls handled by proot, SIGSYS happens after syscall was cancelled by system, so you don't need PR_void.

See tracee/seccomp.c pull request where I've got support for accept->accept4 translation. This translation happens after syscall was blocked by system so nothing has to be cancelled (PR_void), Originally this mechanism was made to just replace SIGSYS signal with ENOSYS errno, above accept translation isn't completely transparent, doesn't restore regs, but libc generally isn't bothered by that. Note that in current implementation accept4 syscall inserted by SIGSYS handler will be intercepted by proot (so it can translate paths then, so this should be used to just switch syscall and path translation will happen later)

Regarding your offer, thanks, but I'm happy with my current job. PRoot stays as my side project (which means not very much time I can spend on development, but I'm happy to answer questions/review not working patches/accept pull requests to this fork)

@corbinlc
Copy link
Contributor

Choose a reason for hiding this comment

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

I copied the example for accept->accept4 but did symlink->symlinkat and the kernel gave another sigsys. I have a pixel running 8.1.0

@corbinlc
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, but I mucked with some of the sysargs in the same place to mat symlinkat

@corbinlc
Copy link
Contributor

Choose a reason for hiding this comment

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

Here is specifically what I did.
case PR_symlink:
VERBOSE(tracee, 1, "In handle seccomp symlink");

            //convert this to
            //int symlinkat(const char *target, int newdirfd, const char *linkpath);
            set_sysnum(tracee, PR_symlinkat);
            poke_reg(tracee, SYSARG_3, peek_reg(tracee, ORIGINAL, SYSARG_2));
            poke_reg(tracee, SYSARG_2, AT_FDCWD);

            /* Move the instruction pointer back to the original trap */
            instr_pointer = peek_reg(tracee, CURRENT, INSTR_POINTER);
            poke_reg(tracee, INSTR_POINTER, instr_pointer - SYSTRAP_SIZE);
            /* Break as usual on entry to syscall */
            tracee->restart_how = PTRACE_SYSCALL;
            push_specific_regs(tracee, true);

            /* Swallow signal */
            signal = 0;
            break;

But oddly, at sysenter start the syscall is still PR_symlink and then PRoot turns it into PR_void even though nothing has told it to do so. Then it runs PR_void and that cause things to terminate. See any obvious idiocy?

What I am doing now instead, is I am notifying extensions on a SIGSYS event. Then I am emulating the behavior withing an extension. I see the notify_extension(SIGSYS_OCC...) working and getting to the extension, now I need to do something with it. Does this sound reasonable to you?

@corbinlc
Copy link
Contributor

Choose a reason for hiding this comment

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

specifically, fake_id0 is where a lot of the code will be for handling all the chown, chmod, etc etc

@corbinlc
Copy link
Contributor

Choose a reason for hiding this comment

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

About working together professionally... that is cool. If there is any way we can help you keep proot moving along, just tell me.

@michalbednarski
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've tried your snippet and with minor customization got it working:

  • Use CURRENT instead of ORIGINAL, this handler currently doesn't fill ORIGINAL
  • proot for AArch64 doesn't recognize PR_symlink and if it is called there it'll be seen as PR_void, did you really see PR_symlink at sysenter start? And if yes then another reason for PR_void is proot deciding syscall has failed and proot substituted error value and cancelled syscall (but in that case it shouldn't reach handle_seccomp_event

@corbinlc
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Even when fixing the CURRENT vs ORIGINAL issue, I still see PR_symlink at sysenter start, even though it should be PR_symlinkat now. Then at sysenter end I see PR_void.

My technique of fully emulating what I want inside an extension seems to be working though. Though I am not sure why the other method does not work for me.

@corbinlc
Copy link
Contributor

Choose a reason for hiding this comment

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

I am using proot built for arm, not arm64 in this case.

Please sign in to comment.