Message-ID: <20200704125027.GB21185@willie-the-truck>
Date:   Sat, 4 Jul 2020 13:50:27 +0100
From:   Will Deacon <will@...nel.org>
To:     Keno Fischer <keno@...iacomputing.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Oleg Nesterov <oleg@...hat.com>,
        Kees Cook <keescook@...omium.org>,
        Andy Lutomirski <luto@...capital.net>,
        Will Drewry <wad@...omium.org>
Subject: Re: ptrace: seccomp: Return value when the call was already invalid

On Fri, Jul 03, 2020 at 04:27:37PM -0400, Keno Fischer wrote:
> > > Now, if we have a seccomp filter that simply does
> > > SECCOMP_RET_TRACE, and a ptracer that simply
> > > does PTRACE_CONT
> >
> > Ok, so this means that we're _skipping_ the system call, right?
> 
> If the system call were positive this would result in the system call
> being executed. The notion of "skipping" the syscall is a bit odd in
> this situation. Having the ptracer set the syscallno to -1 is generally
> accepted as the way to do it, but what happens if the syscallno is
> already -1 or negative is underspecified.

Ok. I think it would be sensible for us to have the same behaviour for
all negative system calls though.

> > > then the assert will fire/fail on arm64, but not on x86_64.
> >
> > It feels weird to me that skipping the system call has any effect on the
> > tracee registers...
> 
> I think the correct way to frame it is to ask whether the behavior
> matches that of the tracee in absence of the ptracer. I would argue
> that if the ptracer doesn't explicitly modify register contents, then
> the tracee shouldn't observe any behavior difference.

That's a useful way of thinking about it and is still the case after this
patch. The difference now is that x0 isn't zapped in the case where a
syscall(-1) is skipped.

> > > Interestingly, arm64 does do something different
> > > if the syscall is -1 rather than -10, where early
> > > in the ptrace stop it does.
> > > ```
> > > /* set default errno for user-issued syscall(-1) */
> > > if (scno == NO_SYSCALL)
> > >     regs->regs[0] = -ENOSYS;
> >
> > ... so I think this should be fixed too. How about the diff below?
> 
> I think the patch behavior is better overall, but I'm not sure it's ideal.
> I think the biggest question is what the behavior should be here and
> if we want a behavioral difference between *the syscall was -1 at entry*
> and *the syscall was -1 because the ptracer wanted to skip the syscall*.
> I think there is a bit of a semantic disconnect because "skipping" the
> syscall is not really an operation that the ptracer has at its disposal
> (unless it's using SYSEMU of course). The only thing it can do is set
> the syscall to -1. However, arguably that already has semantics
> (of returning -ENOSYS), so it's not at all clear to me that we should
> deviate from that. Unfortunately, none of this is currently consistent
> across architectures, so I think before we go changing arm64, we
> should decide what we'd like to happen in theory and then see
> what we can do to improve the situation without being too breaking.

I just object to treating a user-issued -1 differently to any other negative
system call. With this patch, they're all treated the same, which is to say
that they return -ENOSYS normally, but when skipped by a tracer (e.g. by
setting the syscall number to -1 or because of a seccomp failure), then
x0 is preserved.

This means that, with this patch, skipping syscall(-1) behaves the same
way as skipping syscall(-2) with mainline today.

Will

Powered by blists - more mailing lists