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

ptrace #736

Merged
merged 5 commits into from
Oct 12, 2020
Merged

ptrace #736

merged 5 commits into from
Oct 12, 2020

Conversation

saagarjha
Copy link
Member

It's back

tbodt
tbodt reviewed May 27, 2020
View reviewed changes
app/AppGroup.h Outdated Show resolved Hide resolved
@saagarjha saagarjha force-pushed the ptrace branch 4 times, most recently from a437e5e to 4bb76a9 Compare May 28, 2020 03:35
@saagarjha saagarjha force-pushed the ptrace branch 6 times, most recently from 33c0421 to fc70899 Compare June 10, 2020 01:43
@saagarjha saagarjha marked this pull request as ready for review June 10, 2020 01:43
@saagarjha
Copy link
Member Author

What kinda works:

  • Launching GDB
  • Starting a process
  • Setting symbolic breakpoints in libc
  • Setting breakpoints at an address
  • Reading registers
  • Single stepping

What does not work:

  • You tell me, but I will probably be unsurprised

@saagarjha saagarjha force-pushed the ptrace branch 5 times, most recently from 70665ee to f1eb124 Compare June 15, 2020 01:26
tbodt
tbodt reviewed Jun 15, 2020
View reviewed changes
Copy link
Member

@tbodt tbodt left a comment

Choose a reason for hiding this comment

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

Thanks for writing this, it's really cool to finally have it (mostly) working!

fs/proc/pid.c Show resolved Hide resolved
kernel/calls.c
});
unlock(&pids_lock);
} else if (interrupt == INT_DEBUG) {
lock(&pids_lock);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't need to lock pids_lock, as the signal is being sent to current?

Copy link
Member Author

@saagarjha saagarjha Jun 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

signal_delivery_stop requires pids_lock:

ish/kernel/signal.c

Line 109 in f1eb124

unlock(&pids_lock);
. I'm open to suggestions on how I should handle this better, but until now I have just kept all this code under the assumption that this lock is held.

kernel/calls.c
} else if (interrupt == INT_DEBUG) {
lock(&pids_lock);
send_signal(current, SIGTRAP_, (struct siginfo_) {
.sig = SIGTRAP_,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be needed

ish/kernel/signal.c

Line 60 in 95dc3f5

sigqueue->info.sig = sig;

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't have that at first, but IIRC this was necessary because the signal is swallowed by signal_delivery_stop and made available via ptrace(PTRACE_GETSIGINFO, …), at which point the signal needed to be correct.

kernel/exec.c
@@ -616,6 +616,17 @@ int __do_execve(const char *file, struct exec_args argv, struct exec_args envp)

current->did_exec = true;
vfork_notify(current);

if (current->traced) {
lock(&pids_lock);
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Taking the lock, or sending a signal? I think both are necessary…

Copy link
Member

Choose a reason for hiding this comment

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

Taking the lock, but no need to discuss it here, there are like 500 other comments about it

kernel/task.h Outdated Show resolved Hide resolved
kernel/fork.c Outdated

if (current->traced) {
current->ptrace.trap_event = PTRACE_EVENT_FORK_;
lock(&pids_lock);
Copy link
Member

Choose a reason for hiding this comment

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

Missing unlock(&pids_lock), but I dunno if the lock is necessary (see other comments)

kernel/task.h Outdated Show resolved Hide resolved
kernel/task.h Outdated Show resolved Hide resolved
kernel/ptrace.c Outdated Show resolved Hide resolved
kernel/signal.c Outdated Show resolved Hide resolved
@saagarjha saagarjha force-pushed the ptrace branch 3 times, most recently from aa2ae50 to 38806cc Compare June 16, 2020 02:17
tbodt
tbodt reviewed Sep 1, 2020
View reviewed changes
kernel/ptrace.c Show resolved Hide resolved
@asdfugil asdfugil mentioned this pull request Sep 7, 2020
Closed
@abdullah0sedeq

This comment has been minimized.

Sign in to view
@saagarjha saagarjha force-pushed the ptrace branch 4 times, most recently from 922102b to 1d714a7 Compare October 11, 2020 01:50
tbodt
tbodt reviewed Oct 11, 2020
View reviewed changes
kernel/group.c Show resolved Hide resolved
emu/memory.c Show resolved Hide resolved
tbodt
tbodt reviewed Oct 11, 2020
View reviewed changes
kernel/ptrace.c Outdated
user_regs_->edi = cpu->edi;
user_regs_->ebp = cpu->ebp;
user_regs_->eax = cpu->eax;
// user_regs_->xds = cpu->xds;
Copy link
Member

Choose a reason for hiding this comment

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

Indentation on these comments is a bit funky compared to set_user_regs

@saagarjha saagarjha force-pushed the ptrace branch 3 times, most recently from c231b48 to 77aa1b2 Compare October 12, 2020 01:17
@tbodt tbodt force-pushed the ptrace branch 2 times, most recently from 213fbc3 to 77aa1b2 Compare October 12, 2020 02:32
@tbodt tbodt merged commit 7d7c576 into ish-app:master Oct 12, 2020
@saagarjha saagarjha mentioned this pull request Oct 12, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbodt tbodt tbodt left review comments

@DuyAnhComputer DuyAnhComputer DuyAnhComputer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@saagarjha @abdullah0sedeq @tbodt @DuyAnhComputer