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

quoted

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

On Wed, Nov 21, 2018 at 7:58 AM Elvira Khabirova
[off-list ref] 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 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
On Wed, Nov 21, 2018 at 3:56 PM Dmitry V. Levin [off-list ref] 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.
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
On Thu, Nov 22, 2018 at 11:15 AM Dmitry V. Levin [off-list ref] 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
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 [off-list ref] 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
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
On Sat, Nov 24, 2018 at 8:10 PM, Dmitry V. Levin [off-list ref] 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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help