linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH linux-next v10 0/7] ptrace: add PTRACE_GET_SYSCALL_INFO request
@ 2019-04-15 23:43 Dmitry V. Levin
  2019-04-15 23:44 ` [PATCH linux-next v10 5/7] powerpc: define syscall_get_error() Dmitry V. Levin
  2019-05-09 16:14 ` [PATCH linux-next v10 0/7] ptrace: add PTRACE_GET_SYSCALL_INFO request Oleg Nesterov
  0 siblings, 2 replies; 4+ messages in thread
From: Dmitry V. Levin @ 2019-04-15 23:43 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Oleg Nesterov, Paul Mackerras, linux-kselftest, Vincent Chen,
	Shuah Khan, Helge Deller, Eugene Syromyatnikov, Elvira Khabirova,
	James Hogan, James E.J. Bottomley, strace-devel, Kees Cook,
	Greentime Hu, linux-kernel, Andy Lutomirski, linux-parisc,
	linux-api, linux-mips, Ralf Baechle, Richard Kuo, Paul Burton,
	linux-hexagon, linuxppc-dev

[Andrew, could you take this patchset into your tree, please?]

PTRACE_GET_SYSCALL_INFO is a generic ptrace API that lets ptracer obtain
details of the syscall the tracee is blocked in.

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 for the tracer.

Secondly, ptracers also have to support a lot of arch-specific code for
obtaining information about the tracee.  For some architectures, this
requires a ptrace(PTRACE_PEEKUSER, ...) invocation for every syscall
argument and return value.

PTRACE_GET_SYSCALL_INFO returns the following structure:

struct ptrace_syscall_info {
	__u8 op;	/* PTRACE_SYSCALL_INFO_* */
	__u32 arch __attribute__((__aligned__(sizeof(__u32))));
	__u64 instruction_pointer;
	__u64 stack_pointer;
	union {
		struct {
			__u64 nr;
			__u64 args[6];
		} entry;
		struct {
			__s64 rval;
			__u8 is_error;
		} exit;
		struct {
			__u64 nr;
			__u64 args[6];
			__u32 ret_data;
		} seccomp;
	};
};

The structure was chosen according to [2], except for the following
changes:
* seccomp substructure was added as a superset of entry substructure;
* the type of nr field was changed from int to __u64 because syscall
numbers are, as a practical matter, 64 bits;
* stack_pointer field was added along with instruction_pointer field
since it is readily available and can save the tracer from extra
PTRACE_GETREGS/PTRACE_GETREGSET calls;
* arch is always initialized to aid with tracing system calls
* such as execve();
* instruction_pointer and stack_pointer are always initialized
so they could be easily obtained for non-syscall stops;
* a boolean is_error field was added along with rval field, this way
the tracer can more reliably distinguish a return value
from an error value.

strace has been ported to PTRACE_GET_SYSCALL_INFO.
Starting with release 4.26, strace uses PTRACE_GET_SYSCALL_INFO API
as the preferred mechanism of obtaining syscall information.

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

---

