Re: [PATCH v6 1/2] ptrace,syscall_user_dispatch: Implement Syscall User Dispatch Suspension

[Gregory Price]

On Wed, Jan 25, 2023 at 08:54:48PM -0800, Andrei Vagin wrote:
> On Wed, Jan 25, 2023 at 4:30 PM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> >
> > On 01/24, Gregory Price wrote:
> > >
> > > Adds PTRACE_O_SUSPEND_SYSCALL_USER_DISPATCH to ptrace options, and
> > > modify Syscall User Dispatch to suspend interception when enabled.
> > >
> > > This is modeled after the SUSPEND_SECCOMP feature, which suspends
> > > SECCOMP interposition.  Without doing this, software like CRIU will
> > > inject system calls into a process and be intercepted by Syscall
> > > User Dispatch, either causing a crash (due to blocked signals) or
> > > the delivery of those signals to a ptracer (not the intended behavior).
> >
> > Cough... Gregory, I am sorry ;)
> >
> > but can't we drop this patch to ?
> >
> > CRIU needs to do PTRACE_SET_SYSCALL_USER_DISPATCH_CONFIG and check
> > config->mode anyway as we discussed.
> >
> > Then it can simply set *config->selector = SYSCALL_DISPATCH_FILTER_ALLOW
> > with the same effect, no?
> 
> Oleg,
> 
> PTRACE_O_SUSPEND_SYSCALL_USER_DISPATCH is automatically cleared when
> a tracer detaches. It is critical when tracers detach due to unexpected
> reasons
> (crashes, killed by oom, etc). In such cases, we want to be sure that a
> tracee will continue
> running from the point where it has been trapped.
> 
> Thanks,
> Andrei

There might be a better place for the full C/R discussion, but it's worth
the extra context to hash out the SUSPEND flag.

The relevant kernel code i'm concerned about:

static long syscall_trace_enter(struct pt_regs *regs, long syscall,
        unsigned long work)
{
  long ret = 0;
  /* ... snip ... do syscall user dispatch */
  if (work & SYSCALL_WORK_SYSCALL_USER_DISPATCH) {
    if (syscall_user_dispatch(regs))
      return -1L;
   }
 
  /* Handle ptrace */
  if (work & (SYSCALL_WORK_SYSCALL_TRACE | SYSCALL_WORK_SYSCALL_EMU)) {
    ret = ptrace_report_syscall_entry(regs);
    if (ret || (work & SYSCALL_WORK_SYSCALL_EMU))
      return -1L;
  }

  /* Do seccomp after ptrace, to catch any tracer changes. */
  if (work & SYSCALL_WORK_SECCOMP) {
    ret = __secure_computing(NULL);
  }
	/* ... snip ... */
}

The problem i'm seeing with PTRACE_O_SUSPEND_SUD is that SUD comes before
ptrace, while Seccomp comes after.

CRIU seems to use a few different methods to quiesce:
* ptrace syscall entry traps
* breakpoints (on sigreturn it seems)
* masking everything but SIGSTOP and waiting for a STOP

SUD represent an issue in all three cases
* syscall dispatch preempts ptrace traps (though syscalls may come
  from the exclusion area, so it should hit eventually)
* sigreturn can be changed (glibc prevents this, but the raw sigaction
  syscall will take whatever address you give it)
* masking SIGSYS crashes a program on next dispatch if SUD is enabled

Most of this can be worked around.

My concern is whether there is any injection occuring during the quiesce
phase.  If there is no injection - this SUSPEND isn't needed during
quiesce (and in fact, it's dangerous).  If there is injection, then
the current CRIU quiesce method is incompatible with SUD anyway and this
would take more investigation to determine a solution.

Once quiesced, however, SUD has to be disabled to allow injections. And
I think auto-clearing of the PTRACE Options on detach is a good reason
to keep it.


So basic questions are:
1) Andrei do you think any injection occurs during quiesce that can't be
   worked around?

2) Oleg is the auto-clearing nature of the flag sufficient justification
   for keeping SUSPEND?

~Gregory