Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[AKASHI Takahiro]

On 10/04/2014 12:23 AM, Will Deacon wrote:

On Wed, Oct 01, 2014 at 12:08:05PM +0100, AKASHI Takahiro wrote:

Will,

When I was looking into syscall_trace_exit() more closely, I found
another (big) problem.
There are two system calls, execve() and rt_sigreturn(), which change
'syscallno' in pt_regs to -1 in start_thread() and restore_sigframe(),
respectively.


I need to correct my mis-understandings here:


Since syscallno is not valid anymore in syscall_trace_exit() for these
system calls, we cannot create a correct syscall exit record for tracepoint
in trace_sys_exit() (=> ftrace_syscall_exit())


This is true, but since rt_sigreturn() doesn't have a syscall tracepoint (and so
there is no entry under /sys/kernel/tracing/events/syscalls/), it cannot be
traced anyway.


and for audit in audit_syscall_exit().


not true. Since a syscall number is saved as 'major' in a per-thread audit 
context
at audit_syscall_exit(), we will see a correct audit log for both system calls.


This does not happen on arm because syscall numbers are kept in
thread_info on arm.

How can we deal with this issue?


How is this handled on other architectures? x86, for example, seems to zero
orig_ax when restoring the sigcontext, but leaves it alone in start_thread.



What is the impact of this problem? AFAICT, we just miss some exits, right
(as opposed to an OOPs or the like)?


So the impacts here are:
1) We just miss a syscall exit for execve tracepoint (syscalls:sys_exit_execve).
   (no fatal errors like kernel panic)
   (FYI, on x86, there is no tracepoint entry for execve nor sigreturn.)

2) From the viewpoint of my seccomp patch, we cannot skip some syscall exit 
tracing
   for invalid system calls by adding a check for syscallno in the following 
way:
   (I'm not quite sure this might cause a threat with DDoS attach as Russell 
suggested.)

   syscall_trace_exit(struct pt_regs *regs) {
  if (regs->syscallno < NR_syscalls) { /* Adding this check */
 audit_syscall_exit(regs);
 if (test_thread_flags(TIF_SYSCALL_TRACEPOINT))
trace_sys_exit(regs, regs_return_value(regs));
  }
  ...
   }

   As you can imagine, any system call after execve() will hit BUG_ON()
   in audit_syscall_exit() since audit_syscall_exit() is not called for 
execve().

Thanks,
-Takahiro AKASHI


Will


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[AKASHI Takahiro]

On 10/04/2014 12:23 AM, Will Deacon wrote:

On Wed, Oct 01, 2014 at 12:08:05PM +0100, AKASHI Takahiro wrote:

Will,

When I was looking into syscall_trace_exit() more closely, I found
another (big) problem.
There are two system calls, execve() and rt_sigreturn(), which change
'syscallno' in pt_regs to -1 in start_thread() and restore_sigframe(),
respectively.


I need to correct my mis-understandings here:


Since syscallno is not valid anymore in syscall_trace_exit() for these
system calls, we cannot create a correct syscall exit record for tracepoint
in trace_sys_exit() (= ftrace_syscall_exit())


This is true, but since rt_sigreturn() doesn't have a syscall tracepoint (and so
there is no entry under /sys/kernel/tracing/events/syscalls/), it cannot be
traced anyway.


and for audit in audit_syscall_exit().


not true. Since a syscall number is saved as 'major' in a per-thread audit 
context
at audit_syscall_exit(), we will see a correct audit log for both system calls.


This does not happen on arm because syscall numbers are kept in
thread_info on arm.

How can we deal with this issue?


How is this handled on other architectures? x86, for example, seems to zero
orig_ax when restoring the sigcontext, but leaves it alone in start_thread.



What is the impact of this problem? AFAICT, we just miss some exits, right
(as opposed to an OOPs or the like)?


So the impacts here are:
1) We just miss a syscall exit for execve tracepoint (syscalls:sys_exit_execve).
   (no fatal errors like kernel panic)
   (FYI, on x86, there is no tracepoint entry for execve nor sigreturn.)

2) From the viewpoint of my seccomp patch, we cannot skip some syscall exit 
tracing
   for invalid system calls by adding a check for syscallno in the following 
way:
   (I'm not quite sure this might cause a threat with DDoS attach as Russell 
suggested.)

   syscall_trace_exit(struct pt_regs *regs) {
  if (regs-syscallno  NR_syscalls) { /* Adding this check */
 audit_syscall_exit(regs);
 if (test_thread_flags(TIF_SYSCALL_TRACEPOINT))
trace_sys_exit(regs, regs_return_value(regs));
  }
  ...
   }

   As you can imagine, any system call after execve() will hit BUG_ON()
   in audit_syscall_exit() since audit_syscall_exit() is not called for 
execve().

Thanks,
-Takahiro AKASHI


Will


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[Will Deacon]

On Wed, Oct 01, 2014 at 12:08:05PM +0100, AKASHI Takahiro wrote:
> Will,
> 
> When I was looking into syscall_trace_exit() more closely, I found
> another (big) problem.
> There are two system calls, execve() and rt_sigreturn(), which change
> 'syscallno' in pt_regs to -1 in start_thread() and restore_sigframe(),
> respectively.
> 
> Since syscallno is not valid anymore in syscall_trace_exit() for these
> system calls, we cannot create a correct syscall exit record for tracepoint
> in trace_sys_exit() (=> ftrace_syscall_exit()) and for audit in 
> audit_syscall_exit().
> 
> This does not happen on arm because syscall numbers are kept in
> thread_info on arm.
> 
> How can we deal with this issue?

How is this handled on other architectures? x86, for example, seems to zero
orig_ax when restoring the sigcontext, but leaves it alone in start_thread.

What is the impact of this problem? AFAICT, we just miss some exits, right
(as opposed to an OOPs or the like)?

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[Will Deacon]

On Wed, Oct 01, 2014 at 12:08:05PM +0100, AKASHI Takahiro wrote:
 Will,
 
 When I was looking into syscall_trace_exit() more closely, I found
 another (big) problem.
 There are two system calls, execve() and rt_sigreturn(), which change
 'syscallno' in pt_regs to -1 in start_thread() and restore_sigframe(),
 respectively.
 
 Since syscallno is not valid anymore in syscall_trace_exit() for these
 system calls, we cannot create a correct syscall exit record for tracepoint
 in trace_sys_exit() (= ftrace_syscall_exit()) and for audit in 
 audit_syscall_exit().
 
 This does not happen on arm because syscall numbers are kept in
 thread_info on arm.
 
 How can we deal with this issue?

How is this handled on other architectures? x86, for example, seems to zero
orig_ax when restoring the sigcontext, but leaves it alone in start_thread.

What is the impact of this problem? AFAICT, we just miss some exits, right
(as opposed to an OOPs or the like)?

Will
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[AKASHI Takahiro]

Will,

When I was looking into syscall_trace_exit() more closely, I found
another (big) problem.
There are two system calls, execve() and rt_sigreturn(), which change
'syscallno' in pt_regs to -1 in start_thread() and restore_sigframe(),
respectively.

Since syscallno is not valid anymore in syscall_trace_exit() for these
system calls, we cannot create a correct syscall exit record for tracepoint
in trace_sys_exit() (=> ftrace_syscall_exit()) and for audit in 
audit_syscall_exit().

This does not happen on arm because syscall numbers are kept in
thread_info on arm.

How can we deal with this issue?

-Takahiro AKASHI


On 08/27/2014 02:51 AM, Will Deacon wrote:

On Fri, Aug 22, 2014 at 01:35:17AM +0100, AKASHI Takahiro wrote:

On 08/22/2014 02:08 AM, Kees Cook wrote:

