linux-api.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request
       [not found] <20181121165806.07da7c98@akathisia>
@ 2018-11-21 22:56 ` Andy Lutomirski
       [not found]   ` <CALCETrV3TPCkYyhoLLcikXVeF-RdZUuLTCvKReK3Qb9LSS9Csw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 8+ messages in thread
From: Andy Lutomirski @ 2018-11-21 22:56 UTC (permalink / raw)
  To: Elvira Khabirova, Kees Cook, Sasha Levin, Linux API, Jann Horn
  Cc: Oleg Nesterov, Steven Rostedt, Ingo Molnar, LKML, Dmitry V. Levin,
	Eugene Syromiatnikov, Andrew Lutomirski, strace-devel

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?

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

* Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request
       [not found]   ` <CALCETrV3TPCkYyhoLLcikXVeF-RdZUuLTCvKReK3Qb9LSS9Csw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2018-11-21 23:56     ` Dmitry V. Levin
  2018-11-22 14:55       ` Andy Lutomirski
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitry V. Levin @ 2018-11-21 23:56 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Eugene Syromiatnikov, Kees Cook, Jann Horn, Linux API,
	Oleg Nesterov, Steven Rostedt, LKML, Sasha Levin, Ingo Molnar,
	strace-devel-3+4lAyCyj6AWlMsSdNXQLw


[-- 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

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

* Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request
  2018-11-21 23:56     ` Dmitry V. Levin
@ 2018-11-22 14:55       ` Andy Lutomirski
       [not found]         ` <CALCETrXea_uRAqw_srW5CWgOzeM=RubaDbjnxZ=cUMy5Zv1TsA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 8+ messages in thread
From: Andy Lutomirski @ 2018-11-22 14:55 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Andrew Lutomirski, Elvira Khabirova, Kees Cook, Sasha Levin,
	Linux API, Jann Horn, Oleg Nesterov, Steven Rostedt, Ingo Molnar,
	LKML, Eugene Syromiatnikov, strace-devel

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.

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

* Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request
       [not found]         ` <CALCETrXea_uRAqw_srW5CWgOzeM=RubaDbjnxZ=cUMy5Zv1TsA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2018-11-22 19:15           ` Dmitry V. Levin
       [not found]             ` <20181122191504.GB27204-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitry V. Levin @ 2018-11-22 19:15 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Eugene Syromiatnikov, Kees Cook, Jann Horn, Linux API,
	Oleg Nesterov, Steven Rostedt, LKML, Ingo Molnar,
	strace-devel-3+4lAyCyj6AWlMsSdNXQLw


[-- 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

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

* Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request
       [not found]             ` <20181122191504.GB27204-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
@ 2018-11-23  0:19               ` Andy Lutomirski
       [not found]                 ` <CALCETrXvXzRzoEqwEY_VZ7Vpt-sLwaF+rZPg+y_eG2xyzubXtw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 8+ messages in thread
From: Andy Lutomirski @ 2018-11-23  0:19 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Eugene Syromiatnikov, Kees Cook, Jann Horn, Linux API,
	Oleg Nesterov, Steven Rostedt, LKML, Andrew Lutomirski,
	Ingo Molnar, strace-devel-3+4lAyCyj6AWlMsSdNXQLw

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

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

* Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request
       [not found]                 ` <CALCETrXvXzRzoEqwEY_VZ7Vpt-sLwaF+rZPg+y_eG2xyzubXtw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2018-11-23  4:01                   ` Dmitry V. Levin
       [not found]                     ` <20181123040139.GB2572-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitry V. Levin @ 2018-11-23  4:01 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Eugene Syromiatnikov, Kees Cook, Jann Horn, Linux API,
	Oleg Nesterov, Steven Rostedt, LKML, Ingo Molnar,
	strace-devel-3+4lAyCyj6AWlMsSdNXQLw


[-- 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

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

* Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request
       [not found]                     ` <20181123040139.GB2572-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
@ 2018-11-25  4:10                       ` Dmitry V. Levin
  2018-11-27 22:28                         ` Kees Cook
  0 siblings, 1 reply; 8+ messages in thread
From: Dmitry V. Levin @ 2018-11-25  4:10 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Eugene Syromiatnikov, Kees Cook, Jann Horn, Linux API,
	Oleg Nesterov, Steven Rostedt, LKML, Ingo Molnar,
	strace-devel-3+4lAyCyj6AWlMsSdNXQLw


[-- 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

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

* Re: [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request
  2018-11-25  4:10                       ` Dmitry V. Levin
@ 2018-11-27 22:28                         ` Kees Cook
  0 siblings, 0 replies; 8+ messages in thread
From: Kees Cook @ 2018-11-27 22:28 UTC (permalink / raw)
  To: Dmitry V. Levin
  Cc: Andy Lutomirski, Elvira Khabirova, Linux API, Jann Horn,
	Oleg Nesterov, Steven Rostedt, Ingo Molnar, LKML,
	Eugene Syromiatnikov, strace-devel

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

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

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

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20181121165806.07da7c98@akathisia>
2018-11-21 22:56 ` [RFC PATCH v2] ptrace: add PTRACE_GET_SYSCALL_INFO request Andy Lutomirski
     [not found]   ` <CALCETrV3TPCkYyhoLLcikXVeF-RdZUuLTCvKReK3Qb9LSS9Csw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-11-21 23:56     ` Dmitry V. Levin
2018-11-22 14:55       ` Andy Lutomirski
     [not found]         ` <CALCETrXea_uRAqw_srW5CWgOzeM=RubaDbjnxZ=cUMy5Zv1TsA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-11-22 19:15           ` Dmitry V. Levin
     [not found]             ` <20181122191504.GB27204-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
2018-11-23  0:19               ` Andy Lutomirski
     [not found]                 ` <CALCETrXvXzRzoEqwEY_VZ7Vpt-sLwaF+rZPg+y_eG2xyzubXtw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-11-23  4:01                   ` Dmitry V. Levin
     [not found]                     ` <20181123040139.GB2572-u2l5PoMzF/Vg9hUCZPvPmw@public.gmane.org>
2018-11-25  4:10                       ` Dmitry V. Levin
2018-11-27 22:28                         ` Kees Cook

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).