All of lore.kernel.org
 help / color / mirror / Atom feed
From: will.deacon@arm.com (Will Deacon)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v8 2/6] arm64: ptrace: allow tracer to skip a system call
Date: Tue, 18 Nov 2014 14:04:25 +0000	[thread overview]
Message-ID: <20141118140425.GM18842@arm.com> (raw)
In-Reply-To: <1416273038-15590-3-git-send-email-takahiro.akashi@linaro.org>

On Tue, Nov 18, 2014 at 01:10:34AM +0000, AKASHI Takahiro wrote:
> If tracer specifies -1 as a syscall number, this traced system call should
> be skipped with a return value specified in x0.
> This patch implements this semantics, but there is one restriction here:
> 
>     syscall(-1) always return ENOSYS whatever value is stored in x0
>     (a return value) at syscall entry.
> 
> Normally, with ptrace off, syscall(-1) returns -ENOSYS. With ptrace on,
> however, if a tracer didn't pay any attention to user-issued syscall(-1)
> and just let it go, it would return a value in x0 as in other system call
> cases. This means that this system call might succeed and yet see any bogus
> return value. This should be definitely avoided.
> 
> Please also note:
> * syscall entry tracing and syscall exit tracing (ftrace tracepoint and
>   audit) are always executed, if enabled, even when skipping a system call
>   (that is, -1).
>   In this way, we can avoid a potential bug where audit_syscall_entry()
>   might be called without audit_syscall_exit() at the previous system call
>   being called, that would cause OOPs in audit_syscall_entry().
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  arch/arm64/kernel/entry.S  |    3 +++
>  arch/arm64/kernel/ptrace.c |   18 ++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 726b910..01118b1 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -670,6 +670,8 @@ ENDPROC(el0_svc)
>  __sys_trace:
>  	mov	x0, sp
>  	bl	syscall_trace_enter
> +	cmp	w0, #-1				// skip the syscall?
> +	b.eq	__sys_trace_return_skipped
>  	adr	lr, __sys_trace_return		// return address
>  	uxtw	scno, w0			// syscall number (possibly new)
>  	mov	x1, sp				// pointer to regs
> @@ -684,6 +686,7 @@ __sys_trace:
>  
>  __sys_trace_return:
>  	str	x0, [sp]			// save returned x0
> +__sys_trace_return_skipped:
>  	mov	x0, sp
>  	bl	syscall_trace_exit
>  	b	ret_to_user
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 8b98781..34b1e85 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -1149,6 +1149,8 @@ static void tracehook_report_syscall(struct pt_regs *regs,
>  
>  asmlinkage int syscall_trace_enter(struct pt_regs *regs)
>  {
> +	int orig_syscallno = regs->syscallno;
> +
>  	if (test_thread_flag(TIF_SYSCALL_TRACE))
>  		tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
>  
> @@ -1158,6 +1160,22 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs)
>  	audit_syscall_entry(regs->syscallno, regs->orig_x0, regs->regs[1],
>  			    regs->regs[2], regs->regs[3]);
>  
> +	if (((int)regs->syscallno == -1) && (orig_syscallno == -1)) {
> +		/*
> +		 * user-issued syscall(-1):
> +		 * RESTRICTION: We always return ENOSYS whatever value is
> +		 *   stored in x0 (a return value) at this point.
> +		 * Normally, with ptrace off, syscall(-1) returns -ENOSYS.
> +		 * With ptrace on, however, if a tracer didn't pay any
> +		 * attention to user-issued syscall(-1) and just let it go
> +		 * without a hack here, it would return a value in x0 as in
> +		 * other system call cases. This means that this system call
> +		 * might succeed and see any bogus return value.
> +		 * This should be definitely avoided.
> +		 */
> +		regs->regs[0] = -ENOSYS;
> +	}

I'm still really uncomfortable with this, and it doesn't seem to match what
arch/arm/ does either. Doesn't it also prevent a tracer from skipping
syscall(-1)?

Will

WARNING: multiple messages have this Message-ID (diff)
From: Will Deacon <will.deacon@arm.com>
To: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: "keescook@chromium.org" <keescook@chromium.org>,
	Catalin Marinas <Catalin.Marinas@arm.com>,
	"dsaxena@linaro.org" <dsaxena@linaro.org>,
	"arndb@arndb.de" <arndb@arndb.de>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	"linaro-kernel@lists.linaro.org" <linaro-kernel@lists.linaro.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v8 2/6] arm64: ptrace: allow tracer to skip a system call
Date: Tue, 18 Nov 2014 14:04:25 +0000	[thread overview]
Message-ID: <20141118140425.GM18842@arm.com> (raw)
In-Reply-To: <1416273038-15590-3-git-send-email-takahiro.akashi@linaro.org>

