[RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request

[RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request

[Elvira Khabirova]

PTRACE_GET_SYSCALL_INFO lets ptracer obtain details of the syscall
the tracee is blocked in.  The request returns meaningful data only
when the tracee is in a syscall-enter-stop or a syscall-exit-stop.

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; /* 0 for entry, 1 for exit */
	__u8 __pad0[7];
	union {
		struct {
			__s32 nr;
			__u32 arch;
			__u64 instruction_pointer;
			__u64 args[6];
		} entry_info;
		struct {
			__s64 rval;
			__u8 is_error;
			__u8 __pad1[7];
		} exit_info;
	};
};

The structure was chosen according to [2], except for one change:
a boolean is_error field is added along with rval.  This way the tracer
can more reliably distinguish a return value from an error value.

This patch 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/

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>
---
Changes since v1:
 * 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/ptrace.h      |  8 ++++++
 include/linux/tracehook.h   |  9 ++++--
 include/uapi/linux/ptrace.h | 20 +++++++++++++
 kernel/ptrace.c             | 56 +++++++++++++++++++++++++++++++++++++
 4 files changed, 90 insertions(+), 3 deletions(-)

diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 6c2ffed907f5..909930c893d0 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -46,6 +46,14 @@ extern int ptrace_access_vm(struct task_struct *tsk, unsigned long addr,
 #define PT_BLOCKSTEP_BIT	30
 #define PT_BLOCKSTEP		(1<<PT_BLOCKSTEP_BIT)
 
+/*
+ * These values are used by tracehook_report_syscall_* to store
+ * information about current syscall-stop in task->ptrace_message
+ * for later use by PTRACE_GET_SYSCALL_INFO.
+ */
+#define PT_SYSCALL_IS_ENTERING  0x80000000U
+#define PT_SYSCALL_IS_EXITING   0x90000000U
+
 extern long arch_ptrace(struct task_struct *child, long request,
 			unsigned long addr, unsigned long data);
 extern int ptrace_readdata(struct task_struct *tsk, unsigned long src, char __user *dst, int len);
diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
index 40b0b4c1bf7b..24d0e2215ed2 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, PT_SYSCALL_IS_ENTERING);
 }
 
 /**
@@ -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, PT_SYSCALL_IS_EXITING);
 }
 
 /**
diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index d5a1b8a492b9..3f19a4458309 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -73,6 +73,26 @@ struct seccomp_metadata {
 	__u64 flags;		/* Output: filter's flags */
 };
 
+#define PTRACE_GET_SYSCALL_INFO 0x420f
+
+struct ptrace_syscall_info {
+	__u8 op; /* 0 for entry, 1 for exit */
+	__u8 __pad0[7];
+	union {
+		struct {
+			__s32 nr;
+			__u32 arch;
+			__u64 instruction_pointer;
+			__u64 args[6];
+		} entry_info;
+		struct {
+			__s64 rval;
+			__u8 is_error;
+			__u8 __pad1[7];
+		} exit_info;
+	};
+};
+
 /* 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..7c2e92b6c762 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,
@@ -890,6 +894,52 @@ static int ptrace_regset(struct task_struct *task, int req, unsigned int type,
 EXPORT_SYMBOL_GPL(task_user_regset_view);
 #endif
 
+#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
+static int ptrace_get_syscall(struct task_struct *child,
+			      unsigned long user_size, void __user *datavp)
+{
+	struct ptrace_syscall_info info;
+	struct pt_regs *regs = task_pt_regs(child);
+	unsigned long args[ARRAY_SIZE(info.entry_info.args)];
+	unsigned long actual_size;
+	unsigned long write_size;
+	int i;
+
+	switch (child->ptrace_message) {
+	case PT_SYSCALL_IS_ENTERING:
+		info.op = 0;
+		info.entry_info.arch = syscall_get_arch(child);
+		info.entry_info.nr = syscall_get_nr(child, regs);
+		info.entry_info.instruction_pointer =
+			instruction_pointer(task_pt_regs(child));
+		syscall_get_arguments(child, regs, 0, ARRAY_SIZE(args), args);
+		for (i = 0; i < ARRAY_SIZE(args); i++)
+			info.entry_info.args[i] = args[i];
+		actual_size =
+			offsetofend(struct ptrace_syscall_info, entry_info);
+		break;
+
+	case PT_SYSCALL_IS_EXITING:
+		info.op = 1;
+		info.exit_info.rval = syscall_get_error(child, regs);
+		info.exit_info.is_error = !!info.exit_info.rval;
+		if (!info.exit_info.is_error) {
+			info.exit_info.rval =
+				syscall_get_return_value(child, regs);
+		}
+		actual_size =
+			offsetofend(struct ptrace_syscall_info, exit_info);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	write_size = min(actual_size, user_size);
+	return copy_to_user(datavp, &info, write_size) ? -EFAULT : actual_size;
+}
+#endif
+
 int ptrace_request(struct task_struct *child, long request,
 		   unsigned long addr, unsigned long data)
 {
@@ -1105,6 +1155,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(child, addr, datavp);
+		break;
+#endif
+
 	default:
 		break;
 	}
-- 
ldv

Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request

[Andy Lutomirski]

Please cc linux-api@vger.kernel.org for future versions.

On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova
<lineprinter@altlinux.org> wrote:
>
> struct ptrace_syscall_info {
>         __u8 op; /* 0 for entry, 1 for exit */

