2019-04-08 19:35:20

by Dmitry V. Levin

[permalink] [raw]
Subject: [PATCH linux-next v9 0/7] ptrace: add PTRACE_GET_SYSCALL_INFO request

[I suggest to stop waiting for more acks and merge this into linux-next as is.]

PTRACE_GET_SYSCALL_INFO is a generic ptrace API that lets ptracer obtain
details of the syscall the tracee is blocked in.

There are two reasons for a special syscall-related ptrace request.

Firstly, with the current ptrace API there are cases when ptracer cannot
retrieve necessary information about syscalls. Some examples include:
* The notorious int-0x80-from-64-bit-task issue. See [1] for details.
In short, if a 64-bit task performs a syscall through int 0x80, its tracer
has no reliable means to find out that the syscall was, in fact,
a compat syscall, and misidentifies it.
* Syscall-enter-stop and syscall-exit-stop look the same for the tracer.
Common practice is to keep track of the sequence of ptrace-stops in order
not to mix the two syscall-stops up. But it is not as simple as it looks;
for example, strace had a (just recently fixed) long-standing bug where
attaching strace to a tracee that is performing the execve system call
led to the tracer identifying the following syscall-exit-stop as
syscall-enter-stop, which messed up all the state tracking.
* Since the introduction of commit 84d77d3f06e7e8dea057d10e8ec77ad71f721be3
("ptrace: Don't allow accessing an undumpable mm"), both PTRACE_PEEKDATA
and process_vm_readv become unavailable when the process dumpable flag
is cleared. On such architectures as ia64 this results in all syscall
arguments being unavailable for the tracer.

Secondly, ptracers also have to support a lot of arch-specific code for
obtaining information about the tracee. For some architectures, this
requires a ptrace(PTRACE_PEEKUSER, ...) invocation for every syscall
argument and return value.

PTRACE_GET_SYSCALL_INFO returns the following structure:

struct ptrace_syscall_info {
__u8 op; /* PTRACE_SYSCALL_INFO_* */
__u32 arch __attribute__((__aligned__(sizeof(__u32))));
__u64 instruction_pointer;
__u64 stack_pointer;
union {
struct {
__u64 nr;
__u64 args[6];
} entry;
struct {
__s64 rval;
__u8 is_error;
} exit;
struct {
__u64 nr;
__u64 args[6];
__u32 ret_data;
} seccomp;
};
};

The structure was chosen according to [2], except for the following
changes:
* seccomp substructure was added as a superset of entry substructure;
* the type of nr field was changed from int to __u64 because syscall
numbers are, as a practical matter, 64 bits;
* stack_pointer field was added along with instruction_pointer field
since it is readily available and can save the tracer from extra
PTRACE_GETREGS/PTRACE_GETREGSET calls;
* arch is always initialized to aid with tracing system calls
* such as execve();
* instruction_pointer and stack_pointer are always initialized
so they could be easily obtained for non-syscall stops;
* a boolean is_error field was added along with rval field, this way
the tracer can more reliably distinguish a return value
from an error value.

strace has been ported to PTRACE_GET_SYSCALL_INFO.
Starting with release 4.26, strace uses PTRACE_GET_SYSCALL_INFO API
as the preferred mechanism of obtaining syscall information.

[1] https://lore.kernel.org/lkml/CA+55aFzcSVmdDj9Lh_gdbz1OzHyEm6ZrGPBDAJnywm2LF_eVyg@mail.gmail.com/
[2] https://lore.kernel.org/lkml/CAObL_7GM0n80N7J_DFw_eQyfLyzq+sf4y2AvsCCV88Tb3AwEHA@mail.gmail.com/

---

Notes:
v9:
* Rebased to linux-next again due to syscall_get_arguments() signature change.

v8:
* Moved syscall_get_arch() specific patches to a separate patchset
which is now merged into audit/next tree.
* Rebased to linux-next.
* Moved ptrace_get_syscall_info code under #ifdef CONFIG_HAVE_ARCH_TRACEHOOK,
narrowing down the set of architectures supported by this implementation
back to those 19 that enable CONFIG_HAVE_ARCH_TRACEHOOK because
I failed to get all syscall_get_*(), instruction_pointer(),
and user_stack_pointer() functions implemented on some niche
architectures. This leaves the following architectures out:
alpha, h8300, m68k, microblaze, and unicore32.

v7:
* Rebased to v5.0-rc1.
* 5 arch-specific preparatory patches out of 25 have been merged
into v5.0-rc1 via arch trees.

v6:
* Add syscall_get_arguments and syscall_set_arguments wrappers
to asm-generic/syscall.h, requested by Geert.
* Change PTRACE_GET_SYSCALL_INFO return code: do not take trailing paddings
into account, use the end of the last field of the structure being written.
* Change struct ptrace_syscall_info:
* remove .frame_pointer field, is is not needed and not portable;
* make .arch field explicitly aligned, remove no longer needed
padding before .arch field;
* remove trailing pads, they are no longer needed.

v5:
* Merge separate series and patches into the single series.
* Change PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} values as requested by Oleg.
* Change struct ptrace_syscall_info: generalize instruction_pointer,
stack_pointer, and frame_pointer fields by moving them from
ptrace_syscall_info.{entry,seccomp} substructures to ptrace_syscall_info
and initializing them for all stops.
* Add PTRACE_SYSCALL_INFO_NONE, set it when not in a syscall stop,
so e.g. "strace -i" could use PTRACE_SYSCALL_INFO_SECCOMP to obtain
instruction_pointer when the tracee is in a signal stop.
* Patch all remaining architectures to provide all necessary
syscall_get_* functions.
* Make available for all architectures: do not conditionalize on
CONFIG_HAVE_ARCH_TRACEHOOK since all syscall_get_* functions
are implemented on all architectures.
* Add a test for PTRACE_GET_SYSCALL_INFO to selftests/ptrace.

v4:
* Do not introduce task_struct.ptrace_event,
use child->last_siginfo->si_code instead.
* Implement PTRACE_SYSCALL_INFO_SECCOMP and ptrace_syscall_info.seccomp
support along with PTRACE_SYSCALL_INFO_{ENTRY,EXIT} and
ptrace_syscall_info.{entry,exit}.

v3:
* Change struct ptrace_syscall_info.
* Support PTRACE_EVENT_SECCOMP by adding ptrace_event to task_struct.
* Add proper defines for ptrace_syscall_info.op values.
* Rename PT_SYSCALL_IS_ENTERING and PT_SYSCALL_IS_EXITING to
PTRACE_EVENTMSG_SYSCALL_ENTRY and PTRACE_EVENTMSG_SYSCALL_EXIT
* and move them to uapi.

v2:
* Do not use task->ptrace.
* Replace entry_info.is_compat with entry_info.arch, use syscall_get_arch().
* Use addr argument of sys_ptrace to get expected size of the struct;
return full size of the struct.

Dmitry V. Levin (6):
nds32: fix asm/syscall.h # waiting for ack since early January
hexagon: define syscall_get_error() and syscall_get_return_value() # waiting for ack since November
mips: define syscall_get_error() # acked
parisc: define syscall_get_error() # acked
powerpc: define syscall_get_error() # waiting for ack since early December
selftests/ptrace: add a test case for PTRACE_GET_SYSCALL_INFO

