linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/2] ptrace: add PTRACE_GET_SYSCALL_INFO request
@ 2018-11-28 13:04 Dmitry V. Levin
  2018-11-28 13:06 ` [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message Dmitry V. Levin
  2018-11-28 13:07 ` [PATCH v4 2/2] ptrace: add PTRACE_GET_SYSCALL_INFO request Dmitry V. Levin
  0 siblings, 2 replies; 17+ messages in thread
From: Dmitry V. Levin @ 2018-11-28 13:04 UTC (permalink / raw)
  To: Oleg Nesterov, Andy Lutomirski
  Cc: Elvira Khabirova, Eugene Syromyatnikov, Steven Rostedt,
	Ingo Molnar, Kees Cook, Michael Ellerman, linux-api,
	linux-kernel, strace-devel

PTRACE_GET_SYSCALL_INFO lets ptracer obtain details of the syscall
the tracee is blocked in.  The request succeeds when the tracee is in a
syscall-enter-stop, syscall-exit-stop or PTRACE_EVENT_SECCOMP stop,
and fails with -EINVAL otherwise.

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.

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_* */
	__u8 __pad0[3];
	__u32 arch;
	union {
		struct {
			__u64 nr;
			__u64 instruction_pointer;
			__u64 stack_pointer;
			__u64 frame_pointer;
			__u64 args[6];
		} entry;
		struct {
			__s64 rval;
			__u8 is_error;
			__u8 __pad1[7];
		} exit;
		struct {
			__u64 nr;
			__u64 instruction_pointer;
			__u64 stack_pointer;
			__u64 frame_pointer;
			__u64 args[6];
			__u32 ret_data;
			__u8 __pad2[4];
		} seccomp;
	};
};

The structure was chosen according to [2], except for the following
changes:
* arch is returned unconditionally to aid with tracing system calls such as
execve();
* the type of nr field was changed from int to __u64 because syscall
numbers are, as a practical matter, 64 bits;
* stack_pointer and frame_pointer fields were added along with
instruction_pointer field since they are readily available and can save
the tracer from extra PTRACE_GETREGSET calls;
* 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.

This changeset should be applied on top of [3] and [4].