Notes:
    v10: added more Acked-by.

    v9:
    * Rebased to linux-next again due to syscall_get_arguments() signature change.

    v8:
    * Moved syscall_get_arch() specific patches to a separate patchset
      which is now merged into audit/next tree.
    * Rebased to linux-next.
    * Moved ptrace_get_syscall_info code under #ifdef CONFIG_HAVE_ARCH_TRACEHOOK,
      narrowing down the set of architectures supported by this implementation
      back to those 19 that enable CONFIG_HAVE_ARCH_TRACEHOOK because
      I failed to get all syscall_get_*(), instruction_pointer(),
      and user_stack_pointer() functions implemented on some niche
      architectures.  This leaves the following architectures out:
      alpha, h8300, m68k, microblaze, and unicore32.

    v7:
    * Rebased to v5.0-rc1.
    * 5 arch-specific preparatory patches out of 25 have been merged
      into v5.0-rc1 via arch trees.

    v6:
    * Add syscall_get_arguments and syscall_set_arguments wrappers
      to asm-generic/syscall.h, requested by Geert.
    * Change PTRACE_GET_SYSCALL_INFO return code: do not take trailing paddings
      into account, use the end of the last field of the structure being written.
    * Change struct ptrace_syscall_info:
      * remove .frame_pointer field, is is not needed and not portable;
      * make .arch field explicitly aligned, remove no longer needed
        padding before .arch field;
      * remove trailing pads, they are no longer needed.

    v5:
    * Merge separate series and patches into the single series.
    * Change PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} values as requested by Oleg.
    * Change struct ptrace_syscall_info: generalize instruction_pointer,
      stack_pointer, and frame_pointer fields by moving them from
      ptrace_syscall_info.{entry,seccomp} substructures to ptrace_syscall_info
      and initializing them for all stops.
    * Add PTRACE_SYSCALL_INFO_NONE, set it when not in a syscall stop,
      so e.g. "strace -i" could use PTRACE_SYSCALL_INFO_SECCOMP to obtain
      instruction_pointer when the tracee is in a signal stop.
    * Patch all remaining architectures to provide all necessary
      syscall_get_* functions.
    * Make available for all architectures: do not conditionalize on
      CONFIG_HAVE_ARCH_TRACEHOOK since all syscall_get_* functions
      are implemented on all architectures.
    * Add a test for PTRACE_GET_SYSCALL_INFO to selftests/ptrace.
    
    v4:
    * Do not introduce task_struct.ptrace_event,
      use child->last_siginfo->si_code instead.
    * Implement PTRACE_SYSCALL_INFO_SECCOMP and ptrace_syscall_info.seccomp
      support along with PTRACE_SYSCALL_INFO_{ENTRY,EXIT} and
      ptrace_syscall_info.{entry,exit}.
    
    v3:
    * Change struct ptrace_syscall_info.
    * Support PTRACE_EVENT_SECCOMP by adding ptrace_event to task_struct.
    * Add proper defines for ptrace_syscall_info.op values.
    * Rename PT_SYSCALL_IS_ENTERING and PT_SYSCALL_IS_EXITING to
      PTRACE_EVENTMSG_SYSCALL_ENTRY and PTRACE_EVENTMSG_SYSCALL_EXIT
    * and move them to uapi.
    
    v2:
    * Do not use task->ptrace.
    * Replace entry_info.is_compat with entry_info.arch, use syscall_get_arch().
    * Use addr argument of sys_ptrace to get expected size of the struct;
      return full size of the struct.

Dmitry V. Levin (6):
  nds32: fix asm/syscall.h # acked
  hexagon: define syscall_get_error() and syscall_get_return_value() # waiting for ack since November
  mips: define syscall_get_error() # acked
  parisc: define syscall_get_error() # acked
  powerpc: define syscall_get_error() # waiting for ack since early December
  selftests/ptrace: add a test case for PTRACE_GET_SYSCALL_INFO # acked

Elvira Khabirova (1):
  ptrace: add PTRACE_GET_SYSCALL_INFO request # reviewed

 arch/hexagon/include/asm/syscall.h            |  14 +
 arch/mips/include/asm/syscall.h               |   6 +
 arch/nds32/include/asm/syscall.h              |  27 +-
 arch/parisc/include/asm/syscall.h             |   7 +
 arch/powerpc/include/asm/syscall.h            |  10 +
 include/linux/tracehook.h                     |   9 +-
 include/uapi/linux/ptrace.h                   |  35 +++
 kernel/ptrace.c                               | 103 ++++++-
 tools/testing/selftests/ptrace/.gitignore     |   1 +
 tools/testing/selftests/ptrace/Makefile       |   2 +-
 .../selftests/ptrace/get_syscall_info.c       | 271 ++++++++++++++++++
 11 files changed, 470 insertions(+), 15 deletions(-)
 create mode 100644 tools/testing/selftests/ptrace/get_syscall_info.c

-- 
ldv

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

* [PATCH linux-next v10 5/7] powerpc: define syscall_get_error()
  2019-04-15 23:43 [PATCH linux-next v10 0/7] ptrace: add PTRACE_GET_SYSCALL_INFO request Dmitry V. Levin