Elvira Khabirova (1):
ptrace: add PTRACE_GET_SYSCALL_INFO request # reviewed

arch/hexagon/include/asm/syscall.h | 14 +
arch/mips/include/asm/syscall.h | 6 +
arch/nds32/include/asm/syscall.h | 27 +-
arch/parisc/include/asm/syscall.h | 7 +
arch/powerpc/include/asm/syscall.h | 10 +
include/linux/tracehook.h | 9 +-
include/uapi/linux/ptrace.h | 35 +++
kernel/ptrace.c | 103 ++++++-
tools/testing/selftests/ptrace/.gitignore | 1 +
tools/testing/selftests/ptrace/Makefile | 2 +-
.../selftests/ptrace/get_syscall_info.c | 271 ++++++++++++++++++
11 files changed, 470 insertions(+), 15 deletions(-)
create mode 100644 tools/testing/selftests/ptrace/get_syscall_info.c

--
ldv


2019-04-08 18:44:18

by Dmitry V. Levin

[permalink] [raw]
Subject: [PATCH linux-next v9 3/7] mips: define syscall_get_error()

syscall_get_error() is required to be implemented on all
architectures in addition to already implemented syscall_get_nr(),
syscall_get_arguments(), syscall_get_return_value(), and
syscall_get_arch() functions in order to extend the generic
ptrace API with PTRACE_GET_SYSCALL_INFO request.

Acked-by: Paul Burton <[email protected]>
Cc: Elvira Khabirova <[email protected]>
Cc: Eugene Syromyatnikov <[email protected]>
Cc: Ralf Baechle <[email protected]>
Cc: James Hogan <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: [email protected]
Signed-off-by: Dmitry V. Levin <[email protected]>
---

Notes:
v9: unchanged
v8: unchanged
v7: added Acked-by
v6: unchanged
v5: initial revision

arch/mips/include/asm/syscall.h | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
index acf80ae0a430..83bb439597d8 100644
--- a/arch/mips/include/asm/syscall.h
+++ b/arch/mips/include/asm/syscall.h
@@ -89,6 +89,12 @@ static inline unsigned long mips_get_syscall_arg(unsigned long *arg,
unreachable();
}

