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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y9IPCpYzfGb6k0sF@memverge.com>
Date:   Thu, 26 Jan 2023 00:26:34 -0500
From:   Gregory Price <gregory.price@...verge.com>
To:     Andrei Vagin <avagin@...il.com>
Cc:     Oleg Nesterov <oleg@...hat.com>,
        Gregory Price <gourry.memverge@...il.com>,
        LKML <linux-kernel@...r.kernel.org>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>,
        Gabriel Krisman Bertazi <krisman@...labora.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jonathan Corbet <corbet@....net>, Shuah Khan <shuah@...nel.org>
Subject: Re: [PATCH v6 1/2] ptrace,syscall_user_dispatch: Implement Syscall
 User Dispatch Suspension

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@...hat.com> 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