[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/
[3] https://lore.kernel.org/lkml/20181119210139.GA8360@altlinux.org/
[4] https://lore.kernel.org/lkml/20181120001128.GA11300@altlinux.org/

v4:
* Re-split into two commits.
* 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:
* Split into three commits.
* 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.

Elvira Khabirova (2):
  ptrace: save the type of syscall-stop in ptrace_message
  ptrace: add PTRACE_GET_SYSCALL_INFO request

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

-- 
ldv

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
  2018-11-28 13:04 [PATCH v4 0/2] ptrace: add PTRACE_GET_SYSCALL_INFO request Dmitry V. Levin
@ 2018-11-28 13:06 ` Dmitry V. Levin
  2018-11-28 13:49   ` Oleg Nesterov
  2018-11-28 13:07 ` [PATCH v4 2/2] ptrace: add PTRACE_GET_SYSCALL_INFO request Dmitry V. Levin
  1 sibling, 1 reply; 17+ messages in thread
From: Dmitry V. Levin @ 2018-11-28 13:06 UTC (permalink / raw)
  To: Oleg Nesterov, Andy Lutomirski
  Cc: Elvira Khabirova, Eugene Syromyatnikov, Steven Rostedt,
	Ingo Molnar, Kees Cook, Jann Horn, Michael Ellerman, linux-api,
	linux-kernel, strace-devel

From: Elvira Khabirova <lineprinter@altlinux.org>

Define two constants, PTRACE_EVENTMSG_SYSCALL_ENTRY and
PTRACE_EVENTMSG_SYSCALL_EXIT, and place them in ptrace_message
for the duration of syscall-stops.
This way ptracers can distinguish syscall-enter-stops
from syscall-exit-stops using PTRACE_GETEVENTMSG request.

Signed-off-by: Elvira Khabirova <lineprinter@altlinux.org>
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
 include/linux/tracehook.h   |  9 ++++++---
 include/uapi/linux/ptrace.h | 10 ++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
index 40b0b4c1bf7b..633a83fe7051 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..cb138902d042 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -104,6 +104,16 @@ struct seccomp_metadata {
 #define PTRACE_O_MASK		(\
 	0x000000ff | PTRACE_O_EXITKILL | PTRACE_O_SUSPEND_SECCOMP)
 
+/*
+ * These values are stored in task->ptrace_message by tracehook_report_syscall_*
+ * to describe current syscall-stop.
+ *
+ * Values for these constants are chosen so that they do not appear
+ * in task->ptrace_message by other means.
+ */
+#define PTRACE_EVENTMSG_SYSCALL_ENTRY	0x80000000U
+#define PTRACE_EVENTMSG_SYSCALL_EXIT	0x90000000U
+
 #include <asm/ptrace.h>
 
 
-- 
ldv

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v4 2/2] ptrace: add PTRACE_GET_SYSCALL_INFO request
  2018-11-28 13:04 [PATCH v4 0/2] ptrace: add PTRACE_GET_SYSCALL_INFO request Dmitry V. Levin
  2018-11-28 13:06 ` [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message Dmitry V. Levin
@ 2018-11-28 13:07 ` Dmitry V. Levin
  2018-11-28 14:10   ` Oleg Nesterov
  1 sibling, 1 reply; 17+ messages in thread
From: Dmitry V. Levin @ 2018-11-28 13:07 UTC (permalink / raw)
  To: Oleg Nesterov, Andy Lutomirski
  Cc: Elvira Khabirova, Eugene Syromyatnikov, Steven Rostedt,
	Ingo Molnar, Kees Cook, Jann Horn, Michael Ellerman, linux-api,
	linux-kernel, strace-devel

From: Elvira Khabirova <lineprinter@altlinux.org>

PTRACE_GET_SYSCALL_INFO lets ptracer obtain details of the syscall
the tracee is blocked in.  The request succeeds when the tracee is
in a syscall-enter-stop, syscall-exit-stop, or PTRACE_EVENT_SECCOMP,
and fails with -EINVAL otherwise.

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.

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.
       This operation fails with EINVAL if the tracee is not
       in a syscall-enter-stop, a syscall-exit-stop, or
       a PTRACE_EVENT_SECCOMP stop.

Co-authored-by: Dmitry V. Levin <ldv@altlinux.org>
Signed-off-by: Elvira Khabirova <lineprinter@altlinux.org>
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
 include/uapi/linux/ptrace.h |  34 +++++++++++
 kernel/ptrace.c             | 110 +++++++++++++++++++++++++++++++++++-
 2 files changed, 143 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index cb138902d042..ef42149e77f4 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -73,6 +73,40 @@ struct seccomp_metadata {
 	__u64 flags;		/* Output: filter's flags */
 };
 
+#define PTRACE_GET_SYSCALL_INFO		0x420f
+#define PTRACE_SYSCALL_INFO_ENTRY	0
+#define PTRACE_SYSCALL_INFO_EXIT	1
+#define PTRACE_SYSCALL_INFO_SECCOMP	2
+
+struct ptrace_syscall_info {
+	__u8 op;	/* PTRACE_SYSCALL_INFO_* */
+	__u8 __pad0[3];
+	__u32 arch;
+	union {
+		struct {
+			__u64 nr;
+			__u64 instruction_pointer;
+			__u64 stack_pointer;
+			__u64 frame_pointer;
+			__u64 args[6];
+		} entry;
+		struct {
+			__s64 rval;
+			__u8 is_error;
+			__u8 __pad1[7];
+		} exit;
+		struct {
+			__u64 nr;
+			__u64 instruction_pointer;
+			__u64 stack_pointer;
+			__u64 frame_pointer;
+			__u64 args[6];
+			__u32 ret_data;
+			__u8 __pad2[4];
+		} seccomp;
+	};
+};
+
 /* 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 80b34dffdfb9..32ff9c97f941 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -30,6 +30,10 @@
 #include <linux/cn_proc.h>
 #include <linux/compat.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,
@@ -888,7 +892,105 @@ 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 ptrace_syscall_info *info)
+{
+	struct pt_regs *regs = task_pt_regs(child);
+	unsigned long args[ARRAY_SIZE(info->entry.args)];
+	int i;
+
+	info->op = PTRACE_SYSCALL_INFO_ENTRY;
+	info->arch = syscall_get_arch(child);
+	info->entry.nr = syscall_get_nr(child, regs);
+	info->entry.instruction_pointer = instruction_pointer(regs);
+	info->entry.stack_pointer = user_stack_pointer(regs);
+	info->entry.frame_pointer = frame_pointer(regs);
+	syscall_get_arguments(child, regs, 0, ARRAY_SIZE(args), args);
+	for (i = 0; i < ARRAY_SIZE(args); i++)
+		info->entry.args[i] = args[i];
+
+	return offsetofend(struct ptrace_syscall_info, entry);
+}
+
+static unsigned long
+ptrace_get_syscall_info_seccomp(struct task_struct *child,
+				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, info);
+	info->op = PTRACE_SYSCALL_INFO_SECCOMP;
+	info->seccomp.ret_data = child->ptrace_message;
+
+	return offsetofend(struct ptrace_syscall_info, seccomp);
+}
+
+static unsigned long
+ptrace_get_syscall_info_exit(struct task_struct *child,
+			     struct ptrace_syscall_info *info)
+{
+	struct pt_regs *regs = task_pt_regs(child);
+
+	info->op = PTRACE_SYSCALL_INFO_EXIT;
+	info->arch = syscall_get_arch(child);
+	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);
+
+	return offsetofend(struct ptrace_syscall_info, exit);
+}
+
+static int
+ptrace_get_syscall_info(struct task_struct *child, unsigned long user_size,
+			void __user *datavp)
+{
+	unsigned long actual_size = 0;
+	unsigned long write_size;
+	struct ptrace_syscall_info info;
+
+	if (!child->last_siginfo)
+		return -EINVAL;
+
+	/*
+	 * 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->si_code) {
+	case SIGTRAP | 0x80:
+		switch (child->ptrace_message) {
+		case PTRACE_EVENTMSG_SYSCALL_ENTRY:
+			actual_size =
+				ptrace_get_syscall_info_entry(child, &info);
+			break;
+		case PTRACE_EVENTMSG_SYSCALL_EXIT:
+			actual_size =
+				ptrace_get_syscall_info_exit(child, &info);
+			break;
+		}
+		break;
+	case SIGTRAP | (PTRACE_EVENT_SECCOMP << 8):
+		actual_size = ptrace_get_syscall_info_seccomp(child, &info);
+		break;
+	}
+
+	if (!actual_size)
+		return -EINVAL;
+
+	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)
@@ -1105,6 +1207,12 @@ int ptrace_request(struct task_struct *child, long request,
 		ret = seccomp_get_metadata(child, addr, datavp);
 		break;
 
+#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
+	case PTRACE_GET_SYSCALL_INFO:
+		ret = ptrace_get_syscall_info(child, addr, datavp);
+		break;
+#endif
+
 	default:
 		break;
 	}
-- 
ldv

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
  2018-11-28 13:06 ` [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message Dmitry V. Levin
@ 2018-11-28 13:49   ` Oleg Nesterov
  2018-11-28 14:05     ` Dmitry V. Levin
  0 siblings, 1 reply; 17+ messages in thread
From: Oleg Nesterov @ 2018-11-28 13:49 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Andy Lutomirski, Elvira Khabirova, Eugene Syromyatnikov,
	Steven Rostedt, Ingo Molnar, Kees Cook, Jann Horn,
	Michael Ellerman, linux-api, linux-kernel, strace-devel

On 11/28, Dmitry V. Levin wrote:
>
> +/*
> + * These values are stored in task->ptrace_message by tracehook_report_syscall_*
> + * to describe current syscall-stop.
> + *
> + * Values for these constants are chosen so that they do not appear
> + * in task->ptrace_message by other means.
> + */
> +#define PTRACE_EVENTMSG_SYSCALL_ENTRY	0x80000000U
> +#define PTRACE_EVENTMSG_SYSCALL_EXIT	0x90000000U

Again, I do not really understand the comment... Why should we care about
"do not appear in task->ptrace_message by other means" ?

2/2 should detect ptrace_report_syscall() case correctly, so we can use any
numbers, say, 1 and 2?

If debugger does PTRACE_GETEVENTMSG it should know how to interpet the value
anyway after wait(status).

Oleg.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
  2018-11-28 13:49   ` Oleg Nesterov
@ 2018-11-28 14:05     ` Dmitry V. Levin
  2018-11-28 14:20       ` Oleg Nesterov
  0 siblings, 1 reply; 17+ messages in thread
From: Dmitry V. Levin @ 2018-11-28 14:05 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Kees Cook, Jann Horn, Michael Ellerman, Elvira Khabirova,
	Eugene Syromyatnikov, Steven Rostedt, linux-kernel,
	Andy Lutomirski, linux-api, Ingo Molnar, strace-devel

[-- Attachment #1: Type: text/plain, Size: 1358 bytes --]

On Wed, Nov 28, 2018 at 02:49:14PM +0100, Oleg Nesterov wrote:
> On 11/28, Dmitry V. Levin wrote:
> >
> > +/*
> > + * These values are stored in task->ptrace_message by tracehook_report_syscall_*
> > + * to describe current syscall-stop.
> > + *
> > + * Values for these constants are chosen so that they do not appear
> > + * in task->ptrace_message by other means.
> > + */
> > +#define PTRACE_EVENTMSG_SYSCALL_ENTRY	0x80000000U
> > +#define PTRACE_EVENTMSG_SYSCALL_EXIT	0x90000000U
> 
> Again, I do not really understand the comment... Why should we care about
> "do not appear in task->ptrace_message by other means" ?
> 
> 2/2 should detect ptrace_report_syscall() case correctly, so we can use any
> numbers, say, 1 and 2?
> 
> If debugger does PTRACE_GETEVENTMSG it should know how to interpet the value
> anyway after wait(status).

Given that without this patch the value returned by PTRACE_GETEVENTMSG
during syscall stop is undefined, we need two different ptrace_message
values that cannot be set by other ptrace events to enable reliable
identification of syscall-enter-stop and syscall-exit-stop in userspace:
if we make PTRACE_GETEVENTMSG return 0 or any other value routinely set by
other ptrace events, it would be hard for userspace to find out whether
the kernel implements new semantics or not.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/2] ptrace: add PTRACE_GET_SYSCALL_INFO request
  2018-11-28 13:07 ` [PATCH v4 2/2] ptrace: add PTRACE_GET_SYSCALL_INFO request Dmitry V. Levin
@ 2018-11-28 14:10   ` Oleg Nesterov
  2018-11-28 14:29     ` Oleg Nesterov
  0 siblings, 1 reply; 17+ messages in thread
From: Oleg Nesterov @ 2018-11-28 14:10 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Andy Lutomirski, Elvira Khabirova, Eugene Syromyatnikov,
	Steven Rostedt, Ingo Molnar, Kees Cook, Jann Horn,
	Michael Ellerman, linux-api, linux-kernel, strace-devel

On 11/28, Dmitry V. Levin wrote:
>
> +static unsigned long
> +ptrace_get_syscall_info_entry(struct task_struct *child,
> +			      struct ptrace_syscall_info *info)
> +{
> +	struct pt_regs *regs = task_pt_regs(child);
> +	unsigned long args[ARRAY_SIZE(info->entry.args)];
> +	int i;
> +
> +	info->op = PTRACE_SYSCALL_INFO_ENTRY;
> +	info->arch = syscall_get_arch(child);
> +	info->entry.nr = syscall_get_nr(child, regs);
> +	info->entry.instruction_pointer = instruction_pointer(regs);
> +	info->entry.stack_pointer = user_stack_pointer(regs);
> +	info->entry.frame_pointer = frame_pointer(regs);
> +	syscall_get_arguments(child, regs, 0, ARRAY_SIZE(args), args);
> +	for (i = 0; i < ARRAY_SIZE(args); i++)
> +		info->entry.args[i] = args[i];

I must have missed something, but why do we need the temporary args[],
syscall_get_arguments(..., info->entry.args) should equally work?

Oleg.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
  2018-11-28 14:05     ` Dmitry V. Levin
@ 2018-11-28 14:20       ` Oleg Nesterov
  2018-11-28 15:23         ` Dmitry V. Levin
  0 siblings, 1 reply; 17+ messages in thread
From: Oleg Nesterov @ 2018-11-28 14:20 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Kees Cook, Jann Horn, Michael Ellerman, Elvira Khabirova,
	Eugene Syromyatnikov, Steven Rostedt, linux-kernel,
	Andy Lutomirski, linux-api, Ingo Molnar, strace-devel

On 11/28, Dmitry V. Levin wrote:
>
> On Wed, Nov 28, 2018 at 02:49:14PM +0100, Oleg Nesterov wrote:
> > On 11/28, Dmitry V. Levin wrote:
> > >
> > > +/*
> > > + * These values are stored in task->ptrace_message by tracehook_report_syscall_*
> > > + * to describe current syscall-stop.
> > > + *
> > > + * Values for these constants are chosen so that they do not appear
> > > + * in task->ptrace_message by other means.
> > > + */
> > > +#define PTRACE_EVENTMSG_SYSCALL_ENTRY	0x80000000U
> > > +#define PTRACE_EVENTMSG_SYSCALL_EXIT	0x90000000U
> > 
> > Again, I do not really understand the comment... Why should we care about
> > "do not appear in task->ptrace_message by other means" ?
> > 
> > 2/2 should detect ptrace_report_syscall() case correctly, so we can use any
> > numbers, say, 1 and 2?
> > 
> > If debugger does PTRACE_GETEVENTMSG it should know how to interpet the value
> > anyway after wait(status).
> 
> Given that without this patch the value returned by PTRACE_GETEVENTMSG
> during syscall stop is undefined, we need two different ptrace_message
> values that cannot be set by other ptrace events to enable reliable
> identification of syscall-enter-stop and syscall-exit-stop in userspace:
> if we make PTRACE_GETEVENTMSG return 0 or any other value routinely set by
> other ptrace events, it would be hard for userspace to find out whether
> the kernel implements new semantics or not.

Hmm, why? Debugger can just do ptrace(PTRACE_GET_SYSCALL_INFO, NULL), if it
returns EIO then it is not implemented?

Oleg.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/2] ptrace: add PTRACE_GET_SYSCALL_INFO request
  2018-11-28 14:10   ` Oleg Nesterov
@ 2018-11-28 14:29     ` Oleg Nesterov
  0 siblings, 0 replies; 17+ messages in thread
From: Oleg Nesterov @ 2018-11-28 14:29 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Andy Lutomirski, Elvira Khabirova, Eugene Syromyatnikov,
	Steven Rostedt, Ingo Molnar, Kees Cook, Jann Horn,
	Michael Ellerman, linux-api, linux-kernel, strace-devel

On 11/28, Oleg Nesterov wrote:
>
> On 11/28, Dmitry V. Levin wrote:
> >
> > +static unsigned long
> > +ptrace_get_syscall_info_entry(struct task_struct *child,
> > +			      struct ptrace_syscall_info *info)
> > +{
> > +	struct pt_regs *regs = task_pt_regs(child);
> > +	unsigned long args[ARRAY_SIZE(info->entry.args)];
> > +	int i;
> > +
> > +	info->op = PTRACE_SYSCALL_INFO_ENTRY;
> > +	info->arch = syscall_get_arch(child);
> > +	info->entry.nr = syscall_get_nr(child, regs);
> > +	info->entry.instruction_pointer = instruction_pointer(regs);
> > +	info->entry.stack_pointer = user_stack_pointer(regs);
> > +	info->entry.frame_pointer = frame_pointer(regs);
> > +	syscall_get_arguments(child, regs, 0, ARRAY_SIZE(args), args);
> > +	for (i = 0; i < ARRAY_SIZE(args); i++)
> > +		info->entry.args[i] = args[i];
>
> I must have missed something,

Yes ;) I didn't look at info->entry.args, it is __u64[6].

> but why do we need the temporary args[],
> syscall_get_arguments(..., info->entry.args) should equally work?
> 
> Oleg.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
  2018-11-28 14:20       ` Oleg Nesterov
@ 2018-11-28 15:23         ` Dmitry V. Levin
  2018-11-28 22:11           ` Dmitry V. Levin
  0 siblings, 1 reply; 17+ messages in thread
From: Dmitry V. Levin @ 2018-11-28 15:23 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Kees Cook, Jann Horn, Michael Ellerman, Elvira Khabirova,
	Eugene Syromyatnikov, Steven Rostedt, linux-kernel,
	Andy Lutomirski, linux-api, Ingo Molnar, strace-devel

[-- Attachment #1: Type: text/plain, Size: 2056 bytes --]

On Wed, Nov 28, 2018 at 03:20:06PM +0100, Oleg Nesterov wrote:
> On 11/28, Dmitry V. Levin wrote:
> > On Wed, Nov 28, 2018 at 02:49:14PM +0100, Oleg Nesterov wrote:
> > > On 11/28, Dmitry V. Levin wrote:
> > > >
> > > > +/*
> > > > + * These values are stored in task->ptrace_message by tracehook_report_syscall_*
> > > > + * to describe current syscall-stop.
> > > > + *
> > > > + * Values for these constants are chosen so that they do not appear
> > > > + * in task->ptrace_message by other means.
> > > > + */
> > > > +#define PTRACE_EVENTMSG_SYSCALL_ENTRY	0x80000000U
> > > > +#define PTRACE_EVENTMSG_SYSCALL_EXIT	0x90000000U
> > > 
> > > Again, I do not really understand the comment... Why should we care about
> > > "do not appear in task->ptrace_message by other means" ?
> > > 
> > > 2/2 should detect ptrace_report_syscall() case correctly, so we can use any
> > > numbers, say, 1 and 2?
> > > 
> > > If debugger does PTRACE_GETEVENTMSG it should know how to interpet the value
> > > anyway after wait(status).
> > 
> > Given that without this patch the value returned by PTRACE_GETEVENTMSG
> > during syscall stop is undefined, we need two different ptrace_message
> > values that cannot be set by other ptrace events to enable reliable
> > identification of syscall-enter-stop and syscall-exit-stop in userspace:
> > if we make PTRACE_GETEVENTMSG return 0 or any other value routinely set by
> > other ptrace events, it would be hard for userspace to find out whether
> > the kernel implements new semantics or not.
> 
> Hmm, why? Debugger can just do ptrace(PTRACE_GET_SYSCALL_INFO, NULL), if it
> returns EIO then it is not implemented?

The debugger that uses PTRACE_GET_SYSCALL_INFO does not need to call
PTRACE_GETEVENTMSG for syscall stops.
My concern here is the PTRACE_GETEVENTMSG interface itself.  If we use
ptrace_message to implement PTRACE_GET_SYSCALL_INFO and expose
PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} for regular PTRACE_GETEVENTMSG users,
it should have clear semantics.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
  2018-11-28 15:23         ` Dmitry V. Levin
@ 2018-11-28 22:11           ` Dmitry V. Levin
  2018-11-28 23:17             ` Andy Lutomirski
  2018-11-29 14:47             ` Oleg Nesterov
  0 siblings, 2 replies; 17+ messages in thread
From: Dmitry V. Levin @ 2018-11-28 22:11 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Kees Cook, Jann Horn, Michael Ellerman, Elvira Khabirova,
	Eugene Syromyatnikov, Steven Rostedt, linux-kernel,
	Andy Lutomirski, linux-api, Ingo Molnar, strace-devel

[-- Attachment #1: Type: text/plain, Size: 3344 bytes --]

On Wed, Nov 28, 2018 at 06:23:46PM +0300, Dmitry V. Levin wrote:
> On Wed, Nov 28, 2018 at 03:20:06PM +0100, Oleg Nesterov wrote:
> > On 11/28, Dmitry V. Levin wrote:
> > > On Wed, Nov 28, 2018 at 02:49:14PM +0100, Oleg Nesterov wrote:
> > > > On 11/28, Dmitry V. Levin wrote:
> > > > >
> > > > > +/*
> > > > > + * These values are stored in task->ptrace_message by tracehook_report_syscall_*
> > > > > + * to describe current syscall-stop.
> > > > > + *
> > > > > + * Values for these constants are chosen so that they do not appear
> > > > > + * in task->ptrace_message by other means.
> > > > > + */
> > > > > +#define PTRACE_EVENTMSG_SYSCALL_ENTRY	0x80000000U
> > > > > +#define PTRACE_EVENTMSG_SYSCALL_EXIT	0x90000000U
> > > > 
> > > > Again, I do not really understand the comment... Why should we care about
> > > > "do not appear in task->ptrace_message by other means" ?
> > > > 
> > > > 2/2 should detect ptrace_report_syscall() case correctly, so we can use any
> > > > numbers, say, 1 and 2?
> > > > 
> > > > If debugger does PTRACE_GETEVENTMSG it should know how to interpet the value
> > > > anyway after wait(status).
> > > 
> > > Given that without this patch the value returned by PTRACE_GETEVENTMSG
> > > during syscall stop is undefined, we need two different ptrace_message
> > > values that cannot be set by other ptrace events to enable reliable
> > > identification of syscall-enter-stop and syscall-exit-stop in userspace:
> > > if we make PTRACE_GETEVENTMSG return 0 or any other value routinely set by
> > > other ptrace events, it would be hard for userspace to find out whether
> > > the kernel implements new semantics or not.
> > 
> > Hmm, why? Debugger can just do ptrace(PTRACE_GET_SYSCALL_INFO, NULL), if it
> > returns EIO then it is not implemented?
> 
> The debugger that uses PTRACE_GET_SYSCALL_INFO does not need to call
> PTRACE_GETEVENTMSG for syscall stops.
> My concern here is the PTRACE_GETEVENTMSG interface itself.  If we use
> ptrace_message to implement PTRACE_GET_SYSCALL_INFO and expose
> PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} for regular PTRACE_GETEVENTMSG users,
> it should have clear semantics.

Since our implementation of PTRACE_GET_SYSCALL_INFO uses ptrace_message
to distinguish syscall-enter-stop from syscall-exit-stop, we could choose
one of the following approaches:

1. Do not document the values saved into ptrace_message during syscall
stops (and exposed via PTRACE_GETEVENTMSG) as a part of ptrace API,
leaving the value returned by PTRACE_GETEVENTMSG during syscall stops
as undefined.

2. Document these values chosen to avoid collisions with ptrace_message values
set by other ptrace events so that PTRACE_GETEVENTMSG users can easily tell
whether this new semantics is supported by the kernel or not.

The first approach was implemented in v2 of this series: the constants
were PT_SYSCALL_IS_{ENTERING,EXITING} defined in include/linux/ptrace.h.

The second approach was implemented in v3: the constants are
PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} defined in include/uapi/linux/ptrace.h,
they are also going to be documented in ptrace(2) man page.

Since the use of ptrace_message is exposed to PTRACE_GETEVENTMSG users
anyway, I do not see any reason to choose the first approach over the
second.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
  2018-11-28 22:11           ` Dmitry V. Levin
@ 2018-11-28 23:17             ` Andy Lutomirski
  2018-11-29 10:34               ` Dmitry V. Levin
  2018-11-29 15:03               ` Oleg Nesterov
  2018-11-29 14:47             ` Oleg Nesterov
  1 sibling, 2 replies; 17+ messages in thread
From: Andy Lutomirski @ 2018-11-28 23:17 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Oleg Nesterov, Kees Cook, Jann Horn, Michael Ellerman,
	Elvira Khabirova, Eugene Syromiatnikov, Steven Rostedt, LKML,
	Andrew Lutomirski, Linux API, Ingo Molnar, strace-devel

On Wed, Nov 28, 2018 at 2:11 PM Dmitry V. Levin <ldv@altlinux.org> wrote:
>
> On Wed, Nov 28, 2018 at 06:23:46PM +0300, Dmitry V. Levin wrote:
> > On Wed, Nov 28, 2018 at 03:20:06PM +0100, Oleg Nesterov wrote:
> > > On 11/28, Dmitry V. Levin wrote:
> > > > On Wed, Nov 28, 2018 at 02:49:14PM +0100, Oleg Nesterov wrote:
> > > > > On 11/28, Dmitry V. Levin wrote:
> > > > > >
> > > > > > +/*
> > > > > > + * These values are stored in task->ptrace_message by tracehook_report_syscall_*
> > > > > > + * to describe current syscall-stop.
> > > > > > + *
> > > > > > + * Values for these constants are chosen so that they do not appear
> > > > > > + * in task->ptrace_message by other means.
> > > > > > + */
> > > > > > +#define PTRACE_EVENTMSG_SYSCALL_ENTRY        0x80000000U
> > > > > > +#define PTRACE_EVENTMSG_SYSCALL_EXIT 0x90000000U
> > > > >
> > > > > Again, I do not really understand the comment... Why should we care about
> > > > > "do not appear in task->ptrace_message by other means" ?
> > > > >
> > > > > 2/2 should detect ptrace_report_syscall() case correctly, so we can use any
> > > > > numbers, say, 1 and 2?
> > > > >
> > > > > If debugger does PTRACE_GETEVENTMSG it should know how to interpet the value
> > > > > anyway after wait(status).
> > > >
> > > > Given that without this patch the value returned by PTRACE_GETEVENTMSG
> > > > during syscall stop is undefined, we need two different ptrace_message
> > > > values that cannot be set by other ptrace events to enable reliable
> > > > identification of syscall-enter-stop and syscall-exit-stop in userspace:
> > > > if we make PTRACE_GETEVENTMSG return 0 or any other value routinely set by
> > > > other ptrace events, it would be hard for userspace to find out whether
> > > > the kernel implements new semantics or not.
> > >
> > > Hmm, why? Debugger can just do ptrace(PTRACE_GET_SYSCALL_INFO, NULL), if it
> > > returns EIO then it is not implemented?
> >
> > The debugger that uses PTRACE_GET_SYSCALL_INFO does not need to call
> > PTRACE_GETEVENTMSG for syscall stops.
> > My concern here is the PTRACE_GETEVENTMSG interface itself.  If we use
> > ptrace_message to implement PTRACE_GET_SYSCALL_INFO and expose
> > PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} for regular PTRACE_GETEVENTMSG users,
> > it should have clear semantics.
>
> Since our implementation of PTRACE_GET_SYSCALL_INFO uses ptrace_message
> to distinguish syscall-enter-stop from syscall-exit-stop, we could choose
> one of the following approaches:
>
> 1. Do not document the values saved into ptrace_message during syscall
> stops (and exposed via PTRACE_GETEVENTMSG) as a part of ptrace API,
> leaving the value returned by PTRACE_GETEVENTMSG during syscall stops
> as undefined.
>
> 2. Document these values chosen to avoid collisions with ptrace_message values
> set by other ptrace events so that PTRACE_GETEVENTMSG users can easily tell
> whether this new semantics is supported by the kernel or not.

I don't like any of this at all.  Can we please choose a sensible API
design and let the API drive the implementation instead of vice versa?
 ISTM the correct solution is to add some new state to task_struct for
this.

If we're concerned about making task_struct bigger, I have a
half-finished patch to factor all the ptrace tracee state into a
separate struct.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
  2018-11-28 23:17             ` Andy Lutomirski
@ 2018-11-29 10:34               ` Dmitry V. Levin
  2018-11-29 15:03               ` Oleg Nesterov
  1 sibling, 0 replies; 17+ messages in thread
From: Dmitry V. Levin @ 2018-11-29 10:34 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Oleg Nesterov, Kees Cook, Jann Horn, Michael Ellerman,
	Elvira Khabirova, Eugene Syromiatnikov, Steven Rostedt, LKML,
	Linux API, Ingo Molnar, strace-devel

[-- Attachment #1: Type: text/plain, Size: 3930 bytes --]

On Wed, Nov 28, 2018 at 03:17:49PM -0800, Andy Lutomirski wrote:
> On Wed, Nov 28, 2018 at 2:11 PM Dmitry V. Levin <ldv@altlinux.org> wrote:
> >
> > On Wed, Nov 28, 2018 at 06:23:46PM +0300, Dmitry V. Levin wrote:
> > > On Wed, Nov 28, 2018 at 03:20:06PM +0100, Oleg Nesterov wrote:
> > > > On 11/28, Dmitry V. Levin wrote:
> > > > > On Wed, Nov 28, 2018 at 02:49:14PM +0100, Oleg Nesterov wrote:
> > > > > > On 11/28, Dmitry V. Levin wrote:
> > > > > > >
> > > > > > > +/*
> > > > > > > + * These values are stored in task->ptrace_message by tracehook_report_syscall_*
> > > > > > > + * to describe current syscall-stop.
> > > > > > > + *
> > > > > > > + * Values for these constants are chosen so that they do not appear
> > > > > > > + * in task->ptrace_message by other means.
> > > > > > > + */
> > > > > > > +#define PTRACE_EVENTMSG_SYSCALL_ENTRY        0x80000000U
> > > > > > > +#define PTRACE_EVENTMSG_SYSCALL_EXIT 0x90000000U
> > > > > >
> > > > > > Again, I do not really understand the comment... Why should we care about
> > > > > > "do not appear in task->ptrace_message by other means" ?
> > > > > >
> > > > > > 2/2 should detect ptrace_report_syscall() case correctly, so we can use any
> > > > > > numbers, say, 1 and 2?
> > > > > >
> > > > > > If debugger does PTRACE_GETEVENTMSG it should know how to interpet the value
> > > > > > anyway after wait(status).
> > > > >
> > > > > Given that without this patch the value returned by PTRACE_GETEVENTMSG
> > > > > during syscall stop is undefined, we need two different ptrace_message
> > > > > values that cannot be set by other ptrace events to enable reliable
> > > > > identification of syscall-enter-stop and syscall-exit-stop in userspace:
> > > > > if we make PTRACE_GETEVENTMSG return 0 or any other value routinely set by
> > > > > other ptrace events, it would be hard for userspace to find out whether
> > > > > the kernel implements new semantics or not.
> > > >
> > > > Hmm, why? Debugger can just do ptrace(PTRACE_GET_SYSCALL_INFO, NULL), if it
> > > > returns EIO then it is not implemented?
> > >
> > > The debugger that uses PTRACE_GET_SYSCALL_INFO does not need to call
> > > PTRACE_GETEVENTMSG for syscall stops.
> > > My concern here is the PTRACE_GETEVENTMSG interface itself.  If we use
> > > ptrace_message to implement PTRACE_GET_SYSCALL_INFO and expose
> > > PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} for regular PTRACE_GETEVENTMSG users,
> > > it should have clear semantics.
> >
> > Since our implementation of PTRACE_GET_SYSCALL_INFO uses ptrace_message
> > to distinguish syscall-enter-stop from syscall-exit-stop, we could choose
> > one of the following approaches:
> >
> > 1. Do not document the values saved into ptrace_message during syscall
> > stops (and exposed via PTRACE_GETEVENTMSG) as a part of ptrace API,
> > leaving the value returned by PTRACE_GETEVENTMSG during syscall stops
> > as undefined.
> >
> > 2. Document these values chosen to avoid collisions with ptrace_message values
> > set by other ptrace events so that PTRACE_GETEVENTMSG users can easily tell
> > whether this new semantics is supported by the kernel or not.
> 
> I don't like any of this at all.  Can we please choose a sensible API
> design and let the API drive the implementation instead of vice versa?

What are your concerns?  Do you see something wrong in exposing this
information via PTRACE_GETEVENTMSG?

Anyway, can we agree on the PTRACE_GET_SYSCALL_INFO API, please?

>  ISTM the correct solution is to add some new state to task_struct for
> this.
> 
> If we're concerned about making task_struct bigger, I have a
> half-finished patch to factor all the ptrace tracee state into a
> separate struct.

This is refactoring of the kernel - a thing userspace people are not
the best equipped to do.  This part should rather be sorted out by kernel
people.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
  2018-11-28 22:11           ` Dmitry V. Levin
  2018-11-28 23:17             ` Andy Lutomirski
@ 2018-11-29 14:47             ` Oleg Nesterov
  2018-11-29 21:10               ` Dmitry V. Levin
  1 sibling, 1 reply; 17+ messages in thread
From: Oleg Nesterov @ 2018-11-29 14:47 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Kees Cook, Jann Horn, Michael Ellerman, Elvira Khabirova,
	Eugene Syromyatnikov, Steven Rostedt, linux-kernel,
	Andy Lutomirski, linux-api, Ingo Molnar, strace-devel

On 11/29, Dmitry V. Levin wrote:
>
> 2. Document these values

sure, they should be documented and live in include/uapi/,

> chosen to avoid collisions with ptrace_message values
> set by other ptrace events

this is what I can't understand. But to clarify, I don't really care and
won't argue.

If an application wants to use PTRACE_GETEVENTMSG to distinguish entry/exit
(without PTRACE_GET_SYSCALL_INFO) it needs to do wait(status) and check status
anyway, otherwise PTRACE_GETEVENTMSG is simply pointless (wrt syscall entry/
exit). So we do not care if PTRACE_EVENTMSG_SYSCALL_ENTRY conflicts with, say,
SECCOMP_RET_DATA.

> so that PTRACE_GETEVENTMSG users can easily tell
> whether this new semantics is supported by the kernel or not.

Yes. And how much this can help? Again, an application can trivially detect
if this feature implemented or not, and it should do this anyway if it wants
to (try to) use PTRACE_EVENTMSG_SYSCALL_ENTRY/EXIT ?


Again, I won't reallly argue. But if you insist that these values must be
unique then you probably need to add

	BUILD_BUG_ON(PTRACE_EVENTMSG_SYSCALL_ENTRY <= PID_MAX_LIMIT);

?

OK, please forget...

Oleg.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
  2018-11-28 23:17             ` Andy Lutomirski
  2018-11-29 10:34               ` Dmitry V. Levin
@ 2018-11-29 15:03               ` Oleg Nesterov
  1 sibling, 0 replies; 17+ messages in thread
From: Oleg Nesterov @ 2018-11-29 15:03 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Dmitry V. Levin, Kees Cook, Jann Horn, Michael Ellerman,
	Elvira Khabirova, Eugene Syromiatnikov, Steven Rostedt, LKML,
	Linux API, Ingo Molnar, strace-devel

On 11/28, Andy Lutomirski wrote:
>
> I don't like any of this at all.  Can we please choose a sensible API
> design and let the API drive the implementation instead of vice versa?

I too do not understand your concerns...

>  ISTM the correct solution is to add some new state to task_struct for
> this.

Sure we can do this. I have argued with the previous version not because
the new member blows the task_struct. Although I think it is better to avoid
this if possible.

But this doesn't affect the API.

Yes, this version uses ->ptrace_message but I think this is _good_ exactly
because it is already visible to userspace, so if debugger only needs to
distinguish syscall entry/exit it can simply use PTRACE_GETEVENTMSG without
PTRACE_GET_SYSCALL_INFO.

> If we're concerned about making task_struct bigger, I have a
> half-finished patch to factor all the ptrace tracee state into a
> separate struct.

I even sent the patch(es) which does this several years ago ;)

Oleg.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
  2018-11-29 14:47             ` Oleg Nesterov
@ 2018-11-29 21:10               ` Dmitry V. Levin
  2018-11-30 11:29                 ` Oleg Nesterov
  0 siblings, 1 reply; 17+ messages in thread
From: Dmitry V. Levin @ 2018-11-29 21:10 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Kees Cook, Jann Horn, Michael Ellerman, Elvira Khabirova,
	Eugene Syromyatnikov, Steven Rostedt, linux-kernel,
	Andy Lutomirski, linux-api, Ingo Molnar, strace-devel

[-- Attachment #1: Type: text/plain, Size: 2450 bytes --]

On Thu, Nov 29, 2018 at 03:47:43PM +0100, Oleg Nesterov wrote:
> On 11/29, Dmitry V. Levin wrote:
> >
> > 2. Document these values
> 
> sure, they should be documented and live in include/uapi/,
> 
> > chosen to avoid collisions with ptrace_message values
> > set by other ptrace events
> 
> this is what I can't understand. But to clarify, I don't really care and
> won't argue.
> 
> If an application wants to use PTRACE_GETEVENTMSG to distinguish entry/exit
> (without PTRACE_GET_SYSCALL_INFO) it needs to do wait(status) and check status
> anyway, otherwise PTRACE_GETEVENTMSG is simply pointless (wrt syscall entry/
> exit). So we do not care if PTRACE_EVENTMSG_SYSCALL_ENTRY conflicts with, say,
> SECCOMP_RET_DATA.

Yes, once the application has verified that the kernel implements this
feature, there is no risk of collision.

> > so that PTRACE_GETEVENTMSG users can easily tell
> > whether this new semantics is supported by the kernel or not.
> 
> Yes. And how much this can help? Again, an application can trivially detect
> if this feature implemented or not, and it should do this anyway if it wants
> to (try to) use PTRACE_EVENTMSG_SYSCALL_ENTRY/EXIT ?

How an application can easily detect whether this feature is implemented?
By invoking PTRACE_GETEVENTMSG after the first syscall stop reported by
wait and checking whether the returned value is either
PTRACE_EVENTMSG_SYSCALL_ENTRY or PTRACE_EVENTMSG_SYSCALL_EXIT.

So the question is, how can this value be equal to one of these constants
when this feature is not implemented?  Can a value saved to ptrace_message
earlier by one of ptrace events be equal to one of these constants?

Imagine an application attaches to already existing process, enables
PTRACE_O_TRACESECCOMP, and a PTRACE_EVENT_SECCOMP arrives with
ptrace_message set to 1.  If this application then exits and a new invocation
of the same application attaches to the same process, it will very likely see
this 1 returned by PTRACE_GETEVENTMSG if the feature is not implemented
in the kernel.

To avoid that kind of collisions, kernel should use different ptrace_message
values for syscall stops.

> Again, I won't reallly argue. But if you insist that these values must
> be unique then you probably need to add
> 
> 	BUILD_BUG_ON(PTRACE_EVENTMSG_SYSCALL_ENTRY <= PID_MAX_LIMIT);

Yes, it's a good idea.  What is the proper place for this check?


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
  2018-11-29 21:10               ` Dmitry V. Levin
@ 2018-11-30 11:29                 ` Oleg Nesterov
  2018-11-30 22:53                   ` Dmitry V. Levin
  0 siblings, 1 reply; 17+ messages in thread
From: Oleg Nesterov @ 2018-11-30 11:29 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Kees Cook, Jann Horn, Michael Ellerman, Elvira Khabirova,
	Eugene Syromyatnikov, Steven Rostedt, linux-kernel,
	Andy Lutomirski, linux-api, Ingo Molnar, strace-devel

On 11/30, Dmitry V. Levin wrote:
>
> On Thu, Nov 29, 2018 at 03:47:43PM +0100, Oleg Nesterov wrote:
>
> > > so that PTRACE_GETEVENTMSG users can easily tell
> > > whether this new semantics is supported by the kernel or not.
> >
> > Yes. And how much this can help? Again, an application can trivially detect
> > if this feature implemented or not, and it should do this anyway if it wants
> > to (try to) use PTRACE_EVENTMSG_SYSCALL_ENTRY/EXIT ?
>
> How an application can easily detect whether this feature is implemented?

As I already said, it can just do ptrace(PTRACE_GET_SYSCALL_INFO, NULL) ?
If it returns -EIO then this feature is not implemented. Any other error
code (actually EINVAL or EFAULT) means it is implemented.

Oleg.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message
  2018-11-30 11:29                 ` Oleg Nesterov
@ 2018-11-30 22:53                   ` Dmitry V. Levin
  0 siblings, 0 replies; 17+ messages in thread
From: Dmitry V. Levin @ 2018-11-30 22:53 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Kees Cook, Jann Horn, Michael Ellerman, Elvira Khabirova,
	Eugene Syromyatnikov, Steven Rostedt, linux-kernel,
	Andy Lutomirski, linux-api, Ingo Molnar, strace-devel

[-- Attachment #1: Type: text/plain, Size: 974 bytes --]

On Fri, Nov 30, 2018 at 12:29:21PM +0100, Oleg Nesterov wrote:
> On 11/30, Dmitry V. Levin wrote:
> > On Thu, Nov 29, 2018 at 03:47:43PM +0100, Oleg Nesterov wrote:
> >
> > > > so that PTRACE_GETEVENTMSG users can easily tell
> > > > whether this new semantics is supported by the kernel or not.
> > >
> > > Yes. And how much this can help? Again, an application can trivially detect
> > > if this feature implemented or not, and it should do this anyway if it wants
> > > to (try to) use PTRACE_EVENTMSG_SYSCALL_ENTRY/EXIT ?
> >
> > How an application can easily detect whether this feature is implemented?
> 
> As I already said, it can just do ptrace(PTRACE_GET_SYSCALL_INFO, NULL) ?
> If it returns -EIO then this feature is not implemented. Any other error
> code (actually EINVAL or EFAULT) means it is implemented.

Fair enough.
We can change PTRACE_EVENTMSG_SYSCALL_ENTRY/EXIT to 1/2 if you like,
and document this trick somewhere.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2018-11-30 22:53 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-28 13:04 [PATCH v4 0/2] ptrace: add PTRACE_GET_SYSCALL_INFO request Dmitry V. Levin
2018-11-28 13:06 ` [PATCH v4 1/2] ptrace: save the type of syscall-stop in ptrace_message Dmitry V. Levin
2018-11-28 13:49   ` Oleg Nesterov
2018-11-28 14:05     ` Dmitry V. Levin
2018-11-28 14:20       ` Oleg Nesterov
2018-11-28 15:23         ` Dmitry V. Levin
2018-11-28 22:11           ` Dmitry V. Levin
2018-11-28 23:17             ` Andy Lutomirski
2018-11-29 10:34               ` Dmitry V. Levin
2018-11-29 15:03               ` Oleg Nesterov
2018-11-29 14:47             ` Oleg Nesterov
2018-11-29 21:10               ` Dmitry V. Levin
2018-11-30 11:29                 ` Oleg Nesterov
2018-11-30 22:53                   ` Dmitry V. Levin
2018-11-28 13:07 ` [PATCH v4 2/2] ptrace: add PTRACE_GET_SYSCALL_INFO request Dmitry V. Levin
2018-11-28 14:10   ` Oleg Nesterov
2018-11-28 14:29     ` Oleg Nesterov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).