+static inline long syscall_get_error(struct task_struct *task,
+ struct pt_regs *regs)
+{
+ return regs->regs[7] ? -regs->regs[2] : 0;
+}
+
static inline long syscall_get_return_value(struct task_struct *task,
struct pt_regs *regs)
{
--
ldv

2019-04-08 18:45:03

by Dmitry V. Levin

[permalink] [raw]
Subject: [PATCH linux-next v9 4/7] parisc: define syscall_get_error()

syscall_get_error() is required to be implemented on all
architectures in addition to already implemented syscall_get_nr(),
syscall_get_arguments(), syscall_get_return_value(), and
syscall_get_arch() functions in order to extend the generic
ptrace API with PTRACE_GET_SYSCALL_INFO request.

Acked-by: Helge Deller <[email protected]> # parisc
Cc: James E.J. Bottomley <[email protected]>
Cc: Elvira Khabirova <[email protected]>
Cc: Eugene Syromyatnikov <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: [email protected]
Signed-off-by: Dmitry V. Levin <[email protected]>
---

Notes:
v9: unchanged
v8: added Acked-by
v7: unchanged
v6: unchanged
v5: initial revision

arch/parisc/include/asm/syscall.h | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/arch/parisc/include/asm/syscall.h b/arch/parisc/include/asm/syscall.h
index 80757e43cf2c..00b127a5e09b 100644
--- a/arch/parisc/include/asm/syscall.h
+++ b/arch/parisc/include/asm/syscall.h
@@ -29,6 +29,13 @@ static inline void syscall_get_arguments(struct task_struct *tsk,
args[0] = regs->gr[26];
}

+static inline long syscall_get_error(struct task_struct *task,
+ struct pt_regs *regs)
+{
+ unsigned long error = regs->gr[28];
+ return IS_ERR_VALUE(error) ? error : 0;
+}
+
static inline long syscall_get_return_value(struct task_struct *task,
struct pt_regs *regs)
{
--
ldv

2019-04-08 18:45:54

by Dmitry V. Levin

[permalink] [raw]
Subject: [PATCH linux-next v9 7/7] selftests/ptrace: add a test case for PTRACE_GET_SYSCALL_INFO

Check whether PTRACE_GET_SYSCALL_INFO semantics implemented in the kernel
matches userspace expectations.

Cc: Oleg Nesterov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Shuah Khan <[email protected]>
Cc: Elvira Khabirova <[email protected]>
Cc: Eugene Syromyatnikov <[email protected]>
Cc: [email protected]
Signed-off-by: Dmitry V. Levin <[email protected]>
---

Notes:
v9: unchanged
v8: unchanged
v7: unchanged
v6: made PTRACE_GET_SYSCALL_INFO return value checks strict
v5: initial revision

tools/testing/selftests/ptrace/.gitignore | 1 +
tools/testing/selftests/ptrace/Makefile | 2 +-
.../selftests/ptrace/get_syscall_info.c | 271 ++++++++++++++++++
3 files changed, 273 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/ptrace/get_syscall_info.c

diff --git a/tools/testing/selftests/ptrace/.gitignore b/tools/testing/selftests/ptrace/.gitignore
index b3e59d41fd82..cfcc49a7def7 100644
--- a/tools/testing/selftests/ptrace/.gitignore
+++ b/tools/testing/selftests/ptrace/.gitignore
@@ -1 +1,2 @@
+get_syscall_info
peeksiginfo
diff --git a/tools/testing/selftests/ptrace/Makefile b/tools/testing/selftests/ptrace/Makefile
index 8a2bc5562179..4bc550b6b845 100644
--- a/tools/testing/selftests/ptrace/Makefile
+++ b/tools/testing/selftests/ptrace/Makefile
@@ -1,5 +1,5 @@
CFLAGS += -iquote../../../../include/uapi -Wall

-TEST_GEN_PROGS := peeksiginfo
+TEST_GEN_PROGS := get_syscall_info peeksiginfo

include ../lib.mk
diff --git a/tools/testing/selftests/ptrace/get_syscall_info.c b/tools/testing/selftests/ptrace/get_syscall_info.c
new file mode 100644
index 000000000000..28e972825b74
--- /dev/null
+++ b/tools/testing/selftests/ptrace/get_syscall_info.c
@@ -0,0 +1,271 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * Copyright (c) 2018 Dmitry V. Levin <[email protected]>
+ * All rights reserved.
+ *
+ * Check whether PTRACE_GET_SYSCALL_INFO semantics implemented in the kernel
+ * matches userspace expectations.
+ */
+
+#include "../kselftest_harness.h"
+#include <err.h>
+#include <signal.h>
+#include <asm/unistd.h>
+#include "linux/ptrace.h"
+
+static int
+kill_tracee(pid_t pid)
+{
+ if (!pid)
+ return 0;
+
+ int saved_errno = errno;
+
+ int rc = kill(pid, SIGKILL);
+
+ errno = saved_errno;
+ return rc;
+}
+
+static long
+sys_ptrace(int request, pid_t pid, unsigned long addr, unsigned long data)
+{
+ return syscall(__NR_ptrace, request, pid, addr, data);
+}
+
+#define LOG_KILL_TRACEE(fmt, ...) \
+ do { \
+ kill_tracee(pid); \
+ TH_LOG("wait #%d: " fmt, \
+ ptrace_stop, ##__VA_ARGS__); \
+ } while (0)
+
+TEST(get_syscall_info)
+{
+ static const unsigned long args[][7] = {
+ /* a sequence of architecture-agnostic syscalls */
+ {
+ __NR_chdir,
+ (unsigned long) "",
+ 0xbad1fed1,
+ 0xbad2fed2,
+ 0xbad3fed3,
+ 0xbad4fed4,
+ 0xbad5fed5
+ },
+ {
+ __NR_gettid,
+ 0xcaf0bea0,
+ 0xcaf1bea1,
+ 0xcaf2bea2,
+ 0xcaf3bea3,
+ 0xcaf4bea4,
+ 0xcaf5bea5
+ },
+ {
+ __NR_exit_group,
+ 0,
+ 0xfac1c0d1,
+ 0xfac2c0d2,
+ 0xfac3c0d3,
+ 0xfac4c0d4,
+ 0xfac5c0d5
+ }
+ };
+ const unsigned long *exp_args;
+
+ pid_t pid = fork();
+
+ ASSERT_LE(0, pid) {
+ TH_LOG("fork: %m");
+ }
+
+ if (pid == 0) {
+ /* get the pid before PTRACE_TRACEME */
+ pid = getpid();
+ ASSERT_EQ(0, sys_ptrace(PTRACE_TRACEME, 0, 0, 0)) {
+ TH_LOG("PTRACE_TRACEME: %m");
+ }
+ ASSERT_EQ(0, kill(pid, SIGSTOP)) {
+ /* cannot happen */
+ TH_LOG("kill SIGSTOP: %m");
+ }
+ for (unsigned int i = 0; i < ARRAY_SIZE(args); ++i) {
+ syscall(args[i][0],
+ args[i][1], args[i][2], args[i][3],
+ args[i][4], args[i][5], args[i][6]);
+ }
+ /* unreachable */
+ _exit(1);
+ }
+
+ const struct {
+ unsigned int is_error;
+ int rval;
+ } *exp_param, exit_param[] = {
+ { 1, -ENOENT }, /* chdir */
+ { 0, pid } /* gettid */
+ };
+
+ unsigned int ptrace_stop;
+
+ for (ptrace_stop = 0; ; ++ptrace_stop) {
+ struct ptrace_syscall_info info = {
+ .op = 0xff /* invalid PTRACE_SYSCALL_INFO_* op */
+ };
+ const size_t size = sizeof(info);
+ const int expected_none_size =
+ (void *) &info.entry - (void *) &info;
+ const int expected_entry_size =
+ (void *) &info.entry.args[6] - (void *) &info;
+ const int expected_exit_size =
+ (void *) (&info.exit.is_error + 1) -
+ (void *) &info;
+ int status;
+ long rc;
+
+ ASSERT_EQ(pid, wait(&status)) {
+ /* cannot happen */
+ LOG_KILL_TRACEE("wait: %m");
+ }
+ if (WIFEXITED(status)) {
+ pid = 0; /* the tracee is no more */
+ ASSERT_EQ(0, WEXITSTATUS(status));
+ break;
+ }
+ ASSERT_FALSE(WIFSIGNALED(status)) {
+ pid = 0; /* the tracee is no more */
+ LOG_KILL_TRACEE("unexpected signal %u",
+ WTERMSIG(status));
+ }
+ ASSERT_TRUE(WIFSTOPPED(status)) {
+ /* cannot happen */
+ LOG_KILL_TRACEE("unexpected wait status %#x", status);
+ }
+
+ switch (WSTOPSIG(status)) {
+ case SIGSTOP:
+ ASSERT_EQ(0, ptrace_stop) {
+ LOG_KILL_TRACEE("unexpected signal stop");
+ }
+ ASSERT_EQ(0, sys_ptrace(PTRACE_SETOPTIONS, pid, 0,
+ PTRACE_O_TRACESYSGOOD)) {
+ LOG_KILL_TRACEE("PTRACE_SETOPTIONS: %m");
+ }
+ ASSERT_LT(0, (rc = sys_ptrace(PTRACE_GET_SYSCALL_INFO,
+ pid, size,
+ (unsigned long) &info))) {
+ LOG_KILL_TRACEE("PTRACE_GET_SYSCALL_INFO: %m");
+ }
+ ASSERT_EQ(expected_none_size, rc) {
+ LOG_KILL_TRACEE("signal stop mismatch");
+ }
+ ASSERT_EQ(PTRACE_SYSCALL_INFO_NONE, info.op) {
+ LOG_KILL_TRACEE("signal stop mismatch");
+ }
+ ASSERT_TRUE(info.arch) {
+ LOG_KILL_TRACEE("signal stop mismatch");
+ }
+ ASSERT_TRUE(info.instruction_pointer) {
+ LOG_KILL_TRACEE("signal stop mismatch");
+ }
+ ASSERT_TRUE(info.stack_pointer) {
+ LOG_KILL_TRACEE("signal stop mismatch");
+ }
+ break;
+
+ case SIGTRAP | 0x80:
+ ASSERT_LT(0, (rc = sys_ptrace(PTRACE_GET_SYSCALL_INFO,
+ pid, size,
+ (unsigned long) &info))) {
+ LOG_KILL_TRACEE("PTRACE_GET_SYSCALL_INFO: %m");
+ }
+ switch (ptrace_stop) {
+ case 1: /* entering chdir */
+ case 3: /* entering gettid */
+ case 5: /* entering exit_group */
+ exp_args = args[ptrace_stop / 2];
+ ASSERT_EQ(expected_entry_size, rc) {
+ LOG_KILL_TRACEE("entry stop mismatch");
+ }
+ ASSERT_EQ(PTRACE_SYSCALL_INFO_ENTRY, info.op) {
+ LOG_KILL_TRACEE("entry stop mismatch");
+ }
+ ASSERT_TRUE(info.arch) {
+ LOG_KILL_TRACEE("entry stop mismatch");
+ }
+ ASSERT_TRUE(info.instruction_pointer) {
+ LOG_KILL_TRACEE("entry stop mismatch");
+ }
+ ASSERT_TRUE(info.stack_pointer) {
+ LOG_KILL_TRACEE("entry stop mismatch");
+ }
+ ASSERT_EQ(exp_args[0], info.entry.nr) {
+ LOG_KILL_TRACEE("entry stop mismatch");
+ }
+ ASSERT_EQ(exp_args[1], info.entry.args[0]) {
+ LOG_KILL_TRACEE("entry stop mismatch");
+ }
+ ASSERT_EQ(exp_args[2], info.entry.args[1]) {
+ LOG_KILL_TRACEE("entry stop mismatch");
+ }
+ ASSERT_EQ(exp_args[3], info.entry.args[2]) {
+ LOG_KILL_TRACEE("entry stop mismatch");
+ }
+ ASSERT_EQ(exp_args[4], info.entry.args[3]) {
+ LOG_KILL_TRACEE("entry stop mismatch");
+ }
+ ASSERT_EQ(exp_args[5], info.entry.args[4]) {
+ LOG_KILL_TRACEE("entry stop mismatch");
+ }
+ ASSERT_EQ(exp_args[6], info.entry.args[5]) {
+ LOG_KILL_TRACEE("entry stop mismatch");
+ }
+ break;
+ case 2: /* exiting chdir */
+ case 4: /* exiting gettid */
+ exp_param = &exit_param[ptrace_stop / 2 - 1];
+ ASSERT_EQ(expected_exit_size, rc) {
+ LOG_KILL_TRACEE("exit stop mismatch");
+ }
+ ASSERT_EQ(PTRACE_SYSCALL_INFO_EXIT, info.op) {
+ LOG_KILL_TRACEE("exit stop mismatch");
+ }
+ ASSERT_TRUE(info.arch) {
+ LOG_KILL_TRACEE("exit stop mismatch");
+ }
+ ASSERT_TRUE(info.instruction_pointer) {
+ LOG_KILL_TRACEE("exit stop mismatch");
+ }
+ ASSERT_TRUE(info.stack_pointer) {
+ LOG_KILL_TRACEE("exit stop mismatch");
+ }
+ ASSERT_EQ(exp_param->is_error,
+ info.exit.is_error) {
+ LOG_KILL_TRACEE("exit stop mismatch");
+ }
+ ASSERT_EQ(exp_param->rval, info.exit.rval) {
+ LOG_KILL_TRACEE("exit stop mismatch");
+ }
+ break;
+ default:
+ LOG_KILL_TRACEE("unexpected syscall stop");
+ abort();
+ }
+ break;
+
+ default:
+ LOG_KILL_TRACEE("unexpected stop signal %#x",
+ WSTOPSIG(status));
+ abort();
+ }
+
+ ASSERT_EQ(0, sys_ptrace(PTRACE_SYSCALL, pid, 0, 0)) {
+ LOG_KILL_TRACEE("PTRACE_SYSCALL: %m");
+ }
+ }
+
+ ASSERT_EQ(ARRAY_SIZE(args) * 2, ptrace_stop);
+}
+
+TEST_HARNESS_MAIN
--
ldv

2019-04-08 19:36:34

by Dmitry V. Levin

[permalink] [raw]
Subject: [PATCH linux-next v9 6/7] ptrace: add PTRACE_GET_SYSCALL_INFO request

From: Elvira Khabirova <[email protected]>

PTRACE_GET_SYSCALL_INFO is a generic ptrace API that lets ptracer obtain
details of the syscall the tracee is blocked in.

There are two reasons for a special syscall-related ptrace request.

Firstly, with the current ptrace API there are cases when ptracer cannot
retrieve necessary information about syscalls. Some examples include:
* The notorious int-0x80-from-64-bit-task issue. See [1] for details.
In short, if a 64-bit task performs a syscall through int 0x80, its tracer
has no reliable means to find out that the syscall was, in fact,
a compat syscall, and misidentifies it.
* Syscall-enter-stop and syscall-exit-stop look the same for the tracer.
Common practice is to keep track of the sequence of ptrace-stops in order
not to mix the two syscall-stops up. But it is not as simple as it looks;
for example, strace had a (just recently fixed) long-standing bug where
attaching strace to a tracee that is performing the execve system call
led to the tracer identifying the following syscall-exit-stop as
syscall-enter-stop, which messed up all the state tracking.
* Since the introduction of commit 84d77d3f06e7e8dea057d10e8ec77ad71f721be3
("ptrace: Don't allow accessing an undumpable mm"), both PTRACE_PEEKDATA
and process_vm_readv become unavailable when the process dumpable flag
is cleared. On such architectures as ia64 this results in all syscall
arguments being unavailable for the tracer.

Secondly, ptracers also have to support a lot of arch-specific code for
obtaining information about the tracee. For some architectures, this
requires a ptrace(PTRACE_PEEKUSER, ...) invocation for every syscall
argument and return value.

ptrace(2) man page:

long ptrace(enum __ptrace_request request, pid_t pid,
void *addr, void *data);
...
PTRACE_GET_SYSCALL_INFO
Retrieve information about the syscall that caused the stop.
The information is placed into the buffer pointed by "data"
argument, which should be a pointer to a buffer of type
"struct ptrace_syscall_info".
The "addr" argument contains the size of the buffer pointed to
by "data" argument (i.e., sizeof(struct ptrace_syscall_info)).
The return value contains the number of bytes available
to be written by the kernel.
If the size of data to be written by the kernel exceeds the size
specified by "addr" argument, the output is truncated.

Co-authored-by: Dmitry V. Levin <[email protected]>
Reviewed-by: Oleg Nesterov <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Eugene Syromyatnikov <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Elvira Khabirova <[email protected]>
Signed-off-by: Dmitry V. Levin <[email protected]>
---

Notes:
v9:
* Rebased to linux-next again due to syscall_get_arguments() signature change.

v8:
* Rebased to linux-next.
* Moved ptrace_get_syscall_info code under #ifdef CONFIG_HAVE_ARCH_TRACEHOOK,
narrowing down the set of architectures supported by this implementation
back to those 19 that enable CONFIG_HAVE_ARCH_TRACEHOOK because
I failed to get all syscall_get_*(), instruction_pointer(),
and user_stack_pointer() functions implemented on some niche
architectures. This leaves the following architectures out:
alpha, h8300, m68k, microblaze, and unicore32.

v7: unchanged

v6:
* Change PTRACE_GET_SYSCALL_INFO return code: do not take trailing paddings
into account, use the end of the last field of the structure being written.
* Change struct ptrace_syscall_info:
* remove .frame_pointer field, is is not needed and not portable;
* make .arch field explicitly aligned, remove no longer needed
padding before .arch field;
* remove trailing pads, they are no longer needed.

v5:
* Change PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} values as requested by Oleg.
* Change struct ptrace_syscall_info: generalize instruction_pointer,
stack_pointer, and frame_pointer fields by moving them from
ptrace_syscall_info.{entry,seccomp} substructures to ptrace_syscall_info
and initializing them for all stops.
* Add PTRACE_SYSCALL_INFO_NONE, set it when not in a syscall stop,
so e.g. "strace -i" could use PTRACE_SYSCALL_INFO_SECCOMP to obtain
instruction_pointer when the tracee is in a signal stop.
* Make available for all architectures: do not conditionalize on
CONFIG_HAVE_ARCH_TRACEHOOK since all syscall_get_* functions
are implemented on all architectures.

v4:
* Do not introduce task_struct.ptrace_event,
use child->last_siginfo->si_code instead.
* Implement PTRACE_SYSCALL_INFO_SECCOMP and ptrace_syscall_info.seccomp
support along with PTRACE_SYSCALL_INFO_{ENTRY,EXIT} and
ptrace_syscall_info.{entry,exit}.

v3:
* Change struct ptrace_syscall_info.
* Support PTRACE_EVENT_SECCOMP by adding ptrace_event to task_struct.
* Add proper defines for ptrace_syscall_info.op values.
* Rename PT_SYSCALL_IS_ENTERING and PT_SYSCALL_IS_EXITING to
PTRACE_EVENTMSG_SYSCALL_ENTRY and PTRACE_EVENTMSG_SYSCALL_EXIT
* and move them to uapi.

v2:
* Do not use task->ptrace.
* Replace entry_info.is_compat with entry_info.arch, use syscall_get_arch().
* Use addr argument of sys_ptrace to get expected size of the struct;
return full size of the struct.

include/linux/tracehook.h | 9 ++--
include/uapi/linux/ptrace.h | 35 ++++++++++++
kernel/ptrace.c | 103 +++++++++++++++++++++++++++++++++++-
3 files changed, 143 insertions(+), 4 deletions(-)

diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
index df20f8bdbfa3..6bc7a3d58e2f 100644
--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -57,13 +57,15 @@ struct linux_binprm;
/*
* ptrace report for syscall entry and exit looks identical.
*/
-static inline int ptrace_report_syscall(struct pt_regs *regs)
+static inline int ptrace_report_syscall(struct pt_regs *regs,
+ unsigned long message)
{
int ptrace = current->ptrace;

if (!(ptrace & PT_PTRACED))
return 0;

+ current->ptrace_message = message;
ptrace_notify(SIGTRAP | ((ptrace & PT_TRACESYSGOOD) ? 0x80 : 0));

/*
@@ -76,6 +78,7 @@ static inline int ptrace_report_syscall(struct pt_regs *regs)
current->exit_code = 0;
}

+ current->ptrace_message = 0;
return fatal_signal_pending(current);
}

@@ -101,7 +104,7 @@ static inline int ptrace_report_syscall(struct pt_regs *regs)
static inline __must_check int tracehook_report_syscall_entry(
struct pt_regs *regs)
{
- return ptrace_report_syscall(regs);
+ return ptrace_report_syscall(regs, PTRACE_EVENTMSG_SYSCALL_ENTRY);
}

/**
@@ -126,7 +129,7 @@ static inline void tracehook_report_syscall_exit(struct pt_regs *regs, int step)
if (step)
user_single_step_report(regs);
else
- ptrace_report_syscall(regs);
+ ptrace_report_syscall(regs, PTRACE_EVENTMSG_SYSCALL_EXIT);
}

/**
diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index d5a1b8a492b9..a71b6e3b03eb 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -73,6 +73,41 @@ struct seccomp_metadata {
__u64 flags; /* Output: filter's flags */
};

+#define PTRACE_GET_SYSCALL_INFO 0x420e
+#define PTRACE_SYSCALL_INFO_NONE 0
+#define PTRACE_SYSCALL_INFO_ENTRY 1
+#define PTRACE_SYSCALL_INFO_EXIT 2
+#define PTRACE_SYSCALL_INFO_SECCOMP 3
+
+struct ptrace_syscall_info {
+ __u8 op; /* PTRACE_SYSCALL_INFO_* */
+ __u32 arch __attribute__((__aligned__(sizeof(__u32))));
+ __u64 instruction_pointer;
+ __u64 stack_pointer;
+ union {
+ struct {
+ __u64 nr;
+ __u64 args[6];
+ } entry;
+ struct {
+ __s64 rval;
+ __u8 is_error;
+ } exit;
+ struct {
+ __u64 nr;
+ __u64 args[6];
+ __u32 ret_data;
+ } seccomp;
+ };
+};
+
+/*
+ * These values are stored in task->ptrace_message
+ * by tracehook_report_syscall_* to describe the current syscall-stop.
+ */
+#define PTRACE_EVENTMSG_SYSCALL_ENTRY 1
+#define PTRACE_EVENTMSG_SYSCALL_EXIT 2
+
/* Read signals from a shared (process wide) queue */
#define PTRACE_PEEKSIGINFO_SHARED (1 << 0)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 6f357f4fc859..f68c0d44461e 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -31,6 +31,10 @@
#include <linux/compat.h>
#include <linux/sched/signal.h>

+#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
+#include <asm/syscall.h> /* For syscall_get_* */
+#endif
+
/*
* Access another process' address space via ptrace.
* Source/target buffer must be kernel space,
@@ -879,7 +883,100 @@ static int ptrace_regset(struct task_struct *task, int req, unsigned int type,
* to ensure no machine forgets it.
*/
EXPORT_SYMBOL_GPL(task_user_regset_view);
-#endif
+
+static unsigned long
+ptrace_get_syscall_info_entry(struct task_struct *child, struct pt_regs *regs,
+ struct ptrace_syscall_info *info)
+{
+ unsigned long args[ARRAY_SIZE(info->entry.args)];
+ int i;
+
+ info->op = PTRACE_SYSCALL_INFO_ENTRY;
+ info->entry.nr = syscall_get_nr(child, regs);
+ syscall_get_arguments(child, regs, args);
+ for (i = 0; i < ARRAY_SIZE(args); i++)
+ info->entry.args[i] = args[i];
+
+ /* args is the last field in struct ptrace_syscall_info.entry */
+ return offsetofend(struct ptrace_syscall_info, entry.args);
+}
+
+static unsigned long
+ptrace_get_syscall_info_seccomp(struct task_struct *child, struct pt_regs *regs,
+ struct ptrace_syscall_info *info)
+{
+ /*
+ * As struct ptrace_syscall_info.entry is currently a subset
+ * of struct ptrace_syscall_info.seccomp, it makes sense to
+ * initialize that subset using ptrace_get_syscall_info_entry().
+ * This can be reconsidered in the future if these structures
+ * diverge significantly enough.
+ */
+ ptrace_get_syscall_info_entry(child, regs, info);
+ info->op = PTRACE_SYSCALL_INFO_SECCOMP;
+ info->seccomp.ret_data = child->ptrace_message;
+
+ /* ret_data is the last field in struct ptrace_syscall_info.seccomp */
+ return offsetofend(struct ptrace_syscall_info, seccomp.ret_data);
+}
+
+static unsigned long
+ptrace_get_syscall_info_exit(struct task_struct *child, struct pt_regs *regs,
+ struct ptrace_syscall_info *info)
+{
+ info->op = PTRACE_SYSCALL_INFO_EXIT;
+ info->exit.rval = syscall_get_error(child, regs);
+ info->exit.is_error = !!info->exit.rval;
+ if (!info->exit.is_error)
+ info->exit.rval = syscall_get_return_value(child, regs);
+
+ /* is_error is the last field in struct ptrace_syscall_info.exit */
+ return offsetofend(struct ptrace_syscall_info, exit.is_error);
+}
+
+static int
+ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
+ void __user *datavp)
+{
+ struct pt_regs *regs = task_pt_regs(child);
+ struct ptrace_syscall_info info = {
+ .op = PTRACE_SYSCALL_INFO_NONE,
+ .arch = syscall_get_arch(child),
+ .instruction_pointer = instruction_pointer(regs),
+ .stack_pointer = user_stack_pointer(regs),
+ };
+ unsigned long actual_size = offsetof(struct ptrace_syscall_info, entry);
+ unsigned long write_size;
+
+ /*
+ * This does not need lock_task_sighand() to access
+ * child->last_siginfo because ptrace_freeze_traced()
+ * called earlier by ptrace_check_attach() ensures that
+ * the tracee cannot go away and clear its last_siginfo.
+ */
+ switch (child->last_siginfo ? child->last_siginfo->si_code : 0) {
+ case SIGTRAP | 0x80:
+ switch (child->ptrace_message) {
+ case PTRACE_EVENTMSG_SYSCALL_ENTRY:
+ actual_size = ptrace_get_syscall_info_entry(child, regs,
+ &info);
+ break;
+ case PTRACE_EVENTMSG_SYSCALL_EXIT:
+ actual_size = ptrace_get_syscall_info_exit(child, regs,
+ &info);
+ break;
+ }
+ break;
+ case SIGTRAP | (PTRACE_EVENT_SECCOMP << 8):
+ actual_size = ptrace_get_syscall_info_seccomp(child, regs,
+ &info);
+ break;
+ }
+
+ write_size = min(actual_size, user_size);
+ return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
+}
+#endif /* CONFIG_HAVE_ARCH_TRACEHOOK */

int ptrace_request(struct task_struct *child, long request,
unsigned long addr, unsigned long data)
@@ -1096,6 +1193,10 @@ int ptrace_request(struct task_struct *child, long request,
ret = __put_user(kiov.iov_len, &uiov->iov_len);
break;
}
+
+ case PTRACE_GET_SYSCALL_INFO:
+ ret = ptrace_get_syscall_info(child, addr, datavp);
+ break;
#endif

case PTRACE_SECCOMP_GET_FILTER:
--
ldv

2019-04-08 19:38:32

by Shuah Khan

[permalink] [raw]
Subject: Re: [PATCH linux-next v9 7/7] selftests/ptrace: add a test case for PTRACE_GET_SYSCALL_INFO

On 4/8/19 11:42 AM, Dmitry V. Levin wrote:
> Check whether PTRACE_GET_SYSCALL_INFO semantics implemented in the kernel
> matches userspace expectations.
>
> Cc: Oleg Nesterov <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Shuah Khan <[email protected]>
> Cc: Elvira Khabirova <[email protected]>
> Cc: Eugene Syromyatnikov <[email protected]>
> Cc: [email protected]
> Signed-off-by: Dmitry V. Levin <[email protected]>
> ---
>
> Notes:
> v9: unchanged
> v8: unchanged
> v7: unchanged
> v6: made PTRACE_GET_SYSCALL_INFO return value checks strict
> v5: initial revision
>
> tools/testing/selftests/ptrace/.gitignore | 1 +
> tools/testing/selftests/ptrace/Makefile | 2 +-
> .../selftests/ptrace/get_syscall_info.c | 271 ++++++++++++++++++
> 3 files changed, 273 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/ptrace/get_syscall_info.c
>
> diff --git a/tools/testing/selftests/ptrace/.gitignore b/tools/testing/selftests/ptrace/.gitignore
> index b3e59d41fd82..cfcc49a7def7 100644
> --- a/tools/testing/selftests/ptrace/.gitignore
> +++ b/tools/testing/selftests/ptrace/.gitignore
> @@ -1 +1,2 @@
> +get_syscall_info
> peeksiginfo
> diff --git a/tools/testing/selftests/ptrace/Makefile b/tools/testing/selftests/ptrace/Makefile
> index 8a2bc5562179..4bc550b6b845 100644
> --- a/tools/testing/selftests/ptrace/Makefile
> +++ b/tools/testing/selftests/ptrace/Makefile
> @@ -1,5 +1,5 @@
> CFLAGS += -iquote../../../../include/uapi -Wall
>
> -TEST_GEN_PROGS := peeksiginfo
> +TEST_GEN_PROGS := get_syscall_info peeksiginfo
>
> include ../lib.mk
> diff --git a/tools/testing/selftests/ptrace/get_syscall_info.c b/tools/testing/selftests/ptrace/get_syscall_info.c
> new file mode 100644
> index 000000000000..28e972825b74
> --- /dev/null
> +++ b/tools/testing/selftests/ptrace/get_syscall_info.c
> @@ -0,0 +1,271 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later
> + *

This should be just GPL-2.0+

The rest looks good to me. Assuming this patch has dependency on the
rest of the patches in this series and once the above change is made:

Acked-by: Shuah Khan <[email protected]>

thnaks,
-- Shuah

2019-04-08 20:19:32

by Dmitry V. Levin

[permalink] [raw]
Subject: [PATCH linux-next v9 1/7] nds32: fix asm/syscall.h

All syscall_get_*() and syscall_set_*() functions must be defined
as static inline as on all other architectures, otherwise asm/syscall.h
cannot be included in more than one compilation unit.

This bug has to be fixed in order to extend the generic
ptrace API with PTRACE_GET_SYSCALL_INFO request.

Reported-by: kbuild test robot <[email protected]>
Fixes: 1932fbe36e02 ("nds32: System calls handling")
Cc: Greentime Hu <[email protected]>
Cc: Vincent Chen <[email protected]>
Cc: Elvira Khabirova <[email protected]>
Cc: Eugene Syromyatnikov <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Signed-off-by: Dmitry V. Levin <[email protected]>
---

Dear nds32 maintainers, this fix is waiting for ACK since early January.

Notes:
v9: rebased to linux-next again due to syscall_get_arguments() signature change
v8: unchanged
v7: initial revision

arch/nds32/include/asm/syscall.h | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/arch/nds32/include/asm/syscall.h b/arch/nds32/include/asm/syscall.h
index 174b8571d362..d08439a54034 100644
--- a/arch/nds32/include/asm/syscall.h
+++ b/arch/nds32/include/asm/syscall.h
@@ -26,7 +26,8 @@ struct pt_regs;
*
* It's only valid to call this when @task is known to be blocked.
*/
-int syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
+static inline int
+syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
{
return regs->syscallno;
}
@@ -47,7 +48,8 @@ int syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
* system call instruction. This may not be the same as what the
* register state looked like at system call entry tracing.
*/
-void syscall_rollback(struct task_struct *task, struct pt_regs *regs)
+static inline void
+syscall_rollback(struct task_struct *task, struct pt_regs *regs)
{
regs->uregs[0] = regs->orig_r0;
}
@@ -62,7 +64,8 @@ void syscall_rollback(struct task_struct *task, struct pt_regs *regs)
* It's only valid to call this when @task is stopped for tracing on exit
* from a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
*/
-long syscall_get_error(struct task_struct *task, struct pt_regs *regs)
+static inline long
+syscall_get_error(struct task_struct *task, struct pt_regs *regs)
{
unsigned long error = regs->uregs[0];
return IS_ERR_VALUE(error) ? error : 0;
@@ -79,7 +82,8 @@ long syscall_get_error(struct task_struct *task, struct pt_regs *regs)
* It's only valid to call this when @task is stopped for tracing on exit
* from a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
*/
-long syscall_get_return_value(struct task_struct *task, struct pt_regs *regs)
+static inline long
+syscall_get_return_value(struct task_struct *task, struct pt_regs *regs)
{
return regs->uregs[0];
}
@@ -99,8 +103,9 @@ long syscall_get_return_value(struct task_struct *task, struct pt_regs *regs)
* It's only valid to call this when @task is stopped for tracing on exit
* from a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
*/
-void syscall_set_return_value(struct task_struct *task, struct pt_regs *regs,
- int error, long val)
+static inline void
+syscall_set_return_value(struct task_struct *task, struct pt_regs *regs,
+ int error, long val)
{
regs->uregs[0] = (long)error ? error : val;
}
@@ -118,8 +123,9 @@ void syscall_set_return_value(struct task_struct *task, struct pt_regs *regs,
* entry to a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
*/
#define SYSCALL_MAX_ARGS 6
-void syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
- unsigned long *args)
+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+ unsigned long *args)
{
args[0] = regs->orig_r0;
args++;
@@ -138,8 +144,9 @@ void syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
* It's only valid to call this when @task is stopped for tracing on
* entry to a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
*/
-void syscall_set_arguments(struct task_struct *task, struct pt_regs *regs,
- const unsigned long *args)
+static inline void
+syscall_set_arguments(struct task_struct *task, struct pt_regs *regs,
+ const unsigned long *args)
{
regs->orig_r0 = args[0];
args++;
--
ldv

2019-04-08 20:20:45

by Dmitry V. Levin

[permalink] [raw]
Subject: [PATCH linux-next v9 2/7] hexagon: define syscall_get_error() and syscall_get_return_value()

syscall_get_* functions are required to be implemented on all
architectures in order to extend the generic ptrace API with
PTRACE_GET_SYSCALL_INFO request.

This adds remaining 2 syscall_get_* functions as documented in
asm-generic/syscall.h: syscall_get_error and syscall_get_return_value.

Cc: Richard Kuo <[email protected]>
Cc: Elvira Khabirova <[email protected]>
Cc: Eugene Syromyatnikov <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: [email protected]
Signed-off-by: Dmitry V. Levin <[email protected]>
---

Richard, this patch is waiting for ACK since November.

Notes:
v9: unchanged
v8: moved syscall_get_arch to separate audit patchset
v7: unchanged
v6: added missing includes
v5: added syscall_get_error and syscall_get_return_value
v4: unchanged
v3: unchanged
v2: unchanged

arch/hexagon/include/asm/syscall.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)

diff --git a/arch/hexagon/include/asm/syscall.h b/arch/hexagon/include/asm/syscall.h
index dab26a71f577..0f28a6a39c3a 100644
--- a/arch/hexagon/include/asm/syscall.h
+++ b/arch/hexagon/include/asm/syscall.h
@@ -22,6 +22,8 @@
#define _ASM_HEXAGON_SYSCALL_H

#include <uapi/linux/audit.h>
+#include <linux/err.h>
+#include <asm/ptrace.h>

typedef long (*syscall_fn)(unsigned long, unsigned long,
unsigned long, unsigned long,
@@ -44,6 +46,18 @@ static inline void syscall_get_arguments(struct task_struct *task,
memcpy(args, &(&regs->r00)[0], 6 * sizeof(args[0]));
}

+static inline long syscall_get_error(struct task_struct *task,
+ struct pt_regs *regs)
+{
+ return IS_ERR_VALUE(regs->r00) ? regs->r00 : 0;
+}
+
+static inline long syscall_get_return_value(struct task_struct *task,
+ struct pt_regs *regs)
+{
+ return regs->r00;
+}
+
static inline int syscall_get_arch(struct task_struct *task)
{
return AUDIT_ARCH_HEXAGON;
--
ldv

2019-04-08 20:23:46

by Dmitry V. Levin

[permalink] [raw]
Subject: [PATCH linux-next v9 5/7] powerpc: define syscall_get_error()

syscall_get_error() is required to be implemented on this
architecture in addition to already implemented syscall_get_nr(),
syscall_get_arguments(), syscall_get_return_value(), and
syscall_get_arch() functions in order to extend the generic
ptrace API with PTRACE_GET_SYSCALL_INFO request.

Cc: Michael Ellerman <[email protected]>
Cc: Elvira Khabirova <[email protected]>
Cc: Eugene Syromyatnikov <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: [email protected]
Signed-off-by: Dmitry V. Levin <[email protected]>
---

Michael, this patch is waiting for ACK since early December.

Notes:
v9: unchanged
v8: unchanged
v7: unchanged
v6: unchanged
v5: initial revision

This change has been tested with
tools/testing/selftests/ptrace/get_syscall_info.c and strace,
so it's correct from PTRACE_GET_SYSCALL_INFO point of view.

This cast doubts on commit v4.3-rc1~86^2~81 that changed
syscall_set_return_value() in a way that doesn't quite match
syscall_get_error(), but syscall_set_return_value() is out
of scope of this series, so I'll just let you know my concerns.

arch/powerpc/include/asm/syscall.h | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
index a048fed0722f..bd9663137d57 100644
--- a/arch/powerpc/include/asm/syscall.h
+++ b/arch/powerpc/include/asm/syscall.h
@@ -38,6 +38,16 @@ static inline void syscall_rollback(struct task_struct *task,
regs->gpr[3] = regs->orig_gpr3;
}

+static inline long syscall_get_error(struct task_struct *task,
+ struct pt_regs *regs)
+{
+ /*
+ * If the system call failed,
+ * regs->gpr[3] contains a positive ERRORCODE.
+ */
+ return (regs->ccr & 0x10000000UL) ? -regs->gpr[3] : 0;
+}
+
static inline long syscall_get_return_value(struct task_struct *task,
struct pt_regs *regs)
{
--
ldv

2019-04-08 20:24:39

by Dmitry V. Levin

[permalink] [raw]
Subject: Re: [PATCH linux-next v9 7/7] selftests/ptrace: add a test case for PTRACE_GET_SYSCALL_INFO

On Mon, Apr 08, 2019 at 11:51:45AM -0600, shuah wrote:
> On 4/8/19 11:42 AM, Dmitry V. Levin wrote:
> > Check whether PTRACE_GET_SYSCALL_INFO semantics implemented in the kernel
> > matches userspace expectations.
> >
> > Cc: Oleg Nesterov <[email protected]>
> > Cc: Andy Lutomirski <[email protected]>
> > Cc: Shuah Khan <[email protected]>
> > Cc: Elvira Khabirova <[email protected]>
> > Cc: Eugene Syromyatnikov <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Dmitry V. Levin <[email protected]>
> > ---
> >
> > Notes:
> > v9: unchanged
> > v8: unchanged
> > v7: unchanged
> > v6: made PTRACE_GET_SYSCALL_INFO return value checks strict
> > v5: initial revision
> >
> > tools/testing/selftests/ptrace/.gitignore | 1 +
> > tools/testing/selftests/ptrace/Makefile | 2 +-
> > .../selftests/ptrace/get_syscall_info.c | 271 ++++++++++++++++++
> > 3 files changed, 273 insertions(+), 1 deletion(-)
> > create mode 100644 tools/testing/selftests/ptrace/get_syscall_info.c
> >
> > diff --git a/tools/testing/selftests/ptrace/.gitignore b/tools/testing/selftests/ptrace/.gitignore
> > index b3e59d41fd82..cfcc49a7def7 100644
> > --- a/tools/testing/selftests/ptrace/.gitignore
> > +++ b/tools/testing/selftests/ptrace/.gitignore
> > @@ -1 +1,2 @@
> > +get_syscall_info
> > peeksiginfo
> > diff --git a/tools/testing/selftests/ptrace/Makefile b/tools/testing/selftests/ptrace/Makefile
> > index 8a2bc5562179..4bc550b6b845 100644
> > --- a/tools/testing/selftests/ptrace/Makefile
> > +++ b/tools/testing/selftests/ptrace/Makefile
> > @@ -1,5 +1,5 @@
> > CFLAGS += -iquote../../../../include/uapi -Wall
> >
> > -TEST_GEN_PROGS := peeksiginfo
> > +TEST_GEN_PROGS := get_syscall_info peeksiginfo
> >
> > include ../lib.mk
> > diff --git a/tools/testing/selftests/ptrace/get_syscall_info.c b/tools/testing/selftests/ptrace/get_syscall_info.c
> > new file mode 100644
> > index 000000000000..28e972825b74
> > --- /dev/null
> > +++ b/tools/testing/selftests/ptrace/get_syscall_info.c
> > @@ -0,0 +1,271 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later
> > + *
>
> This should be just GPL-2.0+

LICENSES/preferred/GPL-2.0 says these variants are equivalent:
"
[...]
Valid-License-Identifier: GPL-2.0+
Valid-License-Identifier: GPL-2.0-or-later
[...]
For 'GNU General Public License (GPL) version 2 or any later version' use:
SPDX-License-Identifier: GPL-2.0+
or
SPDX-License-Identifier: GPL-2.0-or-later
"

The usage statistics shows that GPL-2.0+ is more popular in the kernel
tree than GPL-2.0-or-later, though.

> The rest looks good to me. Assuming this patch has dependency on the
> rest of the patches in this series and once the above change is made:

No problem, I'm fine with either variant of the license identifier.

> Acked-by: Shuah Khan <[email protected]>

Thanks,

--
ldv


Attachments:
(No filename) (2.91 kB)
signature.asc (817.00 B)
Download all attachments

2019-04-09 08:00:39

by Greentime Hu

[permalink] [raw]
Subject: Re: [PATCH linux-next v9 1/7] nds32: fix asm/syscall.h

Dmitry V. Levin <[email protected]> 於 2019年4月9日 週二 上午1:41寫道:
>
> All syscall_get_*() and syscall_set_*() functions must be defined
> as static inline as on all other architectures, otherwise asm/syscall.h
> cannot be included in more than one compilation unit.
>
> This bug has to be fixed in order to extend the generic
> ptrace API with PTRACE_GET_SYSCALL_INFO request.
>
> Reported-by: kbuild test robot <[email protected]>
> Fixes: 1932fbe36e02 ("nds32: System calls handling")
> Cc: Greentime Hu <[email protected]>
> Cc: Vincent Chen <[email protected]>
> Cc: Elvira Khabirova <[email protected]>
> Cc: Eugene Syromyatnikov <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Cc: Andy Lutomirski <[email protected]>
> Signed-off-by: Dmitry V. Levin <[email protected]>
> ---
>
> Dear nds32 maintainers, this fix is waiting for ACK since early January.
>
> Notes:
> v9: rebased to linux-next again due to syscall_get_arguments() signature change
> v8: unchanged
> v7: initial revision
>
> arch/nds32/include/asm/syscall.h | 27 +++++++++++++++++----------
> 1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/arch/nds32/include/asm/syscall.h b/arch/nds32/include/asm/syscall.h
> index 174b8571d362..d08439a54034 100644
> --- a/arch/nds32/include/asm/syscall.h
> +++ b/arch/nds32/include/asm/syscall.h
> @@ -26,7 +26,8 @@ struct pt_regs;
> *
> * It's only valid to call this when @task is known to be blocked.
> */
> -int syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
> +static inline int
> +syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
> {
> return regs->syscallno;
> }
> @@ -47,7 +48,8 @@ int syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
> * system call instruction. This may not be the same as what the
> * register state looked like at system call entry tracing.
> */
> -void syscall_rollback(struct task_struct *task, struct pt_regs *regs)
> +static inline void
> +syscall_rollback(struct task_struct *task, struct pt_regs *regs)
> {
> regs->uregs[0] = regs->orig_r0;
> }
> @@ -62,7 +64,8 @@ void syscall_rollback(struct task_struct *task, struct pt_regs *regs)
> * It's only valid to call this when @task is stopped for tracing on exit
> * from a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
> */
> -long syscall_get_error(struct task_struct *task, struct pt_regs *regs)
> +static inline long
> +syscall_get_error(struct task_struct *task, struct pt_regs *regs)
> {
> unsigned long error = regs->uregs[0];
> return IS_ERR_VALUE(error) ? error : 0;
> @@ -79,7 +82,8 @@ long syscall_get_error(struct task_struct *task, struct pt_regs *regs)
> * It's only valid to call this when @task is stopped for tracing on exit
> * from a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
> */
> -long syscall_get_return_value(struct task_struct *task, struct pt_regs *regs)
> +static inline long
> +syscall_get_return_value(struct task_struct *task, struct pt_regs *regs)
> {
> return regs->uregs[0];
> }
> @@ -99,8 +103,9 @@ long syscall_get_return_value(struct task_struct *task, struct pt_regs *regs)
> * It's only valid to call this when @task is stopped for tracing on exit
> * from a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
> */
> -void syscall_set_return_value(struct task_struct *task, struct pt_regs *regs,
> - int error, long val)
> +static inline void
> +syscall_set_return_value(struct task_struct *task, struct pt_regs *regs,
> + int error, long val)
> {
> regs->uregs[0] = (long)error ? error : val;
> }
> @@ -118,8 +123,9 @@ void syscall_set_return_value(struct task_struct *task, struct pt_regs *regs,
> * entry to a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
> */
> #define SYSCALL_MAX_ARGS 6
> -void syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
> - unsigned long *args)
> +static inline void
> +syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
> + unsigned long *args)
> {
> args[0] = regs->orig_r0;
> args++;
> @@ -138,8 +144,9 @@ void syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
> * It's only valid to call this when @task is stopped for tracing on
> * entry to a system call, due to %TIF_SYSCALL_TRACE or %TIF_SYSCALL_AUDIT.
> */
> -void syscall_set_arguments(struct task_struct *task, struct pt_regs *regs,
> - const unsigned long *args)
> +static inline void
> +syscall_set_arguments(struct task_struct *task, struct pt_regs *regs,
> + const unsigned long *args)
> {
> regs->orig_r0 = args[0];
> args++;

Sorry for late.
Acked-by: Greentime Hu <[email protected]>