On 7/27/20 10:48 PM, Michael Schmitz wrote:
On 27/07/20 10:03 PM, John Paul Adrian Glaubitz wrote:
Hi Michael!
On 7/27/20 6:19 AM, Michael Schmitz wrote:
m68k (other than Coldfire) uses syscall_trace for both trace entry
and trace exit. Seccomp support requires separate entry points for
trace entry and exit which are already provided for Coldfire.
Replace syscall_trace by syscall_trace_enter and syscall_trace_leave
in preparation for seccomp support. Check return code of
syscall_trace_enter(), and skip syscall if nonzero. Return code
will be left at what had been set by by ptrace or seccomp.
Correct me if I'm wrong, but shouldn't the skip happen when the return
code is -1? At least that's what we're doing on SuperH and that seems
to be what other architectures are doing as well.
What other non-zero return codes do you expect syscall_trace_enter() to set, and what should the action in entry.S be?
I just don't think we should do it any different than the other architectures
which explicitly compare the return value against -1, i.e. RISC-V and PA-RISC.
(Note that according to my reading, your SH patch does not actually do what your description says. If syscall_trace_enter() returns a positive non-zero value, that value is _not_ used as changed syscall number. SH uses r3 for the syscall number, not r0).
You are right, of course. I somehow mixed that up. You're right, it checks
the return value of syscall_trace_enter and it should skip the syscall
if syscall_trace_enter returns -1.
As far as I can see, any non-zero return code should abort the syscall, so I just test for that (for simplicity). Our use of the tracehook_report_syscall_entry() return code (directly passed back from syscall_trace_enter()) doesn't leave much choice there, see comment in include/linux/tracehook.h.
My point is to stay consistent with the other architectures.
If later on seccomp needs any specific action, it should be easy enough to change the syscall number (offset PT_OFF_ORIG_D0 on the stack) or syscall return code (offset PT_OFF_D0). There's code in kernel/ptrace.c to do that AFAICS.
Changing the behaviour of syscall_trace_enter() to match what other architectures do (return changed syscall number, not error code) is beyond the scope of this patch. I suspect the capability to change syscall numbers from ptrace code does predate the seccomp filter approach, and as m68k has never used it in the past, I don't see a point to add this now.
Yes, I agree and my previous comment in this regard was wrong.
Also, shouldn't that part of the change not be part of the patch that
adds support for SECCOMP filter like in [1]? I don't think it makes
sense to add the return code check unless the rest of SECCOMP filter
has been implemented.
After replacing syscall_trace() by syscall_trace_enter() and syscall_trace_leave(), there is a return code provided by syscall_trace_enter() which we must check, hence I added the check at the same time as replacing syscall_trace() for non-Coldfire m68k.
(I note that the same check should probably be added to coldfire/entry.S.)
Do we actually need to check the return value unless we implement SECCOMP_FILTER?
(Which no doubt we will do ;-)).
I can't test any of the later seccomp related stuff, so I'd rather leave that bit to someone else who can.
I will work on the necessary changes for libseccomp this week, so that we can test
whether the libseccomp live tests pass correctly on a patched kernel.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - glaubitz@xxxxxxxxxx
`. `' Freie Universitaet Berlin - glaubitz@xxxxxxxxxxxxxxxxxxx
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
