[rfc] new generic ptrace api for setting syscall info

[Mike Frysinger]

On 25 Oct 2021 13:20, Renzo Davoli wrote:
> (Thank you Eugene to have forwarded Mike's message)
> 
> Mike, I am actually working on the kernel support for PTRACE_SET_SYSCALL_INFO.
> 
> I need it to implement a portable version of the virtualsquare's project vuos.
> https://github.com/virtualsquare/vuos
> 
> The first kernel patch here attached is a proof-of-concept implementation of
> the arch independent part of the support.
> It is already able to update syscall args, exit values.
> 
> The arch specific code needed to complete the support of PTRACE_SET_SYSCALL_INFO
> consists of three (simple) functions:
> void instruction_pointer_set(struct pt_regs *regs, unsigned long val)
> void user_stack_pointer_set(struct pt_regs *regs, unsigned long val)
> void syscall_set_nr(struct task_struct *task, struct pt_regs *regs, int sysno)
> 
> Now, it needs some time-consuming effort to find the right registers for each
> architecture, write the arch-dependent functions and test the code.
> (some of the missing functions are already available for some archs).
> 
> I've also added a new tag PTRACE_SYSCALL_INFO_SECCOMP_SKIP, it should support
> the situation described in the seccomp man page:
> " The tracer can skip the system call by changing the system  call
> number  to  -1.  Alternatively, the tracer can change the system
> call requested by changing the system call  to  a  valid  system
> call  number.   If the tracer asks to skip the system call, then
> the system call will appear to return the value that the  tracer
> puts in the return value register."
> PTRACE_SYSCALL_INFO_SECCOMP_SKIP uses the 'exit' branch of the union in
> struct ptrace_syscall_info.
> When notified for a SECCOMP_RET_TRACE, a process can decide to run the syscall
> maybe using different args (PTRACE_SYSCALL_INFO_SECCOMP) or skip the syscall and
> provide a return value or an errno (PTRACE_SYSCALL_INFO_SECCOMP_SKIP).
> PTRACE_SYSCALL_INFO_SECCOMP_SKIP is meaningful only for PTRACE_SET_SYSCALL_INFO and not
> for PTRACE_GET_SYSCALL_INFO.
> 
> The second patch here attached adds a selftest to check some basic features of
> PTRACE_SET_SYSCALL_INFO.
> 
> I think this proposal needs also:
> * code review
> * support to the discussion on the LKML for the acceptance of the (final) patch
> 
> Any contribution is welcome.

at a high level, i like where this is going.  i think we should break them up as
(1) that's how you get things reviewed & accepted faster,
(2) certain pieces need more help from arches than others,
(3) they look easy to break up & make logically independent,
(4) i selfishly care more for some aspects than others :),
(5) i want to avoid an "all or nothing" type of functionality merge.

the functionality that matters to me with PTRACE_SET_SYSCALL_INFO:
* set syscall nr (e.g. skip it)
* change syscall arguments
* set syscall return value
* set syscall error value
imo, without these, PTRACE_SET_SYSCALL_INFO is not super useful, and seems like
it'd be hard to partially implement any of these.  especially as syscall_set_nr
is the worst performer, and is a bit of the foundation here.  a survey of the
current asm/syscall.h tree:
* syscall_set_nr
  (1/22) only csky has this atm
* syscall_set_arguments
  (14/22) missing alpha arc h8300 hexagon m68k mips parisc sh
* syscall_set_return_value
  (17/22) missing alpha h8300 hexagon m68k sh

you've also thrown in these:
* change IP
* change SP
i'm guessing you've done that simply to mirror the API ?  i would argue that
these are significantly less interesting (i won't say never as i could think
of a few).  so what if we start off with a PTRACE_SET_SYSCALL_INFO that does
not support changing these values.  i'm not sure we could get away with using
a sentinel value here (e.g. require they be set to -1 else we EINVAL), so
maybe requiring them to be the same as would be OK ?  it's reasonable imo to
assume the tracer first called PTRACE_GET_SYSCALL_INFO before modifying, so
they'll have easy access to the current values.  if they don't match, we will
return EINVAL.  this will allow merging of the API without also implementing
the corresponding APIs in asm/ptrace.h.
* instruction_pointer_set
  (11/22) missing alpha arc h8300 hexagon ia64 m68k microblaze nds32 nios2 openrisc xtensa
* stack_pointer_set
  (4/22) only mips riscv sh x86

i guess we're just ignoring arch entirely.  not unreasonable considering
changing execution architecture state is ... not useful, if even possible
for many architectures.  so should we enforce it be 0 and return an EINVAL
if it isn't ?  otherwise it turns into a field we can never use for anything
else in the set path.  afaict, there is no AUDIT_xxx value that is zero atm,
and never will be since it encodes the ELF machine (EM_*), and EM_NONE==0.

next you have seccomp improvements.  these are interesting, but i think
important to not block PTRACE_SET_SYSCALL_INFO on since seccomp support is
a bit spotty already across arches.  and it seems like it calls for a bit
of API bikeshedding.  while your reuse of the existing INFO ops doesn't
feel too terrible since there are direct parallels between them, adding a
new op that only shows up on set feels a little awkward.  it'd be nice if
we could stick to PTRACE_SYSCALL_INFO_SECCOMP somehow.  with the fields we
have now, we have the syscall number, args, and return value.  so can't we
do what you want ?  there's no access to setting an error distinct from a
return value ... but oes seccomp allow that either ?  if not, extending
that limitation from seccomp to ptrace-seccomp doesn't feel bad.

in terms of contributing, i think you can see where my interests align,
and so i'd be happy to chip in for those parts.  but for others, i'll
just wish you the best of luck :).
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20211026/7d229c3c/attachment.bin>