On Thu, Aug 21, 2014 at 3:56 AM, AKASHI Takahiro
 wrote:

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 8876049..c54dbcc 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1121,9 +1121,29 @@ static void tracehook_report_syscall(struct pt_regs 
*regs,

   asmlinkage int syscall_trace_enter(struct pt_regs *regs)
   {
+   unsigned int saved_syscallno = regs->syscallno;
+
  if (test_thread_flag(TIF_SYSCALL_TRACE))
  tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);

+   if (IS_SKIP_SYSCALL(regs->syscallno)) {
+   /*
+* RESTRICTION: we can't modify a return value of user
+* issued syscall(-1) here. In order to ease this flavor,
+* we need to treat whatever value in x0 as a return value,
+* but this might result in a bogus value being returned.
+*/
+   /*
+* NOTE: syscallno may also be set to -1 if fatal signal is
+* detected in tracehook_report_syscall_entry(), but since
+* a value set to x0 here is not used in this case, we may
+* neglect the case.
+*/
+   if (!test_thread_flag(TIF_SYSCALL_TRACE) ||
+   (IS_SKIP_SYSCALL(saved_syscallno)))
+   regs->regs[0] = -ENOSYS;
+   }
+


I don't have a runtime environment yet for arm64, so I can't test this
directly myself, so I'm just trying to eyeball this. :)

Once the seccomp logic is added here, I don't think using -2 as a
special value will work. Doesn't this mean the Oops is possible by the
user issuing a "-2" syscall? As in, if TIF_SYSCALL_WORK is set, and
the user passed -2 as the syscall, audit will be called only on entry,
and then skipped on exit?


Oops, you're absolutely right. I didn't think of this case.
syscall_trace_enter() should not return a syscallno directly, but always
return -1 if syscallno < 0. (except when secure_computing() returns with -1)
This also implies that tracehook_report_syscall() should also have a return 
value.

Will, is this fine with you?


Well, the first thing that jumps out at me is why this is being done
completely differently for arm64 and arm. I thought adding the new ptrace
requests would reconcile the differences?

Will


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[AKASHI Takahiro]

Will,

When I was looking into syscall_trace_exit() more closely, I found
another (big) problem.
There are two system calls, execve() and rt_sigreturn(), which change
'syscallno' in pt_regs to -1 in start_thread() and restore_sigframe(),
respectively.

Since syscallno is not valid anymore in syscall_trace_exit() for these
system calls, we cannot create a correct syscall exit record for tracepoint
in trace_sys_exit() (= ftrace_syscall_exit()) and for audit in 
audit_syscall_exit().

This does not happen on arm because syscall numbers are kept in
thread_info on arm.

How can we deal with this issue?

-Takahiro AKASHI


On 08/27/2014 02:51 AM, Will Deacon wrote:

On Fri, Aug 22, 2014 at 01:35:17AM +0100, AKASHI Takahiro wrote:

On 08/22/2014 02:08 AM, Kees Cook wrote:

On Thu, Aug 21, 2014 at 3:56 AM, AKASHI Takahiro
takahiro.aka...@linaro.org wrote:

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 8876049..c54dbcc 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1121,9 +1121,29 @@ static void tracehook_report_syscall(struct pt_regs 
*regs,

   asmlinkage int syscall_trace_enter(struct pt_regs *regs)
   {
+   unsigned int saved_syscallno = regs-syscallno;
+
  if (test_thread_flag(TIF_SYSCALL_TRACE))
  tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);

+   if (IS_SKIP_SYSCALL(regs-syscallno)) {
+   /*
+* RESTRICTION: we can't modify a return value of user
+* issued syscall(-1) here. In order to ease this flavor,
+* we need to treat whatever value in x0 as a return value,
+* but this might result in a bogus value being returned.
+*/
+   /*
+* NOTE: syscallno may also be set to -1 if fatal signal is
+* detected in tracehook_report_syscall_entry(), but since
+* a value set to x0 here is not used in this case, we may
+* neglect the case.
+*/
+   if (!test_thread_flag(TIF_SYSCALL_TRACE) ||
+   (IS_SKIP_SYSCALL(saved_syscallno)))
+   regs-regs[0] = -ENOSYS;
+   }
+


I don't have a runtime environment yet for arm64, so I can't test this
directly myself, so I'm just trying to eyeball this. :)

Once the seccomp logic is added here, I don't think using -2 as a
special value will work. Doesn't this mean the Oops is possible by the
user issuing a -2 syscall? As in, if TIF_SYSCALL_WORK is set, and
the user passed -2 as the syscall, audit will be called only on entry,
and then skipped on exit?


Oops, you're absolutely right. I didn't think of this case.
syscall_trace_enter() should not return a syscallno directly, but always
return -1 if syscallno  0. (except when secure_computing() returns with -1)
This also implies that tracehook_report_syscall() should also have a return 
value.

Will, is this fine with you?


Well, the first thing that jumps out at me is why this is being done
completely differently for arm64 and arm. I thought adding the new ptrace
requests would reconcile the differences?

Will


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[AKASHI Takahiro]

On 09/02/2014 06:16 PM, Russell King - ARM Linux wrote:

On Tue, Sep 02, 2014 at 05:47:29PM +0900, AKASHI Takahiro wrote:

On 09/01/2014 08:47 PM, Russell King - ARM Linux wrote:

On Wed, Aug 27, 2014 at 02:55:46PM +0900, AKASHI Takahiro wrote:

1)
setting x0 to -ENOSYS is necessary because, otherwise, user-issued syscall(-1) 
will
return a bogus value when audit tracing is on.

Please note that, on arm,
   not traced  traced
   --  --
syscall(-1)  aborted OOPs(BUG_ON)
syscall(-3000)   aborted aborted
syscall(1000)ENOSYS  ENOSYS


Two points here:

1. You've found a case which causes a BUG_ON().  Where is the bug report
 for this, so the problem can be investigated and resolved?


I think that I mentioned it could also happen on arm somewhere in a talk
with Will, but don't remember exactly when.


Sorry, not good enough.  Please report this bug so it can be investigated
and fixed.


Please review my patch as well as the commit message.


2. What do you mean by "aborted" ?


I mean that the process will receive SIGILL and get aborted.
A system call number, like -1 and -3000, won't be trapped by *switch*
statement in asm_syscall() and end up with being signaled.


That is correct behaviour - because numbers greater than 0xf (or
0x9f for OABI - the 0x90 offset on the syscalls on OABI is to
distinguish them from syscalls used by RISC OS) are not intended to
be Linux syscalls per-se.


I tried to make such invalid/pseudo syscalls hehave in the same way
whether or not a task is traced (by seccomp, ptrace or audit).


Please, if you find a problem with 32-bit ARM, report it.  Don't hide it,
because hiding it can be a security issue or in the case of BUG_ON(), it
could be a denial of service issue.

As you're part of Linaro, I would have thought you'd be more responsible
in this regard - after all, Linaro is supposed to be about improving the
ARM kernel...  Maybe I got that wrong, and Linaro is actually about
ensuring that the ARM kernel is stuffed full of broken features?


I thought my first priority was on arm64 (and then arm), but now that
you and Will seem to want to see the fix first on arm, okey, I will
start with arm issue.


So what you're saying there is that if you find a bug in ARM code, which
everyone is currently using, you can ignore it until you've sorted out
ARM64 which almost no one is using.

This is absurd, and whoever has set your priorities is clearly on drugs.


Thank you. I learned a new English word, absurd.

-Takahiro AKASHI






--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[AKASHI Takahiro]

On 09/02/2014 06:16 PM, Russell King - ARM Linux wrote:

On Tue, Sep 02, 2014 at 05:47:29PM +0900, AKASHI Takahiro wrote:

On 09/01/2014 08:47 PM, Russell King - ARM Linux wrote:

On Wed, Aug 27, 2014 at 02:55:46PM +0900, AKASHI Takahiro wrote:

1)
setting x0 to -ENOSYS is necessary because, otherwise, user-issued syscall(-1) 
will
return a bogus value when audit tracing is on.

Please note that, on arm,
   not traced  traced
   --  --
syscall(-1)  aborted OOPs(BUG_ON)
syscall(-3000)   aborted aborted
syscall(1000)ENOSYS  ENOSYS


Two points here:

1. You've found a case which causes a BUG_ON().  Where is the bug report
 for this, so the problem can be investigated and resolved?


I think that I mentioned it could also happen on arm somewhere in a talk
with Will, but don't remember exactly when.


Sorry, not good enough.  Please report this bug so it can be investigated
and fixed.


Please review my patch as well as the commit message.


2. What do you mean by aborted ?


I mean that the process will receive SIGILL and get aborted.
A system call number, like -1 and -3000, won't be trapped by *switch*
statement in asm_syscall() and end up with being signaled.


That is correct behaviour - because numbers greater than 0xf (or
0x9f for OABI - the 0x90 offset on the syscalls on OABI is to
distinguish them from syscalls used by RISC OS) are not intended to
be Linux syscalls per-se.


I tried to make such invalid/pseudo syscalls hehave in the same way
whether or not a task is traced (by seccomp, ptrace or audit).


Please, if you find a problem with 32-bit ARM, report it.  Don't hide it,
because hiding it can be a security issue or in the case of BUG_ON(), it
could be a denial of service issue.

As you're part of Linaro, I would have thought you'd be more responsible
in this regard - after all, Linaro is supposed to be about improving the
ARM kernel...  Maybe I got that wrong, and Linaro is actually about
ensuring that the ARM kernel is stuffed full of broken features?


I thought my first priority was on arm64 (and then arm), but now that
you and Will seem to want to see the fix first on arm, okey, I will
start with arm issue.


So what you're saying there is that if you find a bug in ARM code, which
everyone is currently using, you can ignore it until you've sorted out
ARM64 which almost no one is using.

This is absurd, and whoever has set your priorities is clearly on drugs.


Thank you. I learned a new English word, absurd.

-Takahiro AKASHI






--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[Russell King - ARM Linux]

On Tue, Sep 02, 2014 at 10:16:22AM +0100, Russell King - ARM Linux wrote:
> On Tue, Sep 02, 2014 at 05:47:29PM +0900, AKASHI Takahiro wrote:
> > On 09/01/2014 08:47 PM, Russell King - ARM Linux wrote:
> >> On Wed, Aug 27, 2014 at 02:55:46PM +0900, AKASHI Takahiro wrote:
> >>> 1)
> >>> setting x0 to -ENOSYS is necessary because, otherwise, user-issued 
> >>> syscall(-1) will
> >>> return a bogus value when audit tracing is on.
> >>>
> >>> Please note that, on arm,
> >>>   not traced  traced
> >>>   --  --
> >>> syscall(-1)  aborted OOPs(BUG_ON)
> >>> syscall(-3000)   aborted aborted
> >>> syscall(1000)ENOSYS  ENOSYS
> >>
> >> Two points here:
> >>
> >> 1. You've found a case which causes a BUG_ON().  Where is the bug report
> >> for this, so the problem can be investigated and resolved?
> >
> > I think that I mentioned it could also happen on arm somewhere in a talk
> > with Will, but don't remember exactly when.
> 
> Sorry, not good enough.  Please report this bug so it can be investigated
> and fixed.

