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

[Andrei Vagin]

On Wed, Jan 25, 2023 at 9:26 PM Gregory Price
<gregory.price@xxxxxxxxxxxx> wrote:
>
> 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)

I don't understand why it matters here. If we have
PTRACE_O_SUSPEND_SUD, all these actions
happen when the flag is set and so syscall_user_dispatch always
returns 0 and we reach ptrace
hooks.

If we don't have PTRACE_O_SUSPEND_SUD, we need to disable SUD after attaching to
a process or we need to inject our code into the region that is not under SUD.

> * masking everything but SIGSTOP and waiting for a STOP

I am not sure that I understand what part of criu you refer to here.

>
> 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

CRIU sets breakpoints into the parasite code that is part of the criu
code base and it is under
our control. Second, the parasite code doesn't use libc calls.

> * masking SIGSYS crashes a program on next dispatch if SUD is enabled
>

This is right and criu should not trigger dispatched syscalls ;)

Thanks,
Andrei