@ 2019-04-15 23:44 ` Dmitry V. Levin
  2019-05-06 13:17   ` Michael Ellerman
  2019-05-09 16:14 ` [PATCH linux-next v10 0/7] ptrace: add PTRACE_GET_SYSCALL_INFO request Oleg Nesterov
  1 sibling, 1 reply; 4+ messages in thread
From: Dmitry V. Levin @ 2019-04-15 23:44 UTC (permalink / raw)
  To: Andrew Morton, Michael Ellerman
  Cc: Eugene Syromyatnikov, Oleg Nesterov, Elvira Khabirova,
	Paul Mackerras, Andy Lutomirski, linuxppc-dev, linux-kernel

syscall_get_error() is required to be implemented on this
architecture in addition to already implemented syscall_get_nr(),
syscall_get_arguments(), syscall_get_return_value(), and
syscall_get_arch() functions in order to extend the generic
ptrace API with PTRACE_GET_SYSCALL_INFO request.

Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Elvira Khabirova <lineprinter@altlinux.org>
Cc: Eugene Syromyatnikov <esyr@redhat.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---

Michael, this patch is waiting for ACK since early December.

Notes:
    v10: unchanged
    v9: unchanged
    v8: unchanged
    v7: unchanged
    v6: unchanged
    v5: initial revision
    
    This change has been tested with
    tools/testing/selftests/ptrace/get_syscall_info.c and strace,
    so it's correct from PTRACE_GET_SYSCALL_INFO point of view.
    
    This cast doubts on commit v4.3-rc1~86^2~81 that changed
    syscall_set_return_value() in a way that doesn't quite match
    syscall_get_error(), but syscall_set_return_value() is out
    of scope of this series, so I'll just let you know my concerns.
    
    See also https://lore.kernel.org/lkml/874lbbt3k6.fsf@concordia.ellerman.id.au/
    for more details on powerpc syscall_set_return_value() confusion.

 arch/powerpc/include/asm/syscall.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
index a048fed0722f..bd9663137d57 100644
--- a/arch/powerpc/include/asm/syscall.h
+++ b/arch/powerpc/include/asm/syscall.h
@@ -38,6 +38,16 @@ static inline void syscall_rollback(struct task_struct *task,
 	regs->gpr[3] = regs->orig_gpr3;
 }
 
+static inline long syscall_get_error(struct task_struct *task,
+				     struct pt_regs *regs)
+{
+	/*
+	 * If the system call failed,
+	 * regs->gpr[3] contains a positive ERRORCODE.
+	 */
+	return (regs->ccr & 0x10000000UL) ? -regs->gpr[3] : 0;
+}
+
 static inline long syscall_get_return_value(struct task_struct *task,
 					    struct pt_regs *regs)
 {
-- 
ldv

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

* Re: [PATCH linux-next v10 5/7] powerpc: define syscall_get_error()
  2019-04-15 23:44 ` [PATCH linux-next v10 5/7] powerpc: define syscall_get_error() Dmitry V. Levin
