Page MenuHomeFreeBSD

ptrace(2): add PT_SC_REMOTE remote syscall request
ClosedPublic

Authored by kib on Dec 2 2022, 4:01 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jan 10, 5:35 PM
Unknown Object (File)
Tue, Jan 10, 5:34 PM
Unknown Object (File)
Tue, Jan 10, 5:34 PM
Unknown Object (File)
Tue, Jan 10, 5:33 PM
Unknown Object (File)
Tue, Jan 10, 5:33 PM
Unknown Object (File)
Tue, Jan 10, 5:32 PM
Unknown Object (File)
Mon, Jan 9, 6:42 PM
Unknown Object (File)
Mon, Jan 9, 6:42 PM

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kib requested review of this revision.Dec 2 2022, 4:01 AM

What's the motivation for adding this feature?

sys/kern/kern_sig.c
2680 ↗(On Diff #113767)

Should we try to respect trap_enotcap somehow?

2683 ↗(On Diff #113767)

Why not try to update td_cowgen here?

2723 ↗(On Diff #113767)

The duplication between this function and syscallenter() isn't ideal. I wonder if the latter can be broken up into multiple inline functions, and some of them reused here.

2756 ↗(On Diff #113767)

Extra newline.

kib marked 4 inline comments as done.Dec 5 2022, 9:54 PM
kib added inline comments.
sys/kern/kern_sig.c
2680 ↗(On Diff #113767)

I thought about it, and decided not to do. The main reason is that I do not additionally add a recursion to generate new signal from ptracestop() (it is there anyway, but I do not want to add more). Logical reason is that the code is requested by the debugger already, so it should be capable to see the ECAPMODE by itself, without additional help.

If you still consider it useful to generate SIGTRAP there, please explain.

2723 ↗(On Diff #113767)

I considered it very briefly, but the functions appear to be too small and ad-hoc, IMO. This can be reconsidered, but for me it does not look compelling.

kib marked 2 inline comments as done.Dec 5 2022, 9:59 PM

What's the motivation for adding this feature?

There are some programs doing exactly that, by hacking a place in the .text and injecting their code. I remember that reptyr (ports/sysutils/reptyr) does that. Also some time ago I listen to a talk describing the same hack. Since we already has the hardest part done, the ability to request a code to be executed in the remote context (for coredumps), I finally decided to add this feature in a regular and hopefully more convenient way.

This is the primary reason why Kyle is added as a reviewer.

lib/libc/sys/ptrace.2
996 ↗(On Diff #113767)

Could we add some context about which thread is chosen?

Update CoW fields for curthread on remote syscall req.

kib marked an inline comment as done.

ptrace(2): add a section on the target selection

Hmm, GDB implements this sort of thing by changing registers to point to a block of code inserted into the address space.

In D37590#855703, @jhb wrote:

Hmm, GDB implements this sort of thing by changing registers to point to a block of code inserted into the address space.

Exactly. This is what I am trying to provide a (much) more convenient interface for.

+1; iirc this kills off the only MD part of reptyr, which was mainly the registers that it swaps out for syscall args. It doesn't go as far as a whole block of code, though, iirc- just proceed to syscall entry, swap out syscall args with its own, then rewind %Ip to execute victim's syscall. Still uglier than it needs to be...

In D37590#855715, @kib wrote:
In D37590#855703, @jhb wrote:

Hmm, GDB implements this sort of thing by changing registers to point to a block of code inserted into the address space.

Exactly. This is what I am trying to provide a (much) more convenient interface for.

Well, except that GDB does this for all sorts of things that is more general than just invoking a system call. I can't see it ever making use of this as it is too limited. That said, there's lots of this that GDB does more of on Linux than on FreeBSD, and it might be that using this to call mmap(MAP_ANON...) to get the initial set of scratch space to which compiled code can be written might be useful. Also for things like displaced stepping that is not implemented on FreeBSD.

In D37590#855732, @jhb wrote:
In D37590#855715, @kib wrote:
In D37590#855703, @jhb wrote:

Hmm, GDB implements this sort of thing by changing registers to point to a block of code inserted into the address space.

Exactly. This is what I am trying to provide a (much) more convenient interface for.

Well, except that GDB does this for all sorts of things that is more general than just invoking a system call. I can't see it ever making use of this as it is too limited. That said, there's lots of this that GDB does more of on Linux than on FreeBSD, and it might be that using this to call mmap(MAP_ANON...) to get the initial set of scratch space to which compiled code can be written might be useful. Also for things like displaced stepping that is not implemented on FreeBSD.

Hmm, well, seems on Linux the displaced stepping thing just reuses a block of instructions at the entry point assuming they won't get re-used once the program has started execution.

Any further opinions on this? Could anybody review the patch, please?

lib/libc/sys/ptrace.2
130 ↗(On Diff #113863)
132 ↗(On Diff #113863)
135 ↗(On Diff #113863)
136 ↗(On Diff #113863)
138 ↗(On Diff #113863)
140 ↗(On Diff #113863)

(I think "legitimate" is redundant.)

142 ↗(On Diff #113863)
143 ↗(On Diff #113863)
1004 ↗(On Diff #113863)
1010 ↗(On Diff #113863)
1014 ↗(On Diff #113863)
1016 ↗(On Diff #113863)
1049 ↗(On Diff #113863)

Maybe note specifically that the process can deadlock. In this situation, the only way out is to kill the process.

sys/kern/kern_sig.c
2673 ↗(On Diff #113863)
2751 ↗(On Diff #113863)

As a matter of naming, we have _remotereq, _syscallrq and _coredump. IMHO it'd be nicer to be more consistent and just use req as the suffix everywhere, also for TDB_ flags. rq is somewhat cryptic to me.

2758 ↗(On Diff #113863)

Should we assert that the flag is still set here?

2680 ↗(On Diff #113767)

It's a corner case, not very important. Maybe this behaviour is actually preferable. I would perhaps add a comment noting that trap_enotcap (and the corresponding procctl() flag) are ignored in this mode.

2723 ↗(On Diff #113767)

The amount of duplication is still worrisome to me. I still don't have a concrete suggestion to resolve it though.

sys/kern/sys_process.c
1646 ↗(On Diff #113863)

Why exactly is this assertion true? What if two threads in the tracing process both issue a remote syscall request?

sys/sys/ptrace.h
200 ↗(On Diff #113863)

Should we use register_t instead of u_long to represent parameters? That would be consistent with struct syscall_args, and the memcpy in the PT_SC_REMOTE effectively assumes that the types are compatible already.

kib marked 19 inline comments as done.Dec 16 2022, 7:43 PM
kib added inline comments.
sys/kern/sys_process.c
1646 ↗(On Diff #113863)

When I added PT_COREDUMP, I made ptrace(2) block other requests until the current one finished. Please see P2_PTRACEREQ code near the (long) beginning of kern_ptrace().

sys/sys/ptrace.h
200 ↗(On Diff #113863)

I changed the type to syscallarg_t.

kib marked 2 inline comments as done.

RQ->REQ
u_long->syscallarg_t
Editing of the man page changes

markj added inline comments.
lib/libc/sys/ptrace.2
1057 ↗(On Diff #114205)
This revision is now accepted and ready to land.Dec 19 2022, 3:26 PM
kib marked an inline comment as done.Dec 19 2022, 11:50 PM