Can you add proper defines, like:

#define PTRACE_SYSCALL_ENTRY 0
#define PTRACE_SYSCALL_EXIT 1
#define PTRACE_SYSCALL_SECCOMP 2

and make seccomp work from the start?  I'd rather we don't merge an
implementation that doesn't work for seccomp and then have to rework
it later.

>         __u8 __pad0[7];
>         union {
>                 struct {
>                         __s32 nr;

__u64 please.  Syscall numbers are, as a practical matter, 64 bits.
Admittedly, the actual effects of setting the high bits are unclear,
and seccomp has issues with it, but let's not perpetuate the problem.

>                         __u32 arch;
>                         __u64 instruction_pointer;
>                         __u64 args[6];
>                 } entry_info;
>                 struct {
>                         __s64 rval;
>                         __u8 is_error;
>                         __u8 __pad1[7];
>                 } exit_info;
>         };
> };

Should seccomp events use entry_info or should they just literally
supply seccomp_data?

Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request

[Dmitry V. Levin]

[-- Attachment #1.1: Type: text/plain, Size: 2194 bytes --]

On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> Please cc linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org for future versions.
> 
> On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova wrote:
> >
> > struct ptrace_syscall_info {
> >         __u8 op; /* 0 for entry, 1 for exit */
> 
> Can you add proper defines, like:
> 
> #define PTRACE_SYSCALL_ENTRY 0
> #define PTRACE_SYSCALL_EXIT 1
> #define PTRACE_SYSCALL_SECCOMP 2
> 
> and make seccomp work from the start?  I'd rather we don't merge an
> implementation that doesn't work for seccomp and then have to rework
> it later.

What's the difference between PTRACE_EVENT_SECCOMP and syscall-entry-stop
with regards to PTRACE_GET_SYSCALL_INFO request?  At least they have the
same entry_info to return.

As long as implementation (ab)uses ptrace_message to tell one kind of stop
from another, it can distinguish syscall-entry-stop and syscall-exit-stop
from each other and from many other kinds of stops, but it cannot
distinguish PTRACE_EVENT_SECCOMP from e.g. PTRACE_EVENT_EXIT.

> >         __u8 __pad0[7];
> >         union {
> >                 struct {
> >                         __s32 nr;
> 
> __u64 please.  Syscall numbers are, as a practical matter, 64 bits.
> Admittedly, the actual effects of setting the high bits are unclear,
> and seccomp has issues with it, but let's not perpetuate the problem.

I agree.  Although the implementation uses syscall_get_nr()
which returns int, this could potentially be fixed in the future.

> >                         __u32 arch;
> >                         __u64 instruction_pointer;
> >                         __u64 args[6];
> >                 } entry_info;
> >                 struct {
> >                         __s64 rval;
> >                         __u8 is_error;
> >                         __u8 __pad1[7];
> >                 } exit_info;
> >         };
> > };
> 
> Should seccomp events use entry_info or should they just literally
> supply seccomp_data?

It certainly can use entry_info.
I'd prefer to avoid using in uapi/linux/ptrace.h those types
that are defined in uapi/linux/seccomp.h.


-- 
ldv

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

[-- Attachment #2: Type: text/plain, Size: 137 bytes --]

-- 
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel

Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request

[Dmitry V. Levin]

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

On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> Please cc linux-api@vger.kernel.org for future versions.
> 
> On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova wrote:
> >
> > struct ptrace_syscall_info {
> >         __u8 op; /* 0 for entry, 1 for exit */
> 
> Can you add proper defines, like:
> 
> #define PTRACE_SYSCALL_ENTRY 0
> #define PTRACE_SYSCALL_EXIT 1
> #define PTRACE_SYSCALL_SECCOMP 2
> 
> and make seccomp work from the start?  I'd rather we don't merge an
> implementation that doesn't work for seccomp and then have to rework
> it later.

What's the difference between PTRACE_EVENT_SECCOMP and syscall-entry-stop
with regards to PTRACE_GET_SYSCALL_INFO request?  At least they have the
same entry_info to return.

As long as implementation (ab)uses ptrace_message to tell one kind of stop
from another, it can distinguish syscall-entry-stop and syscall-exit-stop
from each other and from many other kinds of stops, but it cannot
distinguish PTRACE_EVENT_SECCOMP from e.g. PTRACE_EVENT_EXIT.

> >         __u8 __pad0[7];
> >         union {
> >                 struct {
> >                         __s32 nr;
> 
> __u64 please.  Syscall numbers are, as a practical matter, 64 bits.
> Admittedly, the actual effects of setting the high bits are unclear,
> and seccomp has issues with it, but let's not perpetuate the problem.

I agree.  Although the implementation uses syscall_get_nr()
which returns int, this could potentially be fixed in the future.

> >                         __u32 arch;
> >                         __u64 instruction_pointer;
> >                         __u64 args[6];
> >                 } entry_info;
> >                 struct {
> >                         __s64 rval;
> >                         __u8 is_error;
> >                         __u8 __pad1[7];
> >                 } exit_info;
> >         };
> > };
> 
> Should seccomp events use entry_info or should they just literally
> supply seccomp_data?

It certainly can use entry_info.
I'd prefer to avoid using in uapi/linux/ptrace.h those types
that are defined in uapi/linux/seccomp.h.


-- 
ldv

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

Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request

[Andy Lutomirski]

On Wed, Nov 21, 2018 at 3:56 PM Dmitry V. Levin <ldv@altlinux.org> wrote:
>
> On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> > Please cc linux-api@vger.kernel.org for future versions.
> >
> > On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova wrote:
> > >
> > > struct ptrace_syscall_info {
> > >         __u8 op; /* 0 for entry, 1 for exit */
> >
> > Can you add proper defines, like:
> >
> > #define PTRACE_SYSCALL_ENTRY 0
> > #define PTRACE_SYSCALL_EXIT 1
> > #define PTRACE_SYSCALL_SECCOMP 2
> >
> > and make seccomp work from the start?  I'd rather we don't merge an
> > implementation that doesn't work for seccomp and then have to rework
> > it later.
>
> What's the difference between PTRACE_EVENT_SECCOMP and syscall-entry-stop
> with regards to PTRACE_GET_SYSCALL_INFO request?  At least they have the
> same entry_info to return.

I'm not sure there's any material difference.

>
> As long as implementation (ab)uses ptrace_message to tell one kind of stop
> from another, it can distinguish syscall-entry-stop and syscall-exit-stop
> from each other and from many other kinds of stops, but it cannot
> distinguish PTRACE_EVENT_SECCOMP from e.g. PTRACE_EVENT_EXIT.

Hmm.  PTRACE_GET_SYSCALL_INFO should fail for PTRACE_EVENT_EXIT, I think.

>
> > >         __u8 __pad0[7];
> > >         union {
> > >                 struct {
> > >                         __s32 nr;
> >
> > __u64 please.  Syscall numbers are, as a practical matter, 64 bits.
> > Admittedly, the actual effects of setting the high bits are unclear,
> > and seccomp has issues with it, but let's not perpetuate the problem.
>
> I agree.  Although the implementation uses syscall_get_nr()
> which returns int, this could potentially be fixed in the future.

Agreed.  Although if we ever start using those high bits, things will
get confusing.

>
> > >                         __u32 arch;
> > >                         __u64 instruction_pointer;
> > >                         __u64 args[6];
> > >                 } entry_info;
> > >                 struct {
> > >                         __s64 rval;
> > >                         __u8 is_error;
> > >                         __u8 __pad1[7];
> > >                 } exit_info;
> > >         };
> > > };
> >
> > Should seccomp events use entry_info or should they just literally
> > supply seccomp_data?
>
> It certainly can use entry_info.
> I'd prefer to avoid using in uapi/linux/ptrace.h those types
> that are defined in uapi/linux/seccomp.h.

Makes sense to me.  Also, it's possible in principle to extend
seccomp_data with other fields that are only generated if they're
read, so passing struct seccomp_data to userspace as a struct may be
the wrong thing to do.

Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request

[Dmitry V. Levin]

[-- Attachment #1.1: Type: text/plain, Size: 1928 bytes --]

On Thu, Nov 22, 2018 at 06:55:29AM -0800, Andy Lutomirski wrote:
> On Wed, Nov 21, 2018 at 3:56 PM Dmitry V. Levin wrote:
> > On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> > > Please cc linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org for future versions.
> > >
> > > On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova wrote:
> > > >
> > > > struct ptrace_syscall_info {
> > > >         __u8 op; /* 0 for entry, 1 for exit */
> > >
> > > Can you add proper defines, like:
> > >
> > > #define PTRACE_SYSCALL_ENTRY 0
> > > #define PTRACE_SYSCALL_EXIT 1
> > > #define PTRACE_SYSCALL_SECCOMP 2
> > >
> > > and make seccomp work from the start?  I'd rather we don't merge an
> > > implementation that doesn't work for seccomp and then have to rework
> > > it later.
> >
> > What's the difference between PTRACE_EVENT_SECCOMP and syscall-entry-stop
> > with regards to PTRACE_GET_SYSCALL_INFO request?  At least they have the
> > same entry_info to return.
> 
> I'm not sure there's any material difference.

In that case we don't really need PTRACE_SYSCALL_SECCOMP: op field
describes the structure inside the union to use, not the ptrace stop.

> > As long as implementation (ab)uses ptrace_message to tell one kind of stop
> > from another, it can distinguish syscall-entry-stop and syscall-exit-stop
> > from each other and from many other kinds of stops, but it cannot
> > distinguish PTRACE_EVENT_SECCOMP from e.g. PTRACE_EVENT_EXIT.
> 
> Hmm.  PTRACE_GET_SYSCALL_INFO should fail for PTRACE_EVENT_EXIT, I think.

Unless we can change PTRACE_EVENT_SECCOMP to set some higher bits of
ptrace_message (beyond SECCOMP_RET_DATA) which is very unlikely because
it would qualify as an ABI change, this would require an additional field
in struct task_struct because ptrace_message wouldn't be enough
to distinguish PTRACE_EVENT_SECCOMP from PTRACE_EVENT_EXIT.


-- 
ldv

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

[-- Attachment #2: Type: text/plain, Size: 137 bytes --]

-- 
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel

Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request

[Dmitry V. Levin]

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

On Thu, Nov 22, 2018 at 06:55:29AM -0800, Andy Lutomirski wrote:
> On Wed, Nov 21, 2018 at 3:56 PM Dmitry V. Levin wrote:
> > On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> > > Please cc linux-api@vger.kernel.org for future versions.
> > >
> > > On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova wrote:
> > > >
> > > > struct ptrace_syscall_info {
> > > >         __u8 op; /* 0 for entry, 1 for exit */
> > >
> > > Can you add proper defines, like:
> > >
> > > #define PTRACE_SYSCALL_ENTRY 0
> > > #define PTRACE_SYSCALL_EXIT 1
> > > #define PTRACE_SYSCALL_SECCOMP 2
> > >
> > > and make seccomp work from the start?  I'd rather we don't merge an
> > > implementation that doesn't work for seccomp and then have to rework
> > > it later.
> >
> > What's the difference between PTRACE_EVENT_SECCOMP and syscall-entry-stop
> > with regards to PTRACE_GET_SYSCALL_INFO request?  At least they have the
> > same entry_info to return.
> 
> I'm not sure there's any material difference.

In that case we don't really need PTRACE_SYSCALL_SECCOMP: op field
describes the structure inside the union to use, not the ptrace stop.

> > As long as implementation (ab)uses ptrace_message to tell one kind of stop
> > from another, it can distinguish syscall-entry-stop and syscall-exit-stop
> > from each other and from many other kinds of stops, but it cannot
> > distinguish PTRACE_EVENT_SECCOMP from e.g. PTRACE_EVENT_EXIT.
> 
> Hmm.  PTRACE_GET_SYSCALL_INFO should fail for PTRACE_EVENT_EXIT, I think.

Unless we can change PTRACE_EVENT_SECCOMP to set some higher bits of
ptrace_message (beyond SECCOMP_RET_DATA) which is very unlikely because
it would qualify as an ABI change, this would require an additional field
in struct task_struct because ptrace_message wouldn't be enough
to distinguish PTRACE_EVENT_SECCOMP from PTRACE_EVENT_EXIT.


-- 
ldv

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

Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request

[Andy Lutomirski]

On Thu, Nov 22, 2018 at 11:15 AM Dmitry V. Levin <ldv-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org> wrote:
>
> On Thu, Nov 22, 2018 at 06:55:29AM -0800, Andy Lutomirski wrote:
> > On Wed, Nov 21, 2018 at 3:56 PM Dmitry V. Levin wrote:
> > > On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> > > > Please cc linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org for future versions.
> > > >
> > > > On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova wrote:
> > > > >
> > > > > struct ptrace_syscall_info {
> > > > >         __u8 op; /* 0 for entry, 1 for exit */
> > > >
> > > > Can you add proper defines, like:
> > > >
> > > > #define PTRACE_SYSCALL_ENTRY 0
> > > > #define PTRACE_SYSCALL_EXIT 1
> > > > #define PTRACE_SYSCALL_SECCOMP 2
> > > >
> > > > and make seccomp work from the start?  I'd rather we don't merge an
> > > > implementation that doesn't work for seccomp and then have to rework
> > > > it later.
> > >
> > > What's the difference between PTRACE_EVENT_SECCOMP and syscall-entry-stop
> > > with regards to PTRACE_GET_SYSCALL_INFO request?  At least they have the
> > > same entry_info to return.
> >
> > I'm not sure there's any material difference.
>
> In that case we don't really need PTRACE_SYSCALL_SECCOMP: op field
> describes the structure inside the union to use, not the ptrace stop.

Unless we think the structures might diverge in the future.

>
> > > As long as implementation (ab)uses ptrace_message to tell one kind of stop
> > > from another, it can distinguish syscall-entry-stop and syscall-exit-stop
> > > from each other and from many other kinds of stops, but it cannot
> > > distinguish PTRACE_EVENT_SECCOMP from e.g. PTRACE_EVENT_EXIT.
> >
> > Hmm.  PTRACE_GET_SYSCALL_INFO should fail for PTRACE_EVENT_EXIT, I think.
>
> Unless we can change PTRACE_EVENT_SECCOMP to set some higher bits of
> ptrace_message (beyond SECCOMP_RET_DATA) which is very unlikely because
> it would qualify as an ABI change, this would require an additional field
> in struct task_struct because ptrace_message wouldn't be enough
> to distinguish PTRACE_EVENT_SECCOMP from PTRACE_EVENT_EXIT.

At the risk of making the patch more complicated, there's room to
massively clean up the ptrace state.  We could add a struct
ptrace_tracee and put a struct ptrace_tracee *ptrace_tracee into
task_struct.  The struct would contain a pointer to the task_struct as
well as ptrace (the flag field, I think), ptrace_entry, ptracer_cred,
ptrace_message, and last_siginfo.  And then we could add a field for
the ptrace stop state that would indicate the actual reason for the
current stop.  We'd only allocate ptrace_tracee when someone attaches
with ptrace, thus saving quite a few bytes for each task.

It's a bit unfortunate if we allow PTRACE_GET_SYSCALL_INFO to success
if the event is PTRACE_EVENT_EXIT.  I'd also be a bit nervous about
info leaks if we start calling the syscall accessors for tasks that
aren't in syscalls.

--Andy
-- 
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel

Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request

[Andy Lutomirski]

On Thu, Nov 22, 2018 at 11:15 AM Dmitry V. Levin <ldv@altlinux.org> wrote:
>
> On Thu, Nov 22, 2018 at 06:55:29AM -0800, Andy Lutomirski wrote:
> > On Wed, Nov 21, 2018 at 3:56 PM Dmitry V. Levin wrote:
> > > On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> > > > Please cc linux-api@vger.kernel.org for future versions.
> > > >
> > > > On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova wrote:
> > > > >
> > > > > struct ptrace_syscall_info {
> > > > >         __u8 op; /* 0 for entry, 1 for exit */
> > > >
> > > > Can you add proper defines, like:
> > > >
> > > > #define PTRACE_SYSCALL_ENTRY 0
> > > > #define PTRACE_SYSCALL_EXIT 1
> > > > #define PTRACE_SYSCALL_SECCOMP 2
> > > >
> > > > and make seccomp work from the start?  I'd rather we don't merge an
> > > > implementation that doesn't work for seccomp and then have to rework
> > > > it later.
> > >
> > > What's the difference between PTRACE_EVENT_SECCOMP and syscall-entry-stop
> > > with regards to PTRACE_GET_SYSCALL_INFO request?  At least they have the
> > > same entry_info to return.
> >
> > I'm not sure there's any material difference.
>
> In that case we don't really need PTRACE_SYSCALL_SECCOMP: op field
> describes the structure inside the union to use, not the ptrace stop.

Unless we think the structures might diverge in the future.

>
> > > As long as implementation (ab)uses ptrace_message to tell one kind of stop
> > > from another, it can distinguish syscall-entry-stop and syscall-exit-stop
> > > from each other and from many other kinds of stops, but it cannot
> > > distinguish PTRACE_EVENT_SECCOMP from e.g. PTRACE_EVENT_EXIT.
> >
> > Hmm.  PTRACE_GET_SYSCALL_INFO should fail for PTRACE_EVENT_EXIT, I think.
>
> Unless we can change PTRACE_EVENT_SECCOMP to set some higher bits of
> ptrace_message (beyond SECCOMP_RET_DATA) which is very unlikely because
> it would qualify as an ABI change, this would require an additional field
> in struct task_struct because ptrace_message wouldn't be enough
> to distinguish PTRACE_EVENT_SECCOMP from PTRACE_EVENT_EXIT.

At the risk of making the patch more complicated, there's room to
massively clean up the ptrace state.  We could add a struct
ptrace_tracee and put a struct ptrace_tracee *ptrace_tracee into
task_struct.  The struct would contain a pointer to the task_struct as
well as ptrace (the flag field, I think), ptrace_entry, ptracer_cred,
ptrace_message, and last_siginfo.  And then we could add a field for
the ptrace stop state that would indicate the actual reason for the
current stop.  We'd only allocate ptrace_tracee when someone attaches
with ptrace, thus saving quite a few bytes for each task.

It's a bit unfortunate if we allow PTRACE_GET_SYSCALL_INFO to success
if the event is PTRACE_EVENT_EXIT.  I'd also be a bit nervous about
info leaks if we start calling the syscall accessors for tasks that
aren't in syscalls.

--Andy

Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request

[Dmitry V. Levin]

[-- Attachment #1.1: Type: text/plain, Size: 2300 bytes --]

On Thu, Nov 22, 2018 at 04:19:10PM -0800, Andy Lutomirski wrote:
> On Thu, Nov 22, 2018 at 11:15 AM Dmitry V. Levin <ldv-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org> wrote:
> >
> > On Thu, Nov 22, 2018 at 06:55:29AM -0800, Andy Lutomirski wrote:
> > > On Wed, Nov 21, 2018 at 3:56 PM Dmitry V. Levin wrote:
> > > > On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> > > > > Please cc linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org for future versions.
> > > > >
> > > > > On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova wrote:
> > > > > >
> > > > > > struct ptrace_syscall_info {
> > > > > >         __u8 op; /* 0 for entry, 1 for exit */
> > > > >
> > > > > Can you add proper defines, like:
> > > > >
> > > > > #define PTRACE_SYSCALL_ENTRY 0
> > > > > #define PTRACE_SYSCALL_EXIT 1
> > > > > #define PTRACE_SYSCALL_SECCOMP 2
> > > > >
> > > > > and make seccomp work from the start?  I'd rather we don't merge an
> > > > > implementation that doesn't work for seccomp and then have to rework
> > > > > it later.
> > > >
> > > > What's the difference between PTRACE_EVENT_SECCOMP and syscall-entry-stop
> > > > with regards to PTRACE_GET_SYSCALL_INFO request?  At least they have the
> > > > same entry_info to return.
> > >
> > > I'm not sure there's any material difference.
> >
> > In that case we don't really need PTRACE_SYSCALL_SECCOMP: op field
> > describes the structure inside the union to use, not the ptrace stop.
> 
> Unless we think the structures might diverge in the future.

If these structures ever diverge, then a seccomp structure will be added
to the union, and a portable userspace code will likely look this way:

#include <linux/ptrace.h>
...
struct ptrace_syscall_info info;
long rc = ptrace(PTRACE_GET_SYSCALL_INFO, pid, (void *) sizeof(info), &info);
...
switch (info.op) {
	case PTRACE_SYSCALL_INFO_ENTRY:
		/* handle info.entry */
	case PTRACE_SYSCALL_INFO_EXIT:
		/* handle info.exit */
#ifdef PTRACE_SYSCALL_INFO_SECCOMP
	case PTRACE_SYSCALL_INFO_SECCOMP:
		/* handle info.seccomp */
#endif
	default:
		/* handle unknown info.op */
}

In other words, it would be better if PTRACE_SYSCALL_INFO_* selector
constants were introduced along with corresponding structures in the
union.


-- 
ldv

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

[-- Attachment #2: Type: text/plain, Size: 137 bytes --]

-- 
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel

Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request

[Dmitry V. Levin]

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

On Thu, Nov 22, 2018 at 04:19:10PM -0800, Andy Lutomirski wrote:
> On Thu, Nov 22, 2018 at 11:15 AM Dmitry V. Levin <ldv@altlinux.org> wrote:
> >
> > On Thu, Nov 22, 2018 at 06:55:29AM -0800, Andy Lutomirski wrote:
> > > On Wed, Nov 21, 2018 at 3:56 PM Dmitry V. Levin wrote:
> > > > On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> > > > > Please cc linux-api@vger.kernel.org for future versions.
> > > > >
> > > > > On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova wrote:
> > > > > >
> > > > > > struct ptrace_syscall_info {
> > > > > >         __u8 op; /* 0 for entry, 1 for exit */
> > > > >
> > > > > Can you add proper defines, like:
> > > > >
> > > > > #define PTRACE_SYSCALL_ENTRY 0
> > > > > #define PTRACE_SYSCALL_EXIT 1
> > > > > #define PTRACE_SYSCALL_SECCOMP 2
> > > > >
> > > > > and make seccomp work from the start?  I'd rather we don't merge an
> > > > > implementation that doesn't work for seccomp and then have to rework
> > > > > it later.
> > > >
> > > > What's the difference between PTRACE_EVENT_SECCOMP and syscall-entry-stop
> > > > with regards to PTRACE_GET_SYSCALL_INFO request?  At least they have the
> > > > same entry_info to return.
> > >
> > > I'm not sure there's any material difference.
> >
> > In that case we don't really need PTRACE_SYSCALL_SECCOMP: op field
> > describes the structure inside the union to use, not the ptrace stop.
> 
> Unless we think the structures might diverge in the future.

If these structures ever diverge, then a seccomp structure will be added
to the union, and a portable userspace code will likely look this way:

#include <linux/ptrace.h>
...
struct ptrace_syscall_info info;
long rc = ptrace(PTRACE_GET_SYSCALL_INFO, pid, (void *) sizeof(info), &info);
...
switch (info.op) {
	case PTRACE_SYSCALL_INFO_ENTRY:
		/* handle info.entry */
	case PTRACE_SYSCALL_INFO_EXIT:
		/* handle info.exit */
#ifdef PTRACE_SYSCALL_INFO_SECCOMP
	case PTRACE_SYSCALL_INFO_SECCOMP:
		/* handle info.seccomp */
#endif
	default:
		/* handle unknown info.op */
}

In other words, it would be better if PTRACE_SYSCALL_INFO_* selector
constants were introduced along with corresponding structures in the
union.


-- 
ldv

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

Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request

[Dmitry V. Levin]

[-- Attachment #1.1: Type: text/plain, Size: 3266 bytes --]

On Fri, Nov 23, 2018 at 07:01:39AM +0300, Dmitry V. Levin wrote:
> On Thu, Nov 22, 2018 at 04:19:10PM -0800, Andy Lutomirski wrote:
> > On Thu, Nov 22, 2018 at 11:15 AM Dmitry V. Levin wrote:
> > > On Thu, Nov 22, 2018 at 06:55:29AM -0800, Andy Lutomirski wrote:
> > > > On Wed, Nov 21, 2018 at 3:56 PM Dmitry V. Levin wrote:
> > > > > On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> > > > > > Please cc linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org for future versions.
> > > > > >
> > > > > > On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova wrote:
> > > > > > >
> > > > > > > struct ptrace_syscall_info {
> > > > > > >         __u8 op; /* 0 for entry, 1 for exit */
> > > > > >
> > > > > > Can you add proper defines, like:
> > > > > >
> > > > > > #define PTRACE_SYSCALL_ENTRY 0
> > > > > > #define PTRACE_SYSCALL_EXIT 1
> > > > > > #define PTRACE_SYSCALL_SECCOMP 2
> > > > > >
> > > > > > and make seccomp work from the start?  I'd rather we don't merge an
> > > > > > implementation that doesn't work for seccomp and then have to rework
> > > > > > it later.
> > > > >
> > > > > What's the difference between PTRACE_EVENT_SECCOMP and syscall-entry-stop
> > > > > with regards to PTRACE_GET_SYSCALL_INFO request?  At least they have the
> > > > > same entry_info to return.
> > > >
> > > > I'm not sure there's any material difference.
> > >
> > > In that case we don't really need PTRACE_SYSCALL_SECCOMP: op field
> > > describes the structure inside the union to use, not the ptrace stop.
> > 
> > Unless we think the structures might diverge in the future.
> 
> If these structures ever diverge, then a seccomp structure will be added
> to the union, and a portable userspace code will likely look this way:
> 
> #include <linux/ptrace.h>
> ...
> struct ptrace_syscall_info info;
> long rc = ptrace(PTRACE_GET_SYSCALL_INFO, pid, (void *) sizeof(info), &info);
> ...
> switch (info.op) {
> 	case PTRACE_SYSCALL_INFO_ENTRY:
> 		/* handle info.entry */
> 	case PTRACE_SYSCALL_INFO_EXIT:
> 		/* handle info.exit */
> #ifdef PTRACE_SYSCALL_INFO_SECCOMP
> 	case PTRACE_SYSCALL_INFO_SECCOMP:
> 		/* handle info.seccomp */
> #endif
> 	default:
> 		/* handle unknown info.op */
> }
> 
> In other words, it would be better if PTRACE_SYSCALL_INFO_* selector
> constants were introduced along with corresponding structures in the
> union.

However, the approach I suggested doesn't provide forward compatibility:
if userspace is compiled with kernel headers that don't define
PTRACE_SYSCALL_INFO_SECCOMP, it will break when the kernel
starts to use PTRACE_SYSCALL_INFO_SECCOMP instead of
PTRACE_SYSCALL_INFO_ENTRY for PTRACE_EVENT_SECCOMP support
in PTRACE_GET_SYSCALL_INFO.

The solution is to introduce PTRACE_SYSCALL_INFO_SECCOMP and struct
ptrace_syscall_info.seccomp along with PTRACE_EVENT_SECCOMP support
in PTRACE_GET_SYSCALL_INFO.  The initial revision of the seccomp
structure could be made the same as the entry structure, or it can
diverge from the beginning, e.g., by adding ret_data field containing
SECCOMP_RET_DATA return value stored in ptrace_message, this would save
ptracers an extra PTRACE_GETEVENTMSG call currently required to obtain it.


-- 
ldv

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

[-- Attachment #2: Type: text/plain, Size: 137 bytes --]

-- 
Strace-devel mailing list
Strace-devel-3+4lAyCyj6AWlMsSdNXQLw@public.gmane.org
https://lists.strace.io/mailman/listinfo/strace-devel

Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request

[Dmitry V. Levin]

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

On Fri, Nov 23, 2018 at 07:01:39AM +0300, Dmitry V. Levin wrote:
> On Thu, Nov 22, 2018 at 04:19:10PM -0800, Andy Lutomirski wrote:
> > On Thu, Nov 22, 2018 at 11:15 AM Dmitry V. Levin wrote:
> > > On Thu, Nov 22, 2018 at 06:55:29AM -0800, Andy Lutomirski wrote:
> > > > On Wed, Nov 21, 2018 at 3:56 PM Dmitry V. Levin wrote:
> > > > > On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> > > > > > Please cc linux-api@vger.kernel.org for future versions.
> > > > > >
> > > > > > On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova wrote:
> > > > > > >
> > > > > > > struct ptrace_syscall_info {
> > > > > > >         __u8 op; /* 0 for entry, 1 for exit */
> > > > > >
> > > > > > Can you add proper defines, like:
> > > > > >
> > > > > > #define PTRACE_SYSCALL_ENTRY 0
> > > > > > #define PTRACE_SYSCALL_EXIT 1
> > > > > > #define PTRACE_SYSCALL_SECCOMP 2
> > > > > >
> > > > > > and make seccomp work from the start?  I'd rather we don't merge an
> > > > > > implementation that doesn't work for seccomp and then have to rework
> > > > > > it later.
> > > > >
> > > > > What's the difference between PTRACE_EVENT_SECCOMP and syscall-entry-stop
> > > > > with regards to PTRACE_GET_SYSCALL_INFO request?  At least they have the
> > > > > same entry_info to return.
> > > >
> > > > I'm not sure there's any material difference.
> > >
> > > In that case we don't really need PTRACE_SYSCALL_SECCOMP: op field
> > > describes the structure inside the union to use, not the ptrace stop.
> > 
> > Unless we think the structures might diverge in the future.
> 
> If these structures ever diverge, then a seccomp structure will be added
> to the union, and a portable userspace code will likely look this way:
> 
> #include <linux/ptrace.h>
> ...
> struct ptrace_syscall_info info;
> long rc = ptrace(PTRACE_GET_SYSCALL_INFO, pid, (void *) sizeof(info), &info);
> ...
> switch (info.op) {
> 	case PTRACE_SYSCALL_INFO_ENTRY:
> 		/* handle info.entry */
> 	case PTRACE_SYSCALL_INFO_EXIT:
> 		/* handle info.exit */
> #ifdef PTRACE_SYSCALL_INFO_SECCOMP
> 	case PTRACE_SYSCALL_INFO_SECCOMP:
> 		/* handle info.seccomp */
> #endif
> 	default:
> 		/* handle unknown info.op */
> }
> 
> In other words, it would be better if PTRACE_SYSCALL_INFO_* selector
> constants were introduced along with corresponding structures in the
> union.

However, the approach I suggested doesn't provide forward compatibility:
if userspace is compiled with kernel headers that don't define
PTRACE_SYSCALL_INFO_SECCOMP, it will break when the kernel
starts to use PTRACE_SYSCALL_INFO_SECCOMP instead of
PTRACE_SYSCALL_INFO_ENTRY for PTRACE_EVENT_SECCOMP support
in PTRACE_GET_SYSCALL_INFO.

The solution is to introduce PTRACE_SYSCALL_INFO_SECCOMP and struct
ptrace_syscall_info.seccomp along with PTRACE_EVENT_SECCOMP support
in PTRACE_GET_SYSCALL_INFO.  The initial revision of the seccomp
structure could be made the same as the entry structure, or it can
diverge from the beginning, e.g., by adding ret_data field containing
SECCOMP_RET_DATA return value stored in ptrace_message, this would save
ptracers an extra PTRACE_GETEVENTMSG call currently required to obtain it.


-- 
ldv

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

Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request

[Kees Cook]

On Sat, Nov 24, 2018 at 8:10 PM, Dmitry V. Levin <ldv@altlinux.org> wrote:
> On Fri, Nov 23, 2018 at 07:01:39AM +0300, Dmitry V. Levin wrote:
>> On Thu, Nov 22, 2018 at 04:19:10PM -0800, Andy Lutomirski wrote:
>> > On Thu, Nov 22, 2018 at 11:15 AM Dmitry V. Levin wrote:
>> > > On Thu, Nov 22, 2018 at 06:55:29AM -0800, Andy Lutomirski wrote:
>> > > > On Wed, Nov 21, 2018 at 3:56 PM Dmitry V. Levin wrote:
>> > > > > On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
>> > > > > > Please cc linux-api@vger.kernel.org for future versions.
>> > > > > >
>> > > > > > On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova wrote:
>> > > > > > >
>> > > > > > > struct ptrace_syscall_info {
>> > > > > > >         __u8 op; /* 0 for entry, 1 for exit */
>> > > > > >
>> > > > > > Can you add proper defines, like:
>> > > > > >
>> > > > > > #define PTRACE_SYSCALL_ENTRY 0
>> > > > > > #define PTRACE_SYSCALL_EXIT 1
>> > > > > > #define PTRACE_SYSCALL_SECCOMP 2
>> > > > > >
>> > > > > > and make seccomp work from the start?  I'd rather we don't merge an
>> > > > > > implementation that doesn't work for seccomp and then have to rework
>> > > > > > it later.

Yes, please.

>> > > > >
>> > > > > What's the difference between PTRACE_EVENT_SECCOMP and syscall-entry-stop
>> > > > > with regards to PTRACE_GET_SYSCALL_INFO request?  At least they have the
>> > > > > same entry_info to return.
>> > > >
>> > > > I'm not sure there's any material difference.
>> > >
>> > > In that case we don't really need PTRACE_SYSCALL_SECCOMP: op field
>> > > describes the structure inside the union to use, not the ptrace stop.
>> >
>> > Unless we think the structures might diverge in the future.

Yes, I want to make sure we have a way to expand this, especially for
seccomp: we've come close a few times to adding new fields to struct
seccomp_data, for example.

>>
>> If these structures ever diverge, then a seccomp structure will be added
>> to the union, and a portable userspace code will likely look this way:
>>
>> #include <linux/ptrace.h>
>> ...
>> struct ptrace_syscall_info info;
>> long rc = ptrace(PTRACE_GET_SYSCALL_INFO, pid, (void *) sizeof(info), &info);
>> ...
>> switch (info.op) {
>>       case PTRACE_SYSCALL_INFO_ENTRY:
>>               /* handle info.entry */
>>       case PTRACE_SYSCALL_INFO_EXIT:
>>               /* handle info.exit */
>> #ifdef PTRACE_SYSCALL_INFO_SECCOMP
>>       case PTRACE_SYSCALL_INFO_SECCOMP:
>>               /* handle info.seccomp */
>> #endif
>>       default:
>>               /* handle unknown info.op */
>> }
>>
>> In other words, it would be better if PTRACE_SYSCALL_INFO_* selector
>> constants were introduced along with corresponding structures in the
>> union.
>
> However, the approach I suggested doesn't provide forward compatibility:
> if userspace is compiled with kernel headers that don't define
> PTRACE_SYSCALL_INFO_SECCOMP, it will break when the kernel
> starts to use PTRACE_SYSCALL_INFO_SECCOMP instead of
> PTRACE_SYSCALL_INFO_ENTRY for PTRACE_EVENT_SECCOMP support
> in PTRACE_GET_SYSCALL_INFO.
>
> The solution is to introduce PTRACE_SYSCALL_INFO_SECCOMP and struct
> ptrace_syscall_info.seccomp along with PTRACE_EVENT_SECCOMP support
> in PTRACE_GET_SYSCALL_INFO.  The initial revision of the seccomp
> structure could be made the same as the entry structure, or it can
> diverge from the beginning, e.g., by adding ret_data field containing
> SECCOMP_RET_DATA return value stored in ptrace_message, this would save
> ptracers an extra PTRACE_GETEVENTMSG call currently required to obtain it.

Yup, that'd be a nice addition.

-- 
Kees Cook