@ 2019-05-06 13:17   ` Michael Ellerman
  0 siblings, 0 replies; 4+ messages in thread
From: Michael Ellerman @ 2019-05-06 13:17 UTC (permalink / raw)
  To: Dmitry V. Levin, Andrew Morton
  Cc: Eugene Syromyatnikov, Oleg Nesterov, Elvira Khabirova,
	Paul Mackerras, Andy Lutomirski, linuxppc-dev, linux-kernel

"Dmitry V. Levin" <ldv@altlinux.org> writes:

> syscall_get_error() is required to be implemented on this
> architecture in addition to already implemented syscall_get_nr(),
> syscall_get_arguments(), syscall_get_return_value(), and
> syscall_get_arch() functions in order to extend the generic
> ptrace API with PTRACE_GET_SYSCALL_INFO request.
>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Elvira Khabirova <lineprinter@altlinux.org>
> Cc: Eugene Syromyatnikov <esyr@redhat.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
> ---
>
> Michael, this patch is waiting for ACK since early December.

Sorry, the more I look at our seccomp/ptrace code the more problems I
find :/

This change looks OK to me, given it will only be called by your new
ptrace API.

Acked-by: Michael Ellerman <mpe@ellerman.id.au>


> Notes:
>     v10: unchanged
>     v9: unchanged
>     v8: unchanged
>     v7: unchanged
>     v6: unchanged
>     v5: initial revision
>     
>     This change has been tested with
>     tools/testing/selftests/ptrace/get_syscall_info.c and strace,
>     so it's correct from PTRACE_GET_SYSCALL_INFO point of view.
>     
>     This cast doubts on commit v4.3-rc1~86^2~81 that changed
>     syscall_set_return_value() in a way that doesn't quite match
>     syscall_get_error(), but syscall_set_return_value() is out
>     of scope of this series, so I'll just let you know my concerns.
     
Yeah I think you're right. My commit made it work for seccomp but only
on the basis that seccomp calls syscall_set_return_value() and then
immediately goes out via the syscall exit path. And only the combination
of those gets things into the same state that syscall_get_error()
expects.

But with the way the code is currently structured if
syscall_set_return_value() negated the error value, then the syscall
exit path would then store the wrong thing in pt_regs->result. So I
think it needs some more work rather than just reverting 1b1a3702a65c.

But I think fixing that can be orthogonal to this commit going in as the
code does work as it's currently written, the in-between state that
syscall_set_return_value() creates via seccomp should not be visible to
ptrace.

cheers

>     See also https://lore.kernel.org/lkml/874lbbt3k6.fsf@concordia.ellerman.id.au/
>     for more details on powerpc syscall_set_return_value() confusion.
>
>  arch/powerpc/include/asm/syscall.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
> index a048fed0722f..bd9663137d57 100644
> --- a/arch/powerpc/include/asm/syscall.h
> +++ b/arch/powerpc/include/asm/syscall.h
> @@ -38,6 +38,16 @@ static inline void syscall_rollback(struct task_struct *task,
>  	regs->gpr[3] = regs->orig_gpr3;
>  }
>  
> +static inline long syscall_get_error(struct task_struct *task,
> +				     struct pt_regs *regs)
> +{
> +	/*
> +	 * If the system call failed,
> +	 * regs->gpr[3] contains a positive ERRORCODE.
> +	 */
> +	return (regs->ccr & 0x10000000UL) ? -regs->gpr[3] : 0;
> +}
> +
>  static inline long syscall_get_return_value(struct task_struct *task,
>  					    struct pt_regs *regs)
>  {
> -- 
> ldv

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

* Re: [PATCH linux-next v10 0/7] ptrace: add PTRACE_GET_SYSCALL_INFO request
  2019-04-15 23:43 [PATCH linux-next v10 0/7] ptrace: add PTRACE_GET_SYSCALL_INFO request Dmitry V. Levin
  2019-04-15 23:44 ` [PATCH linux-next v10 5/7] powerpc: define syscall_get_error() Dmitry V. Levin
@ 2019-05-09 16:14 ` Oleg Nesterov
  1 sibling, 0 replies; 4+ messages in thread
From: Oleg Nesterov @ 2019-05-09 16:14 UTC (permalink / raw)
  To: Dmitry V. Levin, Andrew Morton
  Cc: James E.J. Bottomley, Paul Mackerras, linux-kselftest,
	Vincent Chen, Shuah Khan, Helge Deller, Eugene Syromyatnikov,
	Elvira Khabirova, James Hogan, strace-devel, Kees Cook,
	Greentime Hu, linux-kernel, Andy Lutomirski, linux-parisc,
	linux-api, linux-mips, Ralf Baechle, Richard Kuo, Paul Burton,
	linux-hexagon, linuxppc-dev

On 04/16, Dmitry V. Levin wrote:
>
> [Andrew, could you take this patchset into your tree, please?]

Just in case...

I have already acked 6/7.

Other patches look good to me too, just I don't think I can actually review
these non-x86 changes.

Oleg.


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

end of thread, other threads:[~2019-05-09 16:16 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-15 23:43 [PATCH linux-next v10 0/7] ptrace: add PTRACE_GET_SYSCALL_INFO request Dmitry V. Levin
2019-04-15 23:44 ` [PATCH linux-next v10 5/7] powerpc: define syscall_get_error() Dmitry V. Levin
2019-05-06 13:17   ` Michael Ellerman
2019-05-09 16:14 ` [PATCH linux-next v10 0/7] ptrace: add PTRACE_GET_SYSCALL_INFO request Oleg Nesterov

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