On Tue, Nov 18, 2014 at 01:10:34AM +0000, AKASHI Takahiro wrote:
> If tracer specifies -1 as a syscall number, this traced system call should
> be skipped with a return value specified in x0.
> This patch implements this semantics, but there is one restriction here:
> 
>     syscall(-1) always return ENOSYS whatever value is stored in x0
>     (a return value) at syscall entry.
> 
> Normally, with ptrace off, syscall(-1) returns -ENOSYS. With ptrace on,
> however, if a tracer didn't pay any attention to user-issued syscall(-1)
> and just let it go, it would return a value in x0 as in other system call
> cases. This means that this system call might succeed and yet see any bogus
> return value. This should be definitely avoided.
> 
> Please also note:
> * syscall entry tracing and syscall exit tracing (ftrace tracepoint and
>   audit) are always executed, if enabled, even when skipping a system call
>   (that is, -1).
>   In this way, we can avoid a potential bug where audit_syscall_entry()
>   might be called without audit_syscall_exit() at the previous system call
>   being called, that would cause OOPs in audit_syscall_entry().
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  arch/arm64/kernel/entry.S  |    3 +++
>  arch/arm64/kernel/ptrace.c |   18 ++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 726b910..01118b1 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -670,6 +670,8 @@ ENDPROC(el0_svc)
>  __sys_trace:
>  	mov	x0, sp
>  	bl	syscall_trace_enter
> +	cmp	w0, #-1				// skip the syscall?
> +	b.eq	__sys_trace_return_skipped
>  	adr	lr, __sys_trace_return		// return address
>  	uxtw	scno, w0			// syscall number (possibly new)
>  	mov	x1, sp				// pointer to regs
> @@ -684,6 +686,7 @@ __sys_trace:
>  
>  __sys_trace_return:
>  	str	x0, [sp]			// save returned x0
> +__sys_trace_return_skipped:
>  	mov	x0, sp
>  	bl	syscall_trace_exit
>  	b	ret_to_user
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 8b98781..34b1e85 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -1149,6 +1149,8 @@ static void tracehook_report_syscall(struct pt_regs *regs,
>  
>  asmlinkage int syscall_trace_enter(struct pt_regs *regs)
>  {
> +	int orig_syscallno = regs->syscallno;
> +
>  	if (test_thread_flag(TIF_SYSCALL_TRACE))
>  		tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
>  
> @@ -1158,6 +1160,22 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs)
>  	audit_syscall_entry(regs->syscallno, regs->orig_x0, regs->regs[1],
>  			    regs->regs[2], regs->regs[3]);
>  
> +	if (((int)regs->syscallno == -1) && (orig_syscallno == -1)) {
> +		/*
> +		 * user-issued syscall(-1):
> +		 * RESTRICTION: We always return ENOSYS whatever value is
> +		 *   stored in x0 (a return value) at this point.
> +		 * Normally, with ptrace off, syscall(-1) returns -ENOSYS.
> +		 * With ptrace on, however, if a tracer didn't pay any
> +		 * attention to user-issued syscall(-1) and just let it go
> +		 * without a hack here, it would return a value in x0 as in
> +		 * other system call cases. This means that this system call
> +		 * might succeed and see any bogus return value.
> +		 * This should be definitely avoided.
> +		 */
> +		regs->regs[0] = -ENOSYS;
> +	}

I'm still really uncomfortable with this, and it doesn't seem to match what
arch/arm/ does either. Doesn't it also prevent a tracer from skipping
syscall(-1)?

Will

  reply	other threads:[~2014-11-18 14:04 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-18  1:10 [PATCH v8 0/6] arm64: add seccomp support AKASHI Takahiro
2014-11-18  1:10 ` AKASHI Takahiro
2014-11-18  1:10 ` [PATCH v8 1/6] arm64: ptrace: add NT_ARM_SYSTEM_CALL regset AKASHI Takahiro
2014-11-18  1:10   ` AKASHI Takahiro
2014-11-18  1:10 ` [PATCH v8 2/6] arm64: ptrace: allow tracer to skip a system call AKASHI Takahiro
2014-11-18  1:10   ` AKASHI Takahiro
2014-11-18 14:04   ` Will Deacon [this message]
2014-11-18 14:04     ` Will Deacon
2014-11-19  8:46     ` AKASHI Takahiro
2014-11-19  8:46       ` AKASHI Takahiro
2014-11-19 19:06       ` Will Deacon
2014-11-19 19:06         ` Will Deacon
2014-11-20  5:13         ` AKASHI Takahiro
2014-11-20  5:13           ` AKASHI Takahiro
2014-11-20  5:52           ` AKASHI Takahiro
2014-11-20  5:52             ` AKASHI Takahiro
2014-11-25 13:56             ` Will Deacon
2014-11-25 13:56               ` Will Deacon
2014-11-20 19:17           ` Will Deacon
2014-11-20 19:17             ` Will Deacon
2014-11-25  7:42             ` AKASHI Takahiro
2014-11-25  7:42               ` AKASHI Takahiro
2014-11-25 10:30               ` Will Deacon
2014-11-25 10:30                 ` Will Deacon
2014-11-25 14:14           ` Russell King - ARM Linux
2014-11-25 14:14             ` Russell King - ARM Linux
2014-11-18  1:10 ` [PATCH v8 3/6] asm-generic: add generic seccomp.h for secure computing mode 1 AKASHI Takahiro
2014-11-18  1:10   ` AKASHI Takahiro
2014-11-18  1:10 ` [PATCH v8 4/6] arm64: add seccomp syscall for compat task AKASHI Takahiro
2014-11-18  1:10   ` AKASHI Takahiro
2014-11-18  1:10 ` [PATCH v8 5/6] arm64: add SIGSYS siginfo " AKASHI Takahiro
2014-11-18  1:10   ` AKASHI Takahiro
2014-11-18  1:10 ` [PATCH v8 6/6] arm64: add seccomp support AKASHI Takahiro
2014-11-18  1:10   ` AKASHI Takahiro

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20141118140425.GM18842@arm.com \
    --to=will.deacon@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.