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/[email protected]/
[4] https://lore.kernel.org/lkml/[email protected]/
Co-authored-by: Dmitry V. Levin <[email protected]>
Signed-off-by: Elvira Khabirova <[email protected]>
Signed-off-by: Dmitry V. Levin <[email protected]>
---
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
Please cc [email protected] for future versions.
On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova
<[email protected]> 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?
On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> Please cc [email protected] 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
On Wed, Nov 21, 2018 at 3:56 PM Dmitry V. Levin <[email protected]> wrote:
>
> On Wed, Nov 21, 2018 at 02:56:57PM -0800, Andy Lutomirski wrote:
> > Please cc [email protected] 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.
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 [email protected] 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
On Thu, Nov 22, 2018 at 11:15 AM Dmitry V. Levin <[email protected]> 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 [email protected] 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
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 <[email protected]> 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 [email protected] 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
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 [email protected] 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
On Sat, Nov 24, 2018 at 8:10 PM, Dmitry V. Levin <[email protected]> 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 [email protected] 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