I'm going to go further than this, and tell you that you have been
downright irresponsible here, and I'm disgusted by your behaviour over
this.

You have revealed a potential security problem publically, effectively
giving details about how to cause it, but without having first reported
it to people who can fix it, nor providing a fix for it.

Why is it a security problem?  Although it can't be used to gain
information, it can be used potentially to deny service.  Any user can
trace a task which they own, and then set the task's syscall to -1,
which according to you results in a kernel oops.

If the kernel oops happens while holding any locks, that part of the
system becomes non-functional and can result in all userland stopping
dead.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[Russell King - ARM Linux]

On Tue, Sep 02, 2014 at 05:47:29PM +0900, AKASHI Takahiro wrote:
> On 09/01/2014 08:47 PM, Russell King - ARM Linux wrote:
>> On Wed, Aug 27, 2014 at 02:55:46PM +0900, AKASHI Takahiro wrote:
>>> 1)
>>> setting x0 to -ENOSYS is necessary because, otherwise, user-issued 
>>> syscall(-1) will
>>> return a bogus value when audit tracing is on.
>>>
>>> Please note that, on arm,
>>>   not traced  traced
>>>   --  --
>>> syscall(-1)  aborted OOPs(BUG_ON)
>>> syscall(-3000)   aborted aborted
>>> syscall(1000)ENOSYS  ENOSYS
>>
>> Two points here:
>>
>> 1. You've found a case which causes a BUG_ON().  Where is the bug report
>> for this, so the problem can be investigated and resolved?
>
> I think that I mentioned it could also happen on arm somewhere in a talk
> with Will, but don't remember exactly when.

Sorry, not good enough.  Please report this bug so it can be investigated
and fixed.

>> 2. What do you mean by "aborted" ?
>
> I mean that the process will receive SIGILL and get aborted.
> A system call number, like -1 and -3000, won't be trapped by *switch*
> statement in asm_syscall() and end up with being signaled.

That is correct behaviour - because numbers greater than 0xf (or
0x9f for OABI - the 0x90 offset on the syscalls on OABI is to
distinguish them from syscalls used by RISC OS) are not intended to
be Linux syscalls per-se.

>> Please, if you find a problem with 32-bit ARM, report it.  Don't hide it,
>> because hiding it can be a security issue or in the case of BUG_ON(), it
>> could be a denial of service issue.
>>
>> As you're part of Linaro, I would have thought you'd be more responsible
>> in this regard - after all, Linaro is supposed to be about improving the
>> ARM kernel...  Maybe I got that wrong, and Linaro is actually about
>> ensuring that the ARM kernel is stuffed full of broken features?
>
> I thought my first priority was on arm64 (and then arm), but now that
> you and Will seem to want to see the fix first on arm, okey, I will
> start with arm issue.

So what you're saying there is that if you find a bug in ARM code, which
everyone is currently using, you can ignore it until you've sorted out
ARM64 which almost no one is using.

This is absurd, and whoever has set your priorities is clearly on drugs.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[AKASHI Takahiro]

On 09/01/2014 08:47 PM, Russell King - ARM Linux wrote:

On Wed, Aug 27, 2014 at 02:55:46PM +0900, AKASHI Takahiro wrote:

1)
setting x0 to -ENOSYS is necessary because, otherwise, user-issued syscall(-1) 
will
return a bogus value when audit tracing is on.

Please note that, on arm,
  not traced  traced
  --  --
syscall(-1)  aborted OOPs(BUG_ON)
syscall(-3000)   aborted aborted
syscall(1000)ENOSYS  ENOSYS


Two points here:

1. You've found a case which causes a BUG_ON().  Where is the bug report
for this, so the problem can be investigated and resolved?


I think that I mentioned it could also happen on arm somewhere in a talk with 
Will,
but don't remember exactly when.


2. What do you mean by "aborted" ?


I mean that the process will receive SIGILL and get aborted.
A system call number, like -1 and -3000, won't be trapped by *switch* statement
in asm_syscall() and end up with being signaled.


Please, if you find a problem with 32-bit ARM, report it.  Don't hide it,
because hiding it can be a security issue or in the case of BUG_ON(), it
could be a denial of service issue.

As you're part of Linaro, I would have thought you'd be more responsible
in this regard - after all, Linaro is supposed to be about improving the
ARM kernel...  Maybe I got that wrong, and Linaro is actually about
ensuring that the ARM kernel is stuffed full of broken features?


I thought my first priority was on arm64 (and then arm), but now that you and 
Will
seem to want to see the fix first on arm, okey, I will start with arm issue.

Thanks,
-Takahiro AKASHI

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[AKASHI Takahiro]

On 09/01/2014 08:37 PM, Will Deacon wrote:

On Wed, Aug 27, 2014 at 06:55:46AM +0100, AKASHI Takahiro wrote:

On 08/27/2014 02:51 AM, Will Deacon wrote:

On Fri, Aug 22, 2014 at 01:35:17AM +0100, AKASHI Takahiro wrote:

Oops, you're absolutely right. I didn't think of this case.
syscall_trace_enter() should not return a syscallno directly, but always
return -1 if syscallno < 0. (except when secure_computing() returns with -1)
This also implies that tracehook_report_syscall() should also have a return 
value.

Will, is this fine with you?


Well, the first thing that jumps out at me is why this is being done
completely differently for arm64 and arm. I thought adding the new ptrace
requests would reconcile the differences?


I'm not sure what portion of my code you mentioned as "completely different", 
but

1)
setting x0 to -ENOSYS is necessary because, otherwise, user-issued syscall(-1) 
will
return a bogus value when audit tracing is on.

Please note that, on arm,
   not traced  traced
   --  --
syscall(-1)  aborted OOPs(BUG_ON)
syscall(-3000)   aborted aborted
syscall(1000)ENOSYS  ENOSYS

So, anyhow, its a bit difficult and meaningless to mimic these invalid cases.


I'm not suggesting we make ourselves bug-compatible with ARM. Instead, I'd
rather see a series of patches getting the ARM code working correctly,
before we go off doing something different for arm64.


I see.


2)
branching a new label, syscall_trace_return_skip (see entry.S), after 
syscall_trace_enter()
is necessary in order to avoid OOPS in audit_syscall_enter() as we discussed.

Did I make it clear?


Sure. So let's fix ARM, then look at the arm64 port after that. I really
want to avoid divergence in this area.


Okey, I will start with fixing the issue on arm.

-Takahiro AKASHI


Will


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[AKASHI Takahiro]

On 09/01/2014 08:37 PM, Will Deacon wrote:

On Wed, Aug 27, 2014 at 06:55:46AM +0100, AKASHI Takahiro wrote:

On 08/27/2014 02:51 AM, Will Deacon wrote:

On Fri, Aug 22, 2014 at 01:35:17AM +0100, AKASHI Takahiro wrote:

Oops, you're absolutely right. I didn't think of this case.
syscall_trace_enter() should not return a syscallno directly, but always
return -1 if syscallno  0. (except when secure_computing() returns with -1)
This also implies that tracehook_report_syscall() should also have a return 
value.

Will, is this fine with you?


Well, the first thing that jumps out at me is why this is being done
completely differently for arm64 and arm. I thought adding the new ptrace
requests would reconcile the differences?


I'm not sure what portion of my code you mentioned as completely different, 
but

1)
setting x0 to -ENOSYS is necessary because, otherwise, user-issued syscall(-1) 
will
return a bogus value when audit tracing is on.

Please note that, on arm,
   not traced  traced
   --  --
syscall(-1)  aborted OOPs(BUG_ON)
syscall(-3000)   aborted aborted
syscall(1000)ENOSYS  ENOSYS

So, anyhow, its a bit difficult and meaningless to mimic these invalid cases.


I'm not suggesting we make ourselves bug-compatible with ARM. Instead, I'd
rather see a series of patches getting the ARM code working correctly,
before we go off doing something different for arm64.


I see.


2)
branching a new label, syscall_trace_return_skip (see entry.S), after 
syscall_trace_enter()
is necessary in order to avoid OOPS in audit_syscall_enter() as we discussed.

Did I make it clear?


Sure. So let's fix ARM, then look at the arm64 port after that. I really
want to avoid divergence in this area.


Okey, I will start with fixing the issue on arm.

-Takahiro AKASHI


Will


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[AKASHI Takahiro]

On 09/01/2014 08:47 PM, Russell King - ARM Linux wrote:

On Wed, Aug 27, 2014 at 02:55:46PM +0900, AKASHI Takahiro wrote:

1)
setting x0 to -ENOSYS is necessary because, otherwise, user-issued syscall(-1) 
will
return a bogus value when audit tracing is on.

Please note that, on arm,
  not traced  traced
  --  --
syscall(-1)  aborted OOPs(BUG_ON)
syscall(-3000)   aborted aborted
syscall(1000)ENOSYS  ENOSYS


Two points here:

1. You've found a case which causes a BUG_ON().  Where is the bug report
for this, so the problem can be investigated and resolved?


I think that I mentioned it could also happen on arm somewhere in a talk with 
Will,
but don't remember exactly when.


2. What do you mean by aborted ?


I mean that the process will receive SIGILL and get aborted.
A system call number, like -1 and -3000, won't be trapped by *switch* statement
in asm_syscall() and end up with being signaled.


Please, if you find a problem with 32-bit ARM, report it.  Don't hide it,
because hiding it can be a security issue or in the case of BUG_ON(), it
could be a denial of service issue.

As you're part of Linaro, I would have thought you'd be more responsible
in this regard - after all, Linaro is supposed to be about improving the
ARM kernel...  Maybe I got that wrong, and Linaro is actually about
ensuring that the ARM kernel is stuffed full of broken features?


I thought my first priority was on arm64 (and then arm), but now that you and 
Will
seem to want to see the fix first on arm, okey, I will start with arm issue.

Thanks,
-Takahiro AKASHI

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[Russell King - ARM Linux]

On Tue, Sep 02, 2014 at 05:47:29PM +0900, AKASHI Takahiro wrote:
 On 09/01/2014 08:47 PM, Russell King - ARM Linux wrote:
 On Wed, Aug 27, 2014 at 02:55:46PM +0900, AKASHI Takahiro wrote:
 1)
 setting x0 to -ENOSYS is necessary because, otherwise, user-issued 
 syscall(-1) will
 return a bogus value when audit tracing is on.

 Please note that, on arm,
   not traced  traced
   --  --
 syscall(-1)  aborted OOPs(BUG_ON)
 syscall(-3000)   aborted aborted
 syscall(1000)ENOSYS  ENOSYS

 Two points here:

 1. You've found a case which causes a BUG_ON().  Where is the bug report
 for this, so the problem can be investigated and resolved?

 I think that I mentioned it could also happen on arm somewhere in a talk
 with Will, but don't remember exactly when.

Sorry, not good enough.  Please report this bug so it can be investigated
and fixed.

 2. What do you mean by aborted ?

 I mean that the process will receive SIGILL and get aborted.
 A system call number, like -1 and -3000, won't be trapped by *switch*
 statement in asm_syscall() and end up with being signaled.

That is correct behaviour - because numbers greater than 0xf (or
0x9f for OABI - the 0x90 offset on the syscalls on OABI is to
distinguish them from syscalls used by RISC OS) are not intended to
be Linux syscalls per-se.

 Please, if you find a problem with 32-bit ARM, report it.  Don't hide it,
 because hiding it can be a security issue or in the case of BUG_ON(), it
 could be a denial of service issue.

 As you're part of Linaro, I would have thought you'd be more responsible
 in this regard - after all, Linaro is supposed to be about improving the
 ARM kernel...  Maybe I got that wrong, and Linaro is actually about
 ensuring that the ARM kernel is stuffed full of broken features?

 I thought my first priority was on arm64 (and then arm), but now that
 you and Will seem to want to see the fix first on arm, okey, I will
 start with arm issue.

So what you're saying there is that if you find a bug in ARM code, which
everyone is currently using, you can ignore it until you've sorted out
ARM64 which almost no one is using.

This is absurd, and whoever has set your priorities is clearly on drugs.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[Russell King - ARM Linux]

On Tue, Sep 02, 2014 at 10:16:22AM +0100, Russell King - ARM Linux wrote:
 On Tue, Sep 02, 2014 at 05:47:29PM +0900, AKASHI Takahiro wrote:
  On 09/01/2014 08:47 PM, Russell King - ARM Linux wrote:
  On Wed, Aug 27, 2014 at 02:55:46PM +0900, AKASHI Takahiro wrote:
  1)
  setting x0 to -ENOSYS is necessary because, otherwise, user-issued 
  syscall(-1) will
  return a bogus value when audit tracing is on.
 
  Please note that, on arm,
not traced  traced
--  --
  syscall(-1)  aborted OOPs(BUG_ON)
  syscall(-3000)   aborted aborted
  syscall(1000)ENOSYS  ENOSYS
 
  Two points here:
 
  1. You've found a case which causes a BUG_ON().  Where is the bug report
  for this, so the problem can be investigated and resolved?
 
  I think that I mentioned it could also happen on arm somewhere in a talk
  with Will, but don't remember exactly when.
 
 Sorry, not good enough.  Please report this bug so it can be investigated
 and fixed.

I'm going to go further than this, and tell you that you have been
downright irresponsible here, and I'm disgusted by your behaviour over
this.

You have revealed a potential security problem publically, effectively
giving details about how to cause it, but without having first reported
it to people who can fix it, nor providing a fix for it.

Why is it a security problem?  Although it can't be used to gain
information, it can be used potentially to deny service.  Any user can
trace a task which they own, and then set the task's syscall to -1,
which according to you results in a kernel oops.

If the kernel oops happens while holding any locks, that part of the
system becomes non-functional and can result in all userland stopping
dead.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[Russell King - ARM Linux]

On Wed, Aug 27, 2014 at 02:55:46PM +0900, AKASHI Takahiro wrote:
> 1)
> setting x0 to -ENOSYS is necessary because, otherwise, user-issued 
> syscall(-1) will
> return a bogus value when audit tracing is on.
>
> Please note that, on arm,
>  not traced  traced
>  --  --
> syscall(-1)  aborted OOPs(BUG_ON)
> syscall(-3000)   aborted aborted
> syscall(1000)ENOSYS  ENOSYS

Two points here:

1. You've found a case which causes a BUG_ON().  Where is the bug report
   for this, so the problem can be investigated and resolved?

2. What do you mean by "aborted" ?

Please, if you find a problem with 32-bit ARM, report it.  Don't hide it,
because hiding it can be a security issue or in the case of BUG_ON(), it
could be a denial of service issue.

As you're part of Linaro, I would have thought you'd be more responsible
in this regard - after all, Linaro is supposed to be about improving the
ARM kernel...  Maybe I got that wrong, and Linaro is actually about
ensuring that the ARM kernel is stuffed full of broken features?

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[Will Deacon]

On Wed, Aug 27, 2014 at 06:55:46AM +0100, AKASHI Takahiro wrote:
> On 08/27/2014 02:51 AM, Will Deacon wrote:
> > On Fri, Aug 22, 2014 at 01:35:17AM +0100, AKASHI Takahiro wrote:
> >> Oops, you're absolutely right. I didn't think of this case.
> >> syscall_trace_enter() should not return a syscallno directly, but always
> >> return -1 if syscallno < 0. (except when secure_computing() returns with 
> >> -1)
> >> This also implies that tracehook_report_syscall() should also have a 
> >> return value.
> >>
> >> Will, is this fine with you?
> >
> > Well, the first thing that jumps out at me is why this is being done
> > completely differently for arm64 and arm. I thought adding the new ptrace
> > requests would reconcile the differences?
> 
> I'm not sure what portion of my code you mentioned as "completely different", 
> but
> 
> 1)
> setting x0 to -ENOSYS is necessary because, otherwise, user-issued 
> syscall(-1) will
> return a bogus value when audit tracing is on.
> 
> Please note that, on arm,
>   not traced  traced
>   --  --
> syscall(-1)  aborted OOPs(BUG_ON)
> syscall(-3000)   aborted aborted
> syscall(1000)ENOSYS  ENOSYS
> 
> So, anyhow, its a bit difficult and meaningless to mimic these invalid cases.

I'm not suggesting we make ourselves bug-compatible with ARM. Instead, I'd
rather see a series of patches getting the ARM code working correctly,
before we go off doing something different for arm64.

> 2)
> branching a new label, syscall_trace_return_skip (see entry.S), after 
> syscall_trace_enter()
> is necessary in order to avoid OOPS in audit_syscall_enter() as we discussed.
> 
> Did I make it clear?

Sure. So let's fix ARM, then look at the arm64 port after that. I really
want to avoid divergence in this area.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[Will Deacon]

On Wed, Aug 27, 2014 at 06:55:46AM +0100, AKASHI Takahiro wrote:
 On 08/27/2014 02:51 AM, Will Deacon wrote:
  On Fri, Aug 22, 2014 at 01:35:17AM +0100, AKASHI Takahiro wrote:
  Oops, you're absolutely right. I didn't think of this case.
  syscall_trace_enter() should not return a syscallno directly, but always
  return -1 if syscallno  0. (except when secure_computing() returns with 
  -1)
  This also implies that tracehook_report_syscall() should also have a 
  return value.
 
  Will, is this fine with you?
 
  Well, the first thing that jumps out at me is why this is being done
  completely differently for arm64 and arm. I thought adding the new ptrace
  requests would reconcile the differences?
 
 I'm not sure what portion of my code you mentioned as completely different, 
 but
 
 1)
 setting x0 to -ENOSYS is necessary because, otherwise, user-issued 
 syscall(-1) will
 return a bogus value when audit tracing is on.
 
 Please note that, on arm,
   not traced  traced
   --  --
 syscall(-1)  aborted OOPs(BUG_ON)
 syscall(-3000)   aborted aborted
 syscall(1000)ENOSYS  ENOSYS
 
 So, anyhow, its a bit difficult and meaningless to mimic these invalid cases.

I'm not suggesting we make ourselves bug-compatible with ARM. Instead, I'd
rather see a series of patches getting the ARM code working correctly,
before we go off doing something different for arm64.

 2)
 branching a new label, syscall_trace_return_skip (see entry.S), after 
 syscall_trace_enter()
 is necessary in order to avoid OOPS in audit_syscall_enter() as we discussed.
 
 Did I make it clear?

Sure. So let's fix ARM, then look at the arm64 port after that. I really
want to avoid divergence in this area.

Will
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[Russell King - ARM Linux]

On Wed, Aug 27, 2014 at 02:55:46PM +0900, AKASHI Takahiro wrote:
 1)
 setting x0 to -ENOSYS is necessary because, otherwise, user-issued 
 syscall(-1) will
 return a bogus value when audit tracing is on.

 Please note that, on arm,
  not traced  traced
  --  --
 syscall(-1)  aborted OOPs(BUG_ON)
 syscall(-3000)   aborted aborted
 syscall(1000)ENOSYS  ENOSYS

Two points here:

1. You've found a case which causes a BUG_ON().  Where is the bug report
   for this, so the problem can be investigated and resolved?

2. What do you mean by aborted ?

Please, if you find a problem with 32-bit ARM, report it.  Don't hide it,
because hiding it can be a security issue or in the case of BUG_ON(), it
could be a denial of service issue.

As you're part of Linaro, I would have thought you'd be more responsible
in this regard - after all, Linaro is supposed to be about improving the
ARM kernel...  Maybe I got that wrong, and Linaro is actually about
ensuring that the ARM kernel is stuffed full of broken features?

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[AKASHI Takahiro]

On 08/27/2014 02:51 AM, Will Deacon wrote:

On Fri, Aug 22, 2014 at 01:35:17AM +0100, AKASHI Takahiro wrote:

On 08/22/2014 02:08 AM, Kees Cook wrote:

On Thu, Aug 21, 2014 at 3:56 AM, AKASHI Takahiro
 wrote:

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 8876049..c54dbcc 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1121,9 +1121,29 @@ static void tracehook_report_syscall(struct pt_regs 
*regs,

   asmlinkage int syscall_trace_enter(struct pt_regs *regs)
   {
+   unsigned int saved_syscallno = regs->syscallno;
+
  if (test_thread_flag(TIF_SYSCALL_TRACE))
  tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);

+   if (IS_SKIP_SYSCALL(regs->syscallno)) {
+   /*
+* RESTRICTION: we can't modify a return value of user
+* issued syscall(-1) here. In order to ease this flavor,
+* we need to treat whatever value in x0 as a return value,
+* but this might result in a bogus value being returned.
+*/
+   /*
+* NOTE: syscallno may also be set to -1 if fatal signal is
+* detected in tracehook_report_syscall_entry(), but since
+* a value set to x0 here is not used in this case, we may
+* neglect the case.
+*/
+   if (!test_thread_flag(TIF_SYSCALL_TRACE) ||
+   (IS_SKIP_SYSCALL(saved_syscallno)))
+   regs->regs[0] = -ENOSYS;
+   }
+


I don't have a runtime environment yet for arm64, so I can't test this
directly myself, so I'm just trying to eyeball this. :)

Once the seccomp logic is added here, I don't think using -2 as a
special value will work. Doesn't this mean the Oops is possible by the
user issuing a "-2" syscall? As in, if TIF_SYSCALL_WORK is set, and
the user passed -2 as the syscall, audit will be called only on entry,
and then skipped on exit?


Oops, you're absolutely right. I didn't think of this case.
syscall_trace_enter() should not return a syscallno directly, but always
return -1 if syscallno < 0. (except when secure_computing() returns with -1)
This also implies that tracehook_report_syscall() should also have a return 
value.

Will, is this fine with you?


Well, the first thing that jumps out at me is why this is being done
completely differently for arm64 and arm. I thought adding the new ptrace
requests would reconcile the differences?


I'm not sure what portion of my code you mentioned as "completely different", 
but

1)
setting x0 to -ENOSYS is necessary because, otherwise, user-issued syscall(-1) 
will
return a bogus value when audit tracing is on.

Please note that, on arm,
 not traced  traced
 --  --
syscall(-1)  aborted OOPs(BUG_ON)
syscall(-3000)   aborted aborted
syscall(1000)ENOSYS  ENOSYS

So, anyhow, its a bit difficult and meaningless to mimic these invalid cases.

2)
branching a new label, syscall_trace_return_skip (see entry.S), after 
syscall_trace_enter()
is necessary in order to avoid OOPS in audit_syscall_enter() as we discussed.

Did I make it clear?

-Takahiro AKASHI


Will


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[Will Deacon]

On Fri, Aug 22, 2014 at 01:35:17AM +0100, AKASHI Takahiro wrote:
> On 08/22/2014 02:08 AM, Kees Cook wrote:
> > On Thu, Aug 21, 2014 at 3:56 AM, AKASHI Takahiro
> >  wrote:
> >> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> >> index 8876049..c54dbcc 100644
> >> --- a/arch/arm64/kernel/ptrace.c
> >> +++ b/arch/arm64/kernel/ptrace.c
> >> @@ -1121,9 +1121,29 @@ static void tracehook_report_syscall(struct pt_regs 
> >> *regs,
> >>
> >>   asmlinkage int syscall_trace_enter(struct pt_regs *regs)
> >>   {
> >> +   unsigned int saved_syscallno = regs->syscallno;
> >> +
> >>  if (test_thread_flag(TIF_SYSCALL_TRACE))
> >>  tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
> >>
> >> +   if (IS_SKIP_SYSCALL(regs->syscallno)) {
> >> +   /*
> >> +* RESTRICTION: we can't modify a return value of user
> >> +* issued syscall(-1) here. In order to ease this flavor,
> >> +* we need to treat whatever value in x0 as a return value,
> >> +* but this might result in a bogus value being returned.
> >> +*/
> >> +   /*
> >> +* NOTE: syscallno may also be set to -1 if fatal signal is
> >> +* detected in tracehook_report_syscall_entry(), but since
> >> +* a value set to x0 here is not used in this case, we may
> >> +* neglect the case.
> >> +*/
> >> +   if (!test_thread_flag(TIF_SYSCALL_TRACE) ||
> >> +   (IS_SKIP_SYSCALL(saved_syscallno)))
> >> +   regs->regs[0] = -ENOSYS;
> >> +   }
> >> +
> >
> > I don't have a runtime environment yet for arm64, so I can't test this
> > directly myself, so I'm just trying to eyeball this. :)
> >
> > Once the seccomp logic is added here, I don't think using -2 as a
> > special value will work. Doesn't this mean the Oops is possible by the
> > user issuing a "-2" syscall? As in, if TIF_SYSCALL_WORK is set, and
> > the user passed -2 as the syscall, audit will be called only on entry,
> > and then skipped on exit?
> 
> Oops, you're absolutely right. I didn't think of this case.
> syscall_trace_enter() should not return a syscallno directly, but always
> return -1 if syscallno < 0. (except when secure_computing() returns with -1)
> This also implies that tracehook_report_syscall() should also have a return 
> value.
> 
> Will, is this fine with you?

Well, the first thing that jumps out at me is why this is being done
completely differently for arm64 and arm. I thought adding the new ptrace
requests would reconcile the differences?

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[Will Deacon]

On Fri, Aug 22, 2014 at 01:35:17AM +0100, AKASHI Takahiro wrote:
 On 08/22/2014 02:08 AM, Kees Cook wrote:
  On Thu, Aug 21, 2014 at 3:56 AM, AKASHI Takahiro
  takahiro.aka...@linaro.org wrote:
  diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
  index 8876049..c54dbcc 100644
  --- a/arch/arm64/kernel/ptrace.c
  +++ b/arch/arm64/kernel/ptrace.c
  @@ -1121,9 +1121,29 @@ static void tracehook_report_syscall(struct pt_regs 
  *regs,
 
asmlinkage int syscall_trace_enter(struct pt_regs *regs)
{
  +   unsigned int saved_syscallno = regs-syscallno;
  +
   if (test_thread_flag(TIF_SYSCALL_TRACE))
   tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
 
  +   if (IS_SKIP_SYSCALL(regs-syscallno)) {
  +   /*
  +* RESTRICTION: we can't modify a return value of user
  +* issued syscall(-1) here. In order to ease this flavor,
  +* we need to treat whatever value in x0 as a return value,
  +* but this might result in a bogus value being returned.
  +*/
  +   /*
  +* NOTE: syscallno may also be set to -1 if fatal signal is
  +* detected in tracehook_report_syscall_entry(), but since
  +* a value set to x0 here is not used in this case, we may
  +* neglect the case.
  +*/
  +   if (!test_thread_flag(TIF_SYSCALL_TRACE) ||
  +   (IS_SKIP_SYSCALL(saved_syscallno)))
  +   regs-regs[0] = -ENOSYS;
  +   }
  +
 
  I don't have a runtime environment yet for arm64, so I can't test this
  directly myself, so I'm just trying to eyeball this. :)
 
  Once the seccomp logic is added here, I don't think using -2 as a
  special value will work. Doesn't this mean the Oops is possible by the
  user issuing a -2 syscall? As in, if TIF_SYSCALL_WORK is set, and
  the user passed -2 as the syscall, audit will be called only on entry,
  and then skipped on exit?
 
 Oops, you're absolutely right. I didn't think of this case.
 syscall_trace_enter() should not return a syscallno directly, but always
 return -1 if syscallno  0. (except when secure_computing() returns with -1)
 This also implies that tracehook_report_syscall() should also have a return 
 value.
 
 Will, is this fine with you?

Well, the first thing that jumps out at me is why this is being done
completely differently for arm64 and arm. I thought adding the new ptrace
requests would reconcile the differences?

Will
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[AKASHI Takahiro]

On 08/27/2014 02:51 AM, Will Deacon wrote:

On Fri, Aug 22, 2014 at 01:35:17AM +0100, AKASHI Takahiro wrote:

On 08/22/2014 02:08 AM, Kees Cook wrote:

On Thu, Aug 21, 2014 at 3:56 AM, AKASHI Takahiro
takahiro.aka...@linaro.org wrote:

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 8876049..c54dbcc 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1121,9 +1121,29 @@ static void tracehook_report_syscall(struct pt_regs 
*regs,

   asmlinkage int syscall_trace_enter(struct pt_regs *regs)
   {
+   unsigned int saved_syscallno = regs-syscallno;
+
  if (test_thread_flag(TIF_SYSCALL_TRACE))
  tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);

+   if (IS_SKIP_SYSCALL(regs-syscallno)) {
+   /*
+* RESTRICTION: we can't modify a return value of user
+* issued syscall(-1) here. In order to ease this flavor,
+* we need to treat whatever value in x0 as a return value,
+* but this might result in a bogus value being returned.
+*/
+   /*
+* NOTE: syscallno may also be set to -1 if fatal signal is
+* detected in tracehook_report_syscall_entry(), but since
+* a value set to x0 here is not used in this case, we may
+* neglect the case.
+*/
+   if (!test_thread_flag(TIF_SYSCALL_TRACE) ||
+   (IS_SKIP_SYSCALL(saved_syscallno)))
+   regs-regs[0] = -ENOSYS;
+   }
+


I don't have a runtime environment yet for arm64, so I can't test this
directly myself, so I'm just trying to eyeball this. :)

Once the seccomp logic is added here, I don't think using -2 as a
special value will work. Doesn't this mean the Oops is possible by the
user issuing a -2 syscall? As in, if TIF_SYSCALL_WORK is set, and
the user passed -2 as the syscall, audit will be called only on entry,
and then skipped on exit?


Oops, you're absolutely right. I didn't think of this case.
syscall_trace_enter() should not return a syscallno directly, but always
return -1 if syscallno  0. (except when secure_computing() returns with -1)
This also implies that tracehook_report_syscall() should also have a return 
value.

Will, is this fine with you?


Well, the first thing that jumps out at me is why this is being done
completely differently for arm64 and arm. I thought adding the new ptrace
requests would reconcile the differences?


I'm not sure what portion of my code you mentioned as completely different, 
but

1)
setting x0 to -ENOSYS is necessary because, otherwise, user-issued syscall(-1) 
will
return a bogus value when audit tracing is on.

Please note that, on arm,
 not traced  traced
 --  --
syscall(-1)  aborted OOPs(BUG_ON)
syscall(-3000)   aborted aborted
syscall(1000)ENOSYS  ENOSYS

So, anyhow, its a bit difficult and meaningless to mimic these invalid cases.

2)
branching a new label, syscall_trace_return_skip (see entry.S), after 
syscall_trace_enter()
is necessary in order to avoid OOPS in audit_syscall_enter() as we discussed.

Did I make it clear?

-Takahiro AKASHI


Will


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[AKASHI Takahiro]

On 08/22/2014 02:08 AM, Kees Cook wrote:

On Thu, Aug 21, 2014 at 3:56 AM, AKASHI Takahiro
 wrote:

If tracer specifies -1 as a syscall number, this traced system call should
be skipped with a value in x0 used as a return value.
This patch enables this semantics, but there is a restriction here:

when syscall(-1) is issued by user, tracer cannot skip this system call
and modify a return value at syscall entry.

In order to ease this flavor, we need to treat whatever value in x0 as
a return value, but this might result in a bogus value being returned,
especially when tracer doesn't do anything at this syscall.
So we always return ENOSYS instead, while we have another chance to change
a return value at syscall exit.

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

* syscallno may also be set to -1 if a fatal signal (SIGKILL) is detected
   in tracehook_report_syscall_entry(), but since a value set to x0 (ENOSYS)
   is not used in this case, we may neglect the case.

Signed-off-by: AKASHI Takahiro 
---
  arch/arm64/include/asm/ptrace.h |8 
  arch/arm64/kernel/entry.S   |4 
  arch/arm64/kernel/ptrace.c  |   20 
  3 files changed, 32 insertions(+)

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 501000f..a58cf62 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -65,6 +65,14 @@
  #define COMPAT_PT_TEXT_ADDR0x1
  #define COMPAT_PT_DATA_ADDR0x10004
  #define COMPAT_PT_TEXT_END_ADDR0x10008
+
+/*
+ * used to skip a system call when tracer changes its number to -1
+ * with ptrace(PTRACE_SET_SYSCALL)
+ */
+#define RET_SKIP_SYSCALL   -1
+#define IS_SKIP_SYSCALL(no)((int)(no & 0x) == -1)
+
  #ifndef __ASSEMBLY__

  /* sizeof(struct user) for AArch32 */
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index f0b5e51..fdd6eae 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -25,6 +25,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 

@@ -671,6 +672,8 @@ ENDPROC(el0_svc)
  __sys_trace:
 mov x0, sp
 bl  syscall_trace_enter
+   cmp w0, #RET_SKIP_SYSCALL   // skip syscall?
+   b.eq__sys_trace_return_skipped
 adr lr, __sys_trace_return  // return address
 uxtwscno, w0// syscall number (possibly 
new)
 mov x1, sp  // pointer to regs
@@ -685,6 +688,7 @@ __sys_trace:

  __sys_trace_return:
 str x0, [sp]// save returned x0
+__sys_trace_return_skipped:// x0 already in regs[0]
 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 8876049..c54dbcc 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1121,9 +1121,29 @@ static void tracehook_report_syscall(struct pt_regs 
*regs,

  asmlinkage int syscall_trace_enter(struct pt_regs *regs)
  {
+   unsigned int saved_syscallno = regs->syscallno;
+
 if (test_thread_flag(TIF_SYSCALL_TRACE))
 tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);

+   if (IS_SKIP_SYSCALL(regs->syscallno)) {
+   /*
+* RESTRICTION: we can't modify a return value of user
+* issued syscall(-1) here. In order to ease this flavor,
+* we need to treat whatever value in x0 as a return value,
+* but this might result in a bogus value being returned.
+*/
+   /*
+* NOTE: syscallno may also be set to -1 if fatal signal is
+* detected in tracehook_report_syscall_entry(), but since
+* a value set to x0 here is not used in this case, we may
+* neglect the case.
+*/
+   if (!test_thread_flag(TIF_SYSCALL_TRACE) ||
+   (IS_SKIP_SYSCALL(saved_syscallno)))
+   regs->regs[0] = -ENOSYS;
+   }
+


I don't have a runtime environment yet for arm64, so I can't test this
directly myself, so I'm just trying to eyeball this. :)

Once the seccomp logic is added here, I don't think using -2 as a
special value will work. Doesn't this mean the Oops is possible by the
user issuing a "-2" syscall? As in, if TIF_SYSCALL_WORK is set, and
the user passed -2 as the syscall, audit will be called only on entry,
and then skipped on exit?


Oops, you're 

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[Kees Cook]

On Thu, Aug 21, 2014 at 3:56 AM, AKASHI Takahiro
 wrote:
> If tracer specifies -1 as a syscall number, this traced system call should
> be skipped with a value in x0 used as a return value.
> This patch enables this semantics, but there is a restriction here:
>
>when syscall(-1) is issued by user, tracer cannot skip this system call
>and modify a return value at syscall entry.
>
> In order to ease this flavor, we need to treat whatever value in x0 as
> a return value, but this might result in a bogus value being returned,
> especially when tracer doesn't do anything at this syscall.
> So we always return ENOSYS instead, while we have another chance to change
> a return value at syscall exit.
>
> 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().
>
> * syscallno may also be set to -1 if a fatal signal (SIGKILL) is detected
>   in tracehook_report_syscall_entry(), but since a value set to x0 (ENOSYS)
>   is not used in this case, we may neglect the case.
>
> Signed-off-by: AKASHI Takahiro 
> ---
>  arch/arm64/include/asm/ptrace.h |8 
>  arch/arm64/kernel/entry.S   |4 
>  arch/arm64/kernel/ptrace.c  |   20 
>  3 files changed, 32 insertions(+)
>
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index 501000f..a58cf62 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -65,6 +65,14 @@
>  #define COMPAT_PT_TEXT_ADDR0x1
>  #define COMPAT_PT_DATA_ADDR0x10004
>  #define COMPAT_PT_TEXT_END_ADDR0x10008
> +
> +/*
> + * used to skip a system call when tracer changes its number to -1
> + * with ptrace(PTRACE_SET_SYSCALL)
> + */
> +#define RET_SKIP_SYSCALL   -1
> +#define IS_SKIP_SYSCALL(no)((int)(no & 0x) == -1)
> +
>  #ifndef __ASSEMBLY__
>
>  /* sizeof(struct user) for AArch32 */
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index f0b5e51..fdd6eae 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -25,6 +25,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>
> @@ -671,6 +672,8 @@ ENDPROC(el0_svc)
>  __sys_trace:
> mov x0, sp
> bl  syscall_trace_enter
> +   cmp w0, #RET_SKIP_SYSCALL   // skip syscall?
> +   b.eq__sys_trace_return_skipped
> adr lr, __sys_trace_return  // return address
> uxtwscno, w0// syscall number (possibly 
> new)
> mov x1, sp  // pointer to regs
> @@ -685,6 +688,7 @@ __sys_trace:
>
>  __sys_trace_return:
> str x0, [sp]// save returned x0
> +__sys_trace_return_skipped:// x0 already in regs[0]
> 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 8876049..c54dbcc 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -1121,9 +1121,29 @@ static void tracehook_report_syscall(struct pt_regs 
> *regs,
>
>  asmlinkage int syscall_trace_enter(struct pt_regs *regs)
>  {
> +   unsigned int saved_syscallno = regs->syscallno;
> +
> if (test_thread_flag(TIF_SYSCALL_TRACE))
> tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
>
> +   if (IS_SKIP_SYSCALL(regs->syscallno)) {
> +   /*
> +* RESTRICTION: we can't modify a return value of user
> +* issued syscall(-1) here. In order to ease this flavor,
> +* we need to treat whatever value in x0 as a return value,
> +* but this might result in a bogus value being returned.
> +*/
> +   /*
> +* NOTE: syscallno may also be set to -1 if fatal signal is
> +* detected in tracehook_report_syscall_entry(), but since
> +* a value set to x0 here is not used in this case, we may
> +* neglect the case.
> +*/
> +   if (!test_thread_flag(TIF_SYSCALL_TRACE) ||
> +   (IS_SKIP_SYSCALL(saved_syscallno)))
> +   regs->regs[0] = -ENOSYS;
> +   }
> +

I don't have a runtime environment yet for arm64, so I can't test this
directly myself, so I'm just trying to eyeball this. :)

Once the seccomp logic is added here, I don't think using -2 as a
special value will work. Doesn't this mean the Oops is possible by the
user issuing a "-2" syscall? As in, if 

[PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[AKASHI Takahiro]

If tracer specifies -1 as a syscall number, this traced system call should
be skipped with a value in x0 used as a return value.
This patch enables this semantics, but there is a restriction here:

   when syscall(-1) is issued by user, tracer cannot skip this system call
   and modify a return value at syscall entry.

In order to ease this flavor, we need to treat whatever value in x0 as
a return value, but this might result in a bogus value being returned,
especially when tracer doesn't do anything at this syscall.
So we always return ENOSYS instead, while we have another chance to change
a return value at syscall exit.

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

* syscallno may also be set to -1 if a fatal signal (SIGKILL) is detected
  in tracehook_report_syscall_entry(), but since a value set to x0 (ENOSYS)
  is not used in this case, we may neglect the case.

Signed-off-by: AKASHI Takahiro 
---
 arch/arm64/include/asm/ptrace.h |8 
 arch/arm64/kernel/entry.S   |4 
 arch/arm64/kernel/ptrace.c  |   20 
 3 files changed, 32 insertions(+)

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 501000f..a58cf62 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -65,6 +65,14 @@
 #define COMPAT_PT_TEXT_ADDR0x1
 #define COMPAT_PT_DATA_ADDR0x10004
 #define COMPAT_PT_TEXT_END_ADDR0x10008
+
+/*
+ * used to skip a system call when tracer changes its number to -1
+ * with ptrace(PTRACE_SET_SYSCALL)
+ */
+#define RET_SKIP_SYSCALL   -1
+#define IS_SKIP_SYSCALL(no)((int)(no & 0x) == -1)
+
 #ifndef __ASSEMBLY__
 
 /* sizeof(struct user) for AArch32 */
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index f0b5e51..fdd6eae 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -671,6 +672,8 @@ ENDPROC(el0_svc)
 __sys_trace:
mov x0, sp
bl  syscall_trace_enter
+   cmp w0, #RET_SKIP_SYSCALL   // skip syscall?
+   b.eq__sys_trace_return_skipped
adr lr, __sys_trace_return  // return address
uxtwscno, w0// syscall number (possibly new)
mov x1, sp  // pointer to regs
@@ -685,6 +688,7 @@ __sys_trace:
 
 __sys_trace_return:
str x0, [sp]// save returned x0
+__sys_trace_return_skipped:// x0 already in regs[0]
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 8876049..c54dbcc 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1121,9 +1121,29 @@ static void tracehook_report_syscall(struct pt_regs 
*regs,
 
 asmlinkage int syscall_trace_enter(struct pt_regs *regs)
 {
+   unsigned int saved_syscallno = regs->syscallno;
+
if (test_thread_flag(TIF_SYSCALL_TRACE))
tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
 
+   if (IS_SKIP_SYSCALL(regs->syscallno)) {
+   /*
+* RESTRICTION: we can't modify a return value of user
+* issued syscall(-1) here. In order to ease this flavor,
+* we need to treat whatever value in x0 as a return value,
+* but this might result in a bogus value being returned.
+*/
+   /*
+* NOTE: syscallno may also be set to -1 if fatal signal is
+* detected in tracehook_report_syscall_entry(), but since
+* a value set to x0 here is not used in this case, we may
+* neglect the case.
+*/
+   if (!test_thread_flag(TIF_SYSCALL_TRACE) ||
+   (IS_SKIP_SYSCALL(saved_syscallno)))
+   regs->regs[0] = -ENOSYS;
+   }
+
if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
trace_sys_enter(regs, regs->syscallno);
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[AKASHI Takahiro]

If tracer specifies -1 as a syscall number, this traced system call should
be skipped with a value in x0 used as a return value.
This patch enables this semantics, but there is a restriction here:

   when syscall(-1) is issued by user, tracer cannot skip this system call
   and modify a return value at syscall entry.

In order to ease this flavor, we need to treat whatever value in x0 as
a return value, but this might result in a bogus value being returned,
especially when tracer doesn't do anything at this syscall.
So we always return ENOSYS instead, while we have another chance to change
a return value at syscall exit.

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

* syscallno may also be set to -1 if a fatal signal (SIGKILL) is detected
  in tracehook_report_syscall_entry(), but since a value set to x0 (ENOSYS)
  is not used in this case, we may neglect the case.

Signed-off-by: AKASHI Takahiro takahiro.aka...@linaro.org
---
 arch/arm64/include/asm/ptrace.h |8 
 arch/arm64/kernel/entry.S   |4 
 arch/arm64/kernel/ptrace.c  |   20 
 3 files changed, 32 insertions(+)

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 501000f..a58cf62 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -65,6 +65,14 @@
 #define COMPAT_PT_TEXT_ADDR0x1
 #define COMPAT_PT_DATA_ADDR0x10004
 #define COMPAT_PT_TEXT_END_ADDR0x10008
+
+/*
+ * used to skip a system call when tracer changes its number to -1
+ * with ptrace(PTRACE_SET_SYSCALL)
+ */
+#define RET_SKIP_SYSCALL   -1
+#define IS_SKIP_SYSCALL(no)((int)(no  0x) == -1)
+
 #ifndef __ASSEMBLY__
 
 /* sizeof(struct user) for AArch32 */
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index f0b5e51..fdd6eae 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -25,6 +25,7 @@
 #include asm/asm-offsets.h
 #include asm/errno.h
 #include asm/esr.h
+#include asm/ptrace.h
 #include asm/thread_info.h
 #include asm/unistd.h
 
@@ -671,6 +672,8 @@ ENDPROC(el0_svc)
 __sys_trace:
mov x0, sp
bl  syscall_trace_enter
+   cmp w0, #RET_SKIP_SYSCALL   // skip syscall?
+   b.eq__sys_trace_return_skipped
adr lr, __sys_trace_return  // return address
uxtwscno, w0// syscall number (possibly new)
mov x1, sp  // pointer to regs
@@ -685,6 +688,7 @@ __sys_trace:
 
 __sys_trace_return:
str x0, [sp]// save returned x0
+__sys_trace_return_skipped:// x0 already in regs[0]
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 8876049..c54dbcc 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1121,9 +1121,29 @@ static void tracehook_report_syscall(struct pt_regs 
*regs,
 
 asmlinkage int syscall_trace_enter(struct pt_regs *regs)
 {
+   unsigned int saved_syscallno = regs-syscallno;
+
if (test_thread_flag(TIF_SYSCALL_TRACE))
tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
 
+   if (IS_SKIP_SYSCALL(regs-syscallno)) {
+   /*
+* RESTRICTION: we can't modify a return value of user
+* issued syscall(-1) here. In order to ease this flavor,
+* we need to treat whatever value in x0 as a return value,
+* but this might result in a bogus value being returned.
+*/
+   /*
+* NOTE: syscallno may also be set to -1 if fatal signal is
+* detected in tracehook_report_syscall_entry(), but since
+* a value set to x0 here is not used in this case, we may
+* neglect the case.
+*/
+   if (!test_thread_flag(TIF_SYSCALL_TRACE) ||
+   (IS_SKIP_SYSCALL(saved_syscallno)))
+   regs-regs[0] = -ENOSYS;
+   }
+
if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
trace_sys_enter(regs, regs-syscallno);
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[Kees Cook]

On Thu, Aug 21, 2014 at 3:56 AM, AKASHI Takahiro
takahiro.aka...@linaro.org wrote:
 If tracer specifies -1 as a syscall number, this traced system call should
 be skipped with a value in x0 used as a return value.
 This patch enables this semantics, but there is a restriction here:

when syscall(-1) is issued by user, tracer cannot skip this system call
and modify a return value at syscall entry.

 In order to ease this flavor, we need to treat whatever value in x0 as
 a return value, but this might result in a bogus value being returned,
 especially when tracer doesn't do anything at this syscall.
 So we always return ENOSYS instead, while we have another chance to change
 a return value at syscall exit.

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

 * syscallno may also be set to -1 if a fatal signal (SIGKILL) is detected
   in tracehook_report_syscall_entry(), but since a value set to x0 (ENOSYS)
   is not used in this case, we may neglect the case.

 Signed-off-by: AKASHI Takahiro takahiro.aka...@linaro.org
 ---
  arch/arm64/include/asm/ptrace.h |8 
  arch/arm64/kernel/entry.S   |4 
  arch/arm64/kernel/ptrace.c  |   20 
  3 files changed, 32 insertions(+)

 diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
 index 501000f..a58cf62 100644
 --- a/arch/arm64/include/asm/ptrace.h
 +++ b/arch/arm64/include/asm/ptrace.h
 @@ -65,6 +65,14 @@
  #define COMPAT_PT_TEXT_ADDR0x1
  #define COMPAT_PT_DATA_ADDR0x10004
  #define COMPAT_PT_TEXT_END_ADDR0x10008
 +
 +/*
 + * used to skip a system call when tracer changes its number to -1
 + * with ptrace(PTRACE_SET_SYSCALL)
 + */
 +#define RET_SKIP_SYSCALL   -1
 +#define IS_SKIP_SYSCALL(no)((int)(no  0x) == -1)
 +
  #ifndef __ASSEMBLY__

  /* sizeof(struct user) for AArch32 */
 diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
 index f0b5e51..fdd6eae 100644
 --- a/arch/arm64/kernel/entry.S
 +++ b/arch/arm64/kernel/entry.S
 @@ -25,6 +25,7 @@
  #include asm/asm-offsets.h
  #include asm/errno.h
  #include asm/esr.h
 +#include asm/ptrace.h
  #include asm/thread_info.h
  #include asm/unistd.h

 @@ -671,6 +672,8 @@ ENDPROC(el0_svc)
  __sys_trace:
 mov x0, sp
 bl  syscall_trace_enter
 +   cmp w0, #RET_SKIP_SYSCALL   // skip syscall?
 +   b.eq__sys_trace_return_skipped
 adr lr, __sys_trace_return  // return address
 uxtwscno, w0// syscall number (possibly 
 new)
 mov x1, sp  // pointer to regs
 @@ -685,6 +688,7 @@ __sys_trace:

  __sys_trace_return:
 str x0, [sp]// save returned x0
 +__sys_trace_return_skipped:// x0 already in regs[0]
 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 8876049..c54dbcc 100644
 --- a/arch/arm64/kernel/ptrace.c
 +++ b/arch/arm64/kernel/ptrace.c
 @@ -1121,9 +1121,29 @@ static void tracehook_report_syscall(struct pt_regs 
 *regs,

  asmlinkage int syscall_trace_enter(struct pt_regs *regs)
  {
 +   unsigned int saved_syscallno = regs-syscallno;
 +
 if (test_thread_flag(TIF_SYSCALL_TRACE))
 tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);

 +   if (IS_SKIP_SYSCALL(regs-syscallno)) {
 +   /*
 +* RESTRICTION: we can't modify a return value of user
 +* issued syscall(-1) here. In order to ease this flavor,
 +* we need to treat whatever value in x0 as a return value,
 +* but this might result in a bogus value being returned.
 +*/
 +   /*
 +* NOTE: syscallno may also be set to -1 if fatal signal is
 +* detected in tracehook_report_syscall_entry(), but since
 +* a value set to x0 here is not used in this case, we may
 +* neglect the case.
 +*/
 +   if (!test_thread_flag(TIF_SYSCALL_TRACE) ||
 +   (IS_SKIP_SYSCALL(saved_syscallno)))
 +   regs-regs[0] = -ENOSYS;
 +   }
 +

I don't have a runtime environment yet for arm64, so I can't test this
directly myself, so I'm just trying to eyeball this. :)

Once the seccomp logic is added here, I don't think using -2 as a
special value will work. Doesn't this mean the Oops is possible by the
user issuing a -2 syscall? As in, if 

Re: [PATCH v6 2/6] arm64: ptrace: allow tracer to skip a system call

[AKASHI Takahiro]

On 08/22/2014 02:08 AM, Kees Cook wrote:

On Thu, Aug 21, 2014 at 3:56 AM, AKASHI Takahiro
takahiro.aka...@linaro.org wrote:

If tracer specifies -1 as a syscall number, this traced system call should
be skipped with a value in x0 used as a return value.
This patch enables this semantics, but there is a restriction here:

when syscall(-1) is issued by user, tracer cannot skip this system call
and modify a return value at syscall entry.

In order to ease this flavor, we need to treat whatever value in x0 as
a return value, but this might result in a bogus value being returned,
especially when tracer doesn't do anything at this syscall.
So we always return ENOSYS instead, while we have another chance to change
a return value at syscall exit.

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

* syscallno may also be set to -1 if a fatal signal (SIGKILL) is detected
   in tracehook_report_syscall_entry(), but since a value set to x0 (ENOSYS)
   is not used in this case, we may neglect the case.

Signed-off-by: AKASHI Takahiro takahiro.aka...@linaro.org
---
  arch/arm64/include/asm/ptrace.h |8 
  arch/arm64/kernel/entry.S   |4 
  arch/arm64/kernel/ptrace.c  |   20 
  3 files changed, 32 insertions(+)

diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index 501000f..a58cf62 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -65,6 +65,14 @@
  #define COMPAT_PT_TEXT_ADDR0x1
  #define COMPAT_PT_DATA_ADDR0x10004
  #define COMPAT_PT_TEXT_END_ADDR0x10008
+
+/*
+ * used to skip a system call when tracer changes its number to -1
+ * with ptrace(PTRACE_SET_SYSCALL)
+ */
+#define RET_SKIP_SYSCALL   -1
+#define IS_SKIP_SYSCALL(no)((int)(no  0x) == -1)
+
  #ifndef __ASSEMBLY__

  /* sizeof(struct user) for AArch32 */
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index f0b5e51..fdd6eae 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -25,6 +25,7 @@
  #include asm/asm-offsets.h
  #include asm/errno.h
  #include asm/esr.h
+#include asm/ptrace.h
  #include asm/thread_info.h
  #include asm/unistd.h

@@ -671,6 +672,8 @@ ENDPROC(el0_svc)
  __sys_trace:
 mov x0, sp
 bl  syscall_trace_enter
+   cmp w0, #RET_SKIP_SYSCALL   // skip syscall?
+   b.eq__sys_trace_return_skipped
 adr lr, __sys_trace_return  // return address
 uxtwscno, w0// syscall number (possibly 
new)
 mov x1, sp  // pointer to regs
@@ -685,6 +688,7 @@ __sys_trace:

  __sys_trace_return:
 str x0, [sp]// save returned x0
+__sys_trace_return_skipped:// x0 already in regs[0]
 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 8876049..c54dbcc 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1121,9 +1121,29 @@ static void tracehook_report_syscall(struct pt_regs 
*regs,

  asmlinkage int syscall_trace_enter(struct pt_regs *regs)
  {
+   unsigned int saved_syscallno = regs-syscallno;
+
 if (test_thread_flag(TIF_SYSCALL_TRACE))
 tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);

+   if (IS_SKIP_SYSCALL(regs-syscallno)) {
+   /*
+* RESTRICTION: we can't modify a return value of user
+* issued syscall(-1) here. In order to ease this flavor,
+* we need to treat whatever value in x0 as a return value,
+* but this might result in a bogus value being returned.
+*/
+   /*
+* NOTE: syscallno may also be set to -1 if fatal signal is
+* detected in tracehook_report_syscall_entry(), but since
+* a value set to x0 here is not used in this case, we may
+* neglect the case.
+*/
+   if (!test_thread_flag(TIF_SYSCALL_TRACE) ||
+   (IS_SKIP_SYSCALL(saved_syscallno)))
+   regs-regs[0] = -ENOSYS;
+   }
+


I don't have a runtime environment yet for arm64, so I can't test this
directly myself, so I'm just trying to eyeball this. :)

Once the seccomp logic is added here, I don't think using -2 as a
special value will work. Doesn't this mean the Oops is possible by the
user issuing a -2 syscall? As in, if TIF_SYSCALL_WORK