[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[Kees Cook]

An x86 tracer wanting to change the syscall uses PTRACE_SETREGS
(stored to regs->orig_ax), and an ARM tracer uses PTRACE_SET_SYSCALL
(stored to current_thread_info()->syscall). When this happens, the
syscall can change across the call to secure_computing(), since it may
block on tracer notification, and the tracer can then make changes
to the process, before we return from secure_computing(). This
means the code must respect the changed syscall after the
secure_computing() call in syscall_trace_enter(). The same is true
for tracehook_report_syscall_entry() which may also block and change
the syscall.

The x86 code handles this (it expects orig_ax to always be the
desired syscall). In the ARM case, this means we should not be touching
current_thread_info()->syscall after its initial assignment. All failures
should result in a -1 syscall, though.

Based on patch by Ricky Zhou.

Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: Ricky Zhou <rickyz@google.com>
Cc: stable at vger.kernel.org
---
 arch/arm/kernel/ptrace.c |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 0dd3b79b15c3..97bd95f6aa01 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -911,6 +911,7 @@ enum ptrace_syscall_dir {
 static int tracehook_report_syscall(struct pt_regs *regs,
 				    enum ptrace_syscall_dir dir)
 {
+	int ret = 0;
 	unsigned long ip;
 
 	/*
@@ -923,30 +924,35 @@ static int tracehook_report_syscall(struct pt_regs *regs,
 	if (dir == PTRACE_SYSCALL_EXIT)
 		tracehook_report_syscall_exit(regs, 0);
 	else if (tracehook_report_syscall_entry(regs))
-		current_thread_info()->syscall = -1;
+		ret = -1;
 
 	regs->ARM_ip = ip;
-	return current_thread_info()->syscall;
+	return ret;
 }
 
 asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
 {
+	int ret = 0;
+
+	/* set up syscall, which may be changed in secure_computing */
 	current_thread_info()->syscall = scno;
 
 	/* Do the secure computing check first; failures should be fast. */
 	if (secure_computing(scno) == -1)
 		return -1;
 
-	if (test_thread_flag(TIF_SYSCALL_TRACE))
-		scno = tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
+	if (test_thread_flag(TIF_SYSCALL_TRACE) &&
+	    tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER))
+		ret = -1;
 
 	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
-		trace_sys_enter(regs, scno);
+		trace_sys_enter(regs, current_thread_info()->syscall);
 
-	audit_syscall_entry(AUDIT_ARCH_ARM, scno, regs->ARM_r0, regs->ARM_r1,
+	audit_syscall_entry(AUDIT_ARCH_ARM, current_thread_info()->syscall,
+			    regs->ARM_r0, regs->ARM_r1,
 			    regs->ARM_r2, regs->ARM_r3);
 
-	return scno;
+	return ret ?: current_thread_info()->syscall;
 }
 
 asmlinkage void syscall_trace_exit(struct pt_regs *regs)
-- 
1.7.9.5


-- 
Kees Cook
Chrome OS Security

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[Andy Lutomirski]

On Wed, Jun 18, 2014 at 1:27 PM, Kees Cook <keescook@chromium.org> wrote:
> An x86 tracer wanting to change the syscall uses PTRACE_SETREGS
> (stored to regs->orig_ax), and an ARM tracer uses PTRACE_SET_SYSCALL
> (stored to current_thread_info()->syscall). When this happens, the
> syscall can change across the call to secure_computing(), since it may
> block on tracer notification, and the tracer can then make changes
> to the process, before we return from secure_computing(). This
> means the code must respect the changed syscall after the
> secure_computing() call in syscall_trace_enter(). The same is true
> for tracehook_report_syscall_entry() which may also block and change
> the syscall.
>
> The x86 code handles this (it expects orig_ax to always be the
> desired syscall). In the ARM case, this means we should not be touching
> current_thread_info()->syscall after its initial assignment. All failures
> should result in a -1 syscall, though.

This looks sensible.

--Andy

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[Will Deacon]

Hi Kees,

I'm struggling to see the bug in the current code, so apologies if my
questions aren't helpful.

On Wed, Jun 18, 2014 at 09:27:48PM +0100, Kees Cook wrote:
> An x86 tracer wanting to change the syscall uses PTRACE_SETREGS
> (stored to regs->orig_ax), and an ARM tracer uses PTRACE_SET_SYSCALL
> (stored to current_thread_info()->syscall). When this happens, the
> syscall can change across the call to secure_computing(), since it may
> block on tracer notification, and the tracer can then make changes
> to the process, before we return from secure_computing(). This
> means the code must respect the changed syscall after the
> secure_computing() call in syscall_trace_enter(). The same is true
> for tracehook_report_syscall_entry() which may also block and change
> the syscall.

I don't think I understand what you mean by `the code must respect the
changed syscall'. The current code does indeed issue the new syscall, so are
you more concerned with secure_computing changing ->syscall, then the
debugger can't see the new syscall when it sees the trap from tracehook?
Are these even supposed to inter-operate?

> The x86 code handles this (it expects orig_ax to always be the
> desired syscall). In the ARM case, this means we should not be touching
> current_thread_info()->syscall after its initial assignment. All failures
> should result in a -1 syscall, though.

The only time we explicitly touch ->syscall is when we're aborting the call
(i.e. writing -1), which I think is fine.

Will

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[Kees Cook]

On Fri, Jun 20, 2014 at 3:22 AM, Will Deacon <will.deacon@arm.com> wrote:
> Hi Kees,
>
> I'm struggling to see the bug in the current code, so apologies if my
> questions aren't helpful.
>
> On Wed, Jun 18, 2014 at 09:27:48PM +0100, Kees Cook wrote:
>> An x86 tracer wanting to change the syscall uses PTRACE_SETREGS
>> (stored to regs->orig_ax), and an ARM tracer uses PTRACE_SET_SYSCALL
>> (stored to current_thread_info()->syscall). When this happens, the
>> syscall can change across the call to secure_computing(), since it may
>> block on tracer notification, and the tracer can then make changes
>> to the process, before we return from secure_computing(). This
>> means the code must respect the changed syscall after the
>> secure_computing() call in syscall_trace_enter(). The same is true
>> for tracehook_report_syscall_entry() which may also block and change
>> the syscall.
>
> I don't think I understand what you mean by `the code must respect the
> changed syscall'. The current code does indeed issue the new syscall, so are
> you more concerned with secure_computing changing ->syscall, then the
> debugger can't see the new syscall when it sees the trap from tracehook?
> Are these even supposed to inter-operate?

The problem is the use of "scno" in the call -- it results in ignoring
the value that may be set up in ->syscall by a tracer:

syscall_trace_enter(regs, scno):
    current_thread_info()->syscall = scno;
    secure_computing(scno):
        [block on ptrace]
        [ptracer changes current_thread_info()->syscall vis PTRACE_SET_SYSCALL]
    ...
    return scno;

This means the tracer's changed syscall value will be ignored
(syscall_trace_enter returns original "scno" instead of actual
current_thread_info()->syscall). In the original code, failure cases
were propagated correctly, but not tracer-induced changes.

Is that more clear? It's not an obvious state (due to the external
modification of process state during the ptrace blocking). I've also
got tests for this, if that's useful to further illustrate:

https://github.com/kees/seccomp/commit/bd24e174593f79784b97178b583f17e0ea9d2aa7

>
>> The x86 code handles this (it expects orig_ax to always be the
>> desired syscall). In the ARM case, this means we should not be touching
>> current_thread_info()->syscall after its initial assignment. All failures
>> should result in a -1 syscall, though.
>
> The only time we explicitly touch ->syscall is when we're aborting the call
> (i.e. writing -1), which I think is fine.

That was the error path, which was okay. This rearranges things to
both handle error conditions and tracer changes.

-Kees

-- 
Kees Cook
Chrome OS Security

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[Will Deacon]

On Fri, Jun 20, 2014 at 05:44:52PM +0100, Kees Cook wrote:
> On Fri, Jun 20, 2014 at 3:22 AM, Will Deacon <will.deacon@arm.com> wrote:
> > I'm struggling to see the bug in the current code, so apologies if my
> > questions aren't helpful.
> >
> > On Wed, Jun 18, 2014 at 09:27:48PM +0100, Kees Cook wrote:
> >> An x86 tracer wanting to change the syscall uses PTRACE_SETREGS
> >> (stored to regs->orig_ax), and an ARM tracer uses PTRACE_SET_SYSCALL
> >> (stored to current_thread_info()->syscall). When this happens, the
> >> syscall can change across the call to secure_computing(), since it may
> >> block on tracer notification, and the tracer can then make changes
> >> to the process, before we return from secure_computing(). This
> >> means the code must respect the changed syscall after the
> >> secure_computing() call in syscall_trace_enter(). The same is true
> >> for tracehook_report_syscall_entry() which may also block and change
> >> the syscall.
> >
> > I don't think I understand what you mean by `the code must respect the
> > changed syscall'. The current code does indeed issue the new syscall, so are
> > you more concerned with secure_computing changing ->syscall, then the
> > debugger can't see the new syscall when it sees the trap from tracehook?
> > Are these even supposed to inter-operate?
> 
> The problem is the use of "scno" in the call -- it results in ignoring
> the value that may be set up in ->syscall by a tracer:
> 
> syscall_trace_enter(regs, scno):
>     current_thread_info()->syscall = scno;
>     secure_computing(scno):
>         [block on ptrace]
>         [ptracer changes current_thread_info()->syscall vis PTRACE_SET_SYSCALL]
>     ...
>     return scno;
> 
> This means the tracer's changed syscall value will be ignored
> (syscall_trace_enter returns original "scno" instead of actual
> current_thread_info()->syscall). In the original code, failure cases
> were propagated correctly, but not tracer-induced changes.
> 
> Is that more clear? It's not an obvious state (due to the external
> modification of process state during the ptrace blocking). I've also
> got tests for this, if that's useful to further illustrate:
> 
> https://github.com/kees/seccomp/commit/bd24e174593f79784b97178b583f17e0ea9d2aa7

Right, gotcha. Thanks for the explanation. I was confused, because
tracehook_report_syscall does the right thing (returns
current_thread_info()->syscall), but if we don't have TIF_SYSCALL_TRACE set,
then updates during the secure_computing callback will be ignored.

However, my fix to this is significantly smaller than your patch, so I fear
I'm still missing something.

Will

--->8

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 0dd3b79b15c3..0c27ed6f3f23 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -908,7 +908,7 @@ enum ptrace_syscall_dir {
        PTRACE_SYSCALL_EXIT,
 };
 
-static int tracehook_report_syscall(struct pt_regs *regs,
+static void tracehook_report_syscall(struct pt_regs *regs,
                                    enum ptrace_syscall_dir dir)
 {
        unsigned long ip;
@@ -926,7 +926,6 @@ static int tracehook_report_syscall(struct pt_regs *regs,
                current_thread_info()->syscall = -1;
 
        regs->ARM_ip = ip;
-       return current_thread_info()->syscall;
 }
 
 asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
@@ -938,7 +937,9 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
                return -1;
 
        if (test_thread_flag(TIF_SYSCALL_TRACE))
-               scno = tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
+               tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
+
+       scno = current_thread_info()->syscall;
 
        if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
                trace_sys_enter(regs, scno);

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[Kees Cook]

On Fri, Jun 20, 2014 at 10:23 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Jun 20, 2014 at 05:44:52PM +0100, Kees Cook wrote:
>> On Fri, Jun 20, 2014 at 3:22 AM, Will Deacon <will.deacon@arm.com> wrote:
>> > I'm struggling to see the bug in the current code, so apologies if my
>> > questions aren't helpful.
>> >
>> > On Wed, Jun 18, 2014 at 09:27:48PM +0100, Kees Cook wrote:
>> >> An x86 tracer wanting to change the syscall uses PTRACE_SETREGS
>> >> (stored to regs->orig_ax), and an ARM tracer uses PTRACE_SET_SYSCALL
>> >> (stored to current_thread_info()->syscall). When this happens, the
>> >> syscall can change across the call to secure_computing(), since it may
>> >> block on tracer notification, and the tracer can then make changes
>> >> to the process, before we return from secure_computing(). This
>> >> means the code must respect the changed syscall after the
>> >> secure_computing() call in syscall_trace_enter(). The same is true
>> >> for tracehook_report_syscall_entry() which may also block and change
>> >> the syscall.
>> >
>> > I don't think I understand what you mean by `the code must respect the
>> > changed syscall'. The current code does indeed issue the new syscall, so are
>> > you more concerned with secure_computing changing ->syscall, then the
>> > debugger can't see the new syscall when it sees the trap from tracehook?
>> > Are these even supposed to inter-operate?
>>
>> The problem is the use of "scno" in the call -- it results in ignoring
>> the value that may be set up in ->syscall by a tracer:
>>
>> syscall_trace_enter(regs, scno):
>>     current_thread_info()->syscall = scno;
>>     secure_computing(scno):
>>         [block on ptrace]
>>         [ptracer changes current_thread_info()->syscall vis PTRACE_SET_SYSCALL]
>>     ...
>>     return scno;
>>
>> This means the tracer's changed syscall value will be ignored
>> (syscall_trace_enter returns original "scno" instead of actual
>> current_thread_info()->syscall). In the original code, failure cases
>> were propagated correctly, but not tracer-induced changes.
>>
>> Is that more clear? It's not an obvious state (due to the external
>> modification of process state during the ptrace blocking). I've also
>> got tests for this, if that's useful to further illustrate:
>>
>> https://github.com/kees/seccomp/commit/bd24e174593f79784b97178b583f17e0ea9d2aa7
>
> Right, gotcha. Thanks for the explanation. I was confused, because
> tracehook_report_syscall does the right thing (returns
> current_thread_info()->syscall), but if we don't have TIF_SYSCALL_TRACE set,
> then updates during the secure_computing callback will be ignored.
>
> However, my fix to this is significantly smaller than your patch, so I fear
> I'm still missing something.

Oh, yes, that's much smaller. Nice! I will test this and report back.

>
> Will
>
> --->8
>
> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
> index 0dd3b79b15c3..0c27ed6f3f23 100644
> --- a/arch/arm/kernel/ptrace.c
> +++ b/arch/arm/kernel/ptrace.c
> @@ -908,7 +908,7 @@ enum ptrace_syscall_dir {
>         PTRACE_SYSCALL_EXIT,
>  };
>
> -static int tracehook_report_syscall(struct pt_regs *regs,
> +static void tracehook_report_syscall(struct pt_regs *regs,
>                                     enum ptrace_syscall_dir dir)
>  {
>         unsigned long ip;
> @@ -926,7 +926,6 @@ static int tracehook_report_syscall(struct pt_regs *regs,
>                 current_thread_info()->syscall = -1;
>
>         regs->ARM_ip = ip;
> -       return current_thread_info()->syscall;
>  }
>
>  asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
> @@ -938,7 +937,9 @@ asmlinkage int syscall_trace_enter(struct pt_regs *regs, int scno)
>                 return -1;
>
>         if (test_thread_flag(TIF_SYSCALL_TRACE))
> -               scno = tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
> +               tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
> +
> +       scno = current_thread_info()->syscall;

Perhaps it'd be worth adding a comment above this line just for people
looking at this in the future. Something like:

/* secure_computing and tracehook_report_syscall may have changed syscall */

-Kees

>
>         if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
>                 trace_sys_enter(regs, scno);



-- 
Kees Cook
Chrome OS Security

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[Kees Cook]

On Fri, Jun 20, 2014 at 10:36 AM, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Jun 20, 2014 at 10:23 AM, Will Deacon <will.deacon@arm.com> wrote:
>> On Fri, Jun 20, 2014 at 05:44:52PM +0100, Kees Cook wrote:
>>> On Fri, Jun 20, 2014 at 3:22 AM, Will Deacon <will.deacon@arm.com> wrote:
>>> > I'm struggling to see the bug in the current code, so apologies if my
>>> > questions aren't helpful.
>>> >
>>> > On Wed, Jun 18, 2014 at 09:27:48PM +0100, Kees Cook wrote:
>>> >> An x86 tracer wanting to change the syscall uses PTRACE_SETREGS
>>> >> (stored to regs->orig_ax), and an ARM tracer uses PTRACE_SET_SYSCALL
>>> >> (stored to current_thread_info()->syscall). When this happens, the
>>> >> syscall can change across the call to secure_computing(), since it may
>>> >> block on tracer notification, and the tracer can then make changes
>>> >> to the process, before we return from secure_computing(). This
>>> >> means the code must respect the changed syscall after the
>>> >> secure_computing() call in syscall_trace_enter(). The same is true
>>> >> for tracehook_report_syscall_entry() which may also block and change
>>> >> the syscall.
>>> >
>>> > I don't think I understand what you mean by `the code must respect the
>>> > changed syscall'. The current code does indeed issue the new syscall, so are
>>> > you more concerned with secure_computing changing ->syscall, then the
>>> > debugger can't see the new syscall when it sees the trap from tracehook?
>>> > Are these even supposed to inter-operate?
>>>
>>> The problem is the use of "scno" in the call -- it results in ignoring
>>> the value that may be set up in ->syscall by a tracer:
>>>
>>> syscall_trace_enter(regs, scno):
>>>     current_thread_info()->syscall = scno;
>>>     secure_computing(scno):
>>>         [block on ptrace]
>>>         [ptracer changes current_thread_info()->syscall vis PTRACE_SET_SYSCALL]
>>>     ...
>>>     return scno;
>>>
>>> This means the tracer's changed syscall value will be ignored
>>> (syscall_trace_enter returns original "scno" instead of actual
>>> current_thread_info()->syscall). In the original code, failure cases
>>> were propagated correctly, but not tracer-induced changes.
>>>
>>> Is that more clear? It's not an obvious state (due to the external
>>> modification of process state during the ptrace blocking). I've also
>>> got tests for this, if that's useful to further illustrate:
>>>
>>> https://github.com/kees/seccomp/commit/bd24e174593f79784b97178b583f17e0ea9d2aa7
>>
>> Right, gotcha. Thanks for the explanation. I was confused, because
>> tracehook_report_syscall does the right thing (returns
>> current_thread_info()->syscall), but if we don't have TIF_SYSCALL_TRACE set,
>> then updates during the secure_computing callback will be ignored.
>>
>> However, my fix to this is significantly smaller than your patch, so I fear
>> I'm still missing something.
>
> Oh, yes, that's much smaller. Nice! I will test this and report back.

Yup, I can confirm this works. Thanks!

Tested-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Chrome OS Security

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[Will Deacon]

On Fri, Jun 20, 2014 at 07:10:46PM +0100, Kees Cook wrote:
> On Fri, Jun 20, 2014 at 10:36 AM, Kees Cook <keescook@chromium.org> wrote:
> > On Fri, Jun 20, 2014 at 10:23 AM, Will Deacon <will.deacon@arm.com> wrote:
> >> Right, gotcha. Thanks for the explanation. I was confused, because
> >> tracehook_report_syscall does the right thing (returns
> >> current_thread_info()->syscall), but if we don't have TIF_SYSCALL_TRACE set,
> >> then updates during the secure_computing callback will be ignored.
> >>
> >> However, my fix to this is significantly smaller than your patch, so I fear
> >> I'm still missing something.
> >
> > Oh, yes, that's much smaller. Nice! I will test this and report back.
> 
> Yup, I can confirm this works. Thanks!
> 
> Tested-by: Kees Cook <keescook@chromium.org>

Thanks, Kees. I'll post a patch shortly. I'll try and remember to keep an
eye out for this when seccomp lands for arm64 too.

Will

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[Kees Cook]

On Mon, Jun 23, 2014 at 1:46 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Jun 20, 2014 at 07:10:46PM +0100, Kees Cook wrote:
>> On Fri, Jun 20, 2014 at 10:36 AM, Kees Cook <keescook@chromium.org> wrote:
>> > On Fri, Jun 20, 2014 at 10:23 AM, Will Deacon <will.deacon@arm.com> wrote:
>> >> Right, gotcha. Thanks for the explanation. I was confused, because
>> >> tracehook_report_syscall does the right thing (returns
>> >> current_thread_info()->syscall), but if we don't have TIF_SYSCALL_TRACE set,
>> >> then updates during the secure_computing callback will be ignored.
>> >>
>> >> However, my fix to this is significantly smaller than your patch, so I fear
>> >> I'm still missing something.
>> >
>> > Oh, yes, that's much smaller. Nice! I will test this and report back.
>>
>> Yup, I can confirm this works. Thanks!
>>
>> Tested-by: Kees Cook <keescook@chromium.org>
>
> Thanks, Kees. I'll post a patch shortly. I'll try and remember to keep an
> eye out for this when seccomp lands for arm64 too.

Great, thanks!

What's the state of seccomp on arm64? I saw a series back in March,
but nothing since then? It looked complete, but I haven't set up a
test environment yet to verify.

-Kees

-- 
Kees Cook
Chrome OS Security

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[Will Deacon]

On Mon, Jun 23, 2014 at 08:46:52PM +0100, Kees Cook wrote:
> On Mon, Jun 23, 2014 at 1:46 AM, Will Deacon <will.deacon@arm.com> wrote:
> > On Fri, Jun 20, 2014 at 07:10:46PM +0100, Kees Cook wrote:
> >> On Fri, Jun 20, 2014 at 10:36 AM, Kees Cook <keescook@chromium.org> wrote:
> >> > On Fri, Jun 20, 2014 at 10:23 AM, Will Deacon <will.deacon@arm.com> wrote:
> >> >> Right, gotcha. Thanks for the explanation. I was confused, because
> >> >> tracehook_report_syscall does the right thing (returns
> >> >> current_thread_info()->syscall), but if we don't have TIF_SYSCALL_TRACE set,
> >> >> then updates during the secure_computing callback will be ignored.
> >> >>
> >> >> However, my fix to this is significantly smaller than your patch, so I fear
> >> >> I'm still missing something.
> >> >
> >> > Oh, yes, that's much smaller. Nice! I will test this and report back.
> >>
> >> Yup, I can confirm this works. Thanks!
> >>
> >> Tested-by: Kees Cook <keescook@chromium.org>
> >
> > Thanks, Kees. I'll post a patch shortly. I'll try and remember to keep an
> > eye out for this when seccomp lands for arm64 too.
> 
> Great, thanks!
> 
> What's the state of seccomp on arm64? I saw a series back in March,
> but nothing since then? It looked complete, but I haven't set up a
> test environment yet to verify.

I think Akashi was going to repost `real soon now' so we can include them
for 3.17. He missed the merge window last time around.

Will

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[AKASHI Takahiro]

On 06/24/2014 05:54 PM, Will Deacon wrote:
> On Mon, Jun 23, 2014 at 08:46:52PM +0100, Kees Cook wrote:
>> On Mon, Jun 23, 2014 at 1:46 AM, Will Deacon <will.deacon@arm.com> wrote:
>>> On Fri, Jun 20, 2014 at 07:10:46PM +0100, Kees Cook wrote:
>>>> On Fri, Jun 20, 2014 at 10:36 AM, Kees Cook <keescook@chromium.org> wrote:
>>>>> On Fri, Jun 20, 2014 at 10:23 AM, Will Deacon <will.deacon@arm.com> wrote:
>>>>>> Right, gotcha. Thanks for the explanation. I was confused, because
>>>>>> tracehook_report_syscall does the right thing (returns
>>>>>> current_thread_info()->syscall), but if we don't have TIF_SYSCALL_TRACE set,
>>>>>> then updates during the secure_computing callback will be ignored.
>>>>>>
>>>>>> However, my fix to this is significantly smaller than your patch, so I fear
>>>>>> I'm still missing something.
>>>>>
>>>>> Oh, yes, that's much smaller. Nice! I will test this and report back.
>>>>
>>>> Yup, I can confirm this works. Thanks!
>>>>
>>>> Tested-by: Kees Cook <keescook@chromium.org>
>>>
>>> Thanks, Kees. I'll post a patch shortly. I'll try and remember to keep an
>>> eye out for this when seccomp lands for arm64 too.
>>
>> Great, thanks!
>>
>> What's the state of seccomp on arm64? I saw a series back in March,
>> but nothing since then? It looked complete, but I haven't set up a
>> test environment yet to verify.
>
> I think Akashi was going to repost `real soon now' so we can include them
> for 3.17. He missed the merge window last time around.

Do I really have to repost my patch even without any changes since the last one?
Just kidding. I will do so once I confirm the issue discussed here.
(I've finally found out a user of my patch :)

-Takahiro AKASHI

> Will
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[AKASHI Takahiro]

Hi Will,

On 06/24/2014 05:54 PM, Will Deacon wrote:
> On Mon, Jun 23, 2014 at 08:46:52PM +0100, Kees Cook wrote:
>> On Mon, Jun 23, 2014 at 1:46 AM, Will Deacon <will.deacon@arm.com> wrote:
>>> On Fri, Jun 20, 2014 at 07:10:46PM +0100, Kees Cook wrote:
>>>> On Fri, Jun 20, 2014 at 10:36 AM, Kees Cook <keescook@chromium.org> wrote:
>>>>> On Fri, Jun 20, 2014 at 10:23 AM, Will Deacon <will.deacon@arm.com> wrote:
>>>>>> Right, gotcha. Thanks for the explanation. I was confused, because
>>>>>> tracehook_report_syscall does the right thing (returns
>>>>>> current_thread_info()->syscall), but if we don't have TIF_SYSCALL_TRACE set,
>>>>>> then updates during the secure_computing callback will be ignored.re
>>>>>>
>>>>>> However, my fix to this is significantly smaller than your patch, so I fear
>>>>>> I'm still missing something.
>>>>>
>>>>> Oh, yes, that's much smaller. Nice! I will test this and report back.
>>>>
>>>> Yup, I can confirm this works. Thanks!
>>>>
>>>> Tested-by: Kees Cook <keescook@chromium.org>
>>>
>>> Thanks, Kees. I'll post a patch shortly. I'll try and remember to keep an
>>> eye out for this when seccomp lands for arm64 too.
>>
>> Great, thanks!
>>
>> What's the state of seccomp on arm64? I saw a series back in March,
>> but nothing since then? It looked complete, but I haven't set up a
>> test environment yet to verify.
>
> I think Akashi was going to repost `real soon now' so we can include them
> for 3.17. He missed the merge window last time around.

I took a quick look at the current implementation of ptrace.
ptrace(PTRACE_GETREGSET/SETREGSET), eventually gpr_get/set(), handles only 'struct user_pt_regs',
and we have no way to modify orig_x0 nor syscallno in 'struct pt_regs' directly.
So it seems to me that we can't change a system call by ptrace().
Do I misunderstand anything?

-Takahiro AKASHI


> Will
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[Will Deacon]

On Thu, Jul 03, 2014 at 08:43:07AM +0100, AKASHI Takahiro wrote:
> Hi Will,

Hi Akashi,

> On 06/24/2014 05:54 PM, Will Deacon wrote:
> > On Mon, Jun 23, 2014 at 08:46:52PM +0100, Kees Cook wrote:
> >> What's the state of seccomp on arm64? I saw a series back in March,
> >> but nothing since then? It looked complete, but I haven't set up a
> >> test environment yet to verify.
> >
> > I think Akashi was going to repost `real soon now' so we can include them
> > for 3.17. He missed the merge window last time around.
> 
> I took a quick look at the current implementation of ptrace.
> ptrace(PTRACE_GETREGSET/SETREGSET), eventually gpr_get/set(), handles only
> 'struct user_pt_regs', and we have no way to modify orig_x0 nor syscallno
> in 'struct pt_regs' directly.
> So it seems to me that we can't change a system call by ptrace().
> Do I misunderstand anything?

No, it looks like you have a point here. I don't think userspace has any
business with orig_x0, but changing syscallno is certainly useful. I can
think of two ways to fix this:

  (1) Updating syscallno based on w8, but this ties us to the current ABI
      and could get messy if this register changes in the future.

  (2) Adding a PTRACE_SET_SYSCALL request, like we have for arch/arm/,
      but that means adding arch-specific stuff to arch_ptrace (which
      currently goes straight to ptrace_request on arm64).

It looks like x86 uses orig_ax, which I *think* means we would go with
(1) above if we followed their lead.

Anybody else have an opinion about this?

Will

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[Andy Lutomirski]

On Thu, Jul 3, 2014 at 3:24 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Jul 03, 2014 at 08:43:07AM +0100, AKASHI Takahiro wrote:
>> Hi Will,
>
> Hi Akashi,
>
>> On 06/24/2014 05:54 PM, Will Deacon wrote:
>> > On Mon, Jun 23, 2014 at 08:46:52PM +0100, Kees Cook wrote:
>> >> What's the state of seccomp on arm64? I saw a series back in March,
>> >> but nothing since then? It looked complete, but I haven't set up a
>> >> test environment yet to verify.
>> >
>> > I think Akashi was going to repost `real soon now' so we can include them
>> > for 3.17. He missed the merge window last time around.
>>
>> I took a quick look at the current implementation of ptrace.
>> ptrace(PTRACE_GETREGSET/SETREGSET), eventually gpr_get/set(), handles only
>> 'struct user_pt_regs', and we have no way to modify orig_x0 nor syscallno
>> in 'struct pt_regs' directly.
>> So it seems to me that we can't change a system call by ptrace().
>> Do I misunderstand anything?
>
> No, it looks like you have a point here. I don't think userspace has any
> business with orig_x0, but changing syscallno is certainly useful. I can
> think of two ways to fix this:
>
>   (1) Updating syscallno based on w8, but this ties us to the current ABI
>       and could get messy if this register changes in the future.
>
>   (2) Adding a PTRACE_SET_SYSCALL request, like we have for arch/arm/,
>       but that means adding arch-specific stuff to arch_ptrace (which
>       currently goes straight to ptrace_request on arm64).
>
> It looks like x86 uses orig_ax, which I *think* means we would go with
> (1) above if we followed their lead.

w8 is a real register, right?  On x86, at least orig_ax isn't a real
register, so it's quite unlikely to conflict with hardware stuff.

On x86, the "user_struct" thing has nothing to do with any real kernel
data structure, so it's extensible.  Can you just add syscallno to it?

--Andy

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[Will Deacon]

On Thu, Jul 03, 2014 at 04:39:21PM +0100, Andy Lutomirski wrote:
> On Thu, Jul 3, 2014 at 3:24 AM, Will Deacon <will.deacon@arm.com> wrote:
> > On Thu, Jul 03, 2014 at 08:43:07AM +0100, AKASHI Takahiro wrote:
> >> On 06/24/2014 05:54 PM, Will Deacon wrote:
> >> > On Mon, Jun 23, 2014 at 08:46:52PM +0100, Kees Cook wrote:
> >> >> What's the state of seccomp on arm64? I saw a series back in March,
> >> >> but nothing since then? It looked complete, but I haven't set up a
> >> >> test environment yet to verify.
> >> >
> >> > I think Akashi was going to repost `real soon now' so we can include them
> >> > for 3.17. He missed the merge window last time around.
> >>
> >> I took a quick look at the current implementation of ptrace.
> >> ptrace(PTRACE_GETREGSET/SETREGSET), eventually gpr_get/set(), handles only
> >> 'struct user_pt_regs', and we have no way to modify orig_x0 nor syscallno
> >> in 'struct pt_regs' directly.
> >> So it seems to me that we can't change a system call by ptrace().
> >> Do I misunderstand anything?
> >
> > No, it looks like you have a point here. I don't think userspace has any
> > business with orig_x0, but changing syscallno is certainly useful. I can
> > think of two ways to fix this:
> >
> >   (1) Updating syscallno based on w8, but this ties us to the current ABI
> >       and could get messy if this register changes in the future.
> >
> >   (2) Adding a PTRACE_SET_SYSCALL request, like we have for arch/arm/,
> >       but that means adding arch-specific stuff to arch_ptrace (which
> >       currently goes straight to ptrace_request on arm64).
> >
> > It looks like x86 uses orig_ax, which I *think* means we would go with
> > (1) above if we followed their lead.
> 
> w8 is a real register, right?  On x86, at least orig_ax isn't a real
> register, so it's quite unlikely to conflict with hardware stuff.

Yeah, w8 is the hardware register which the Linux ABI uses for the system
call number. I was thinking We could allow the debugger/tracer to update
the syscall number by updating that register, or do you see an issue with
that? (other than tying us to the current ABI).

> On x86, the "user_struct" thing has nothing to do with any real kernel
> data structure, so it's extensible.  Can you just add syscallno to it?

I'm really not keen on changing user-facing structures like that. For
example, KVM embeds user_pt_regs into kvm_regs.

We can add a new ptrace request if we have to.

Will

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[Andy Lutomirski]

On Thu, Jul 3, 2014 at 9:11 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Jul 03, 2014 at 04:39:21PM +0100, Andy Lutomirski wrote:
>> On Thu, Jul 3, 2014 at 3:24 AM, Will Deacon <will.deacon@arm.com> wrote:
>> > On Thu, Jul 03, 2014 at 08:43:07AM +0100, AKASHI Takahiro wrote:
>> >> On 06/24/2014 05:54 PM, Will Deacon wrote:
>> >> > On Mon, Jun 23, 2014 at 08:46:52PM +0100, Kees Cook wrote:
>> >> >> What's the state of seccomp on arm64? I saw a series back in March,
>> >> >> but nothing since then? It looked complete, but I haven't set up a
>> >> >> test environment yet to verify.
>> >> >
>> >> > I think Akashi was going to repost `real soon now' so we can include them
>> >> > for 3.17. He missed the merge window last time around.
>> >>
>> >> I took a quick look at the current implementation of ptrace.
>> >> ptrace(PTRACE_GETREGSET/SETREGSET), eventually gpr_get/set(), handles only
>> >> 'struct user_pt_regs', and we have no way to modify orig_x0 nor syscallno
>> >> in 'struct pt_regs' directly.
>> >> So it seems to me that we can't change a system call by ptrace().
>> >> Do I misunderstand anything?
>> >
>> > No, it looks like you have a point here. I don't think userspace has any
>> > business with orig_x0, but changing syscallno is certainly useful. I can
>> > think of two ways to fix this:
>> >
>> >   (1) Updating syscallno based on w8, but this ties us to the current ABI
>> >       and could get messy if this register changes in the future.
>> >
>> >   (2) Adding a PTRACE_SET_SYSCALL request, like we have for arch/arm/,
>> >       but that means adding arch-specific stuff to arch_ptrace (which
>> >       currently goes straight to ptrace_request on arm64).
>> >
>> > It looks like x86 uses orig_ax, which I *think* means we would go with
>> > (1) above if we followed their lead.
>>
>> w8 is a real register, right?  On x86, at least orig_ax isn't a real
>> register, so it's quite unlikely to conflict with hardware stuff.
>
> Yeah, w8 is the hardware register which the Linux ABI uses for the system
> call number. I was thinking We could allow the debugger/tracer to update
> the syscall number by updating that register, or do you see an issue with
> that? (other than tying us to the current ABI).

Not immediately, but I'm not super-familiar with ptrace.

Is w8 clobbered or otherwise changed by syscalls?  Using w8 for this
has the odd effect that tracers can't force a return with a specific
value of w8 without executing the corresponding syscall.  If that's a
meaningful limitation, then presumably some other channel should be
used.

>
>> On x86, the "user_struct" thing has nothing to do with any real kernel
>> data structure, so it's extensible.  Can you just add syscallno to it?
>
> I'm really not keen on changing user-facing structures like that. For
> example, KVM embeds user_pt_regs into kvm_regs.

Fair enough.

>
> We can add a new ptrace request if we have to.
>
> Will



-- 
Andy Lutomirski
AMA Capital Management, LLC

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[Will Deacon]

On Thu, Jul 03, 2014 at 05:13:50PM +0100, Andy Lutomirski wrote:
> On Thu, Jul 3, 2014 at 9:11 AM, Will Deacon <will.deacon@arm.com> wrote:
> > On Thu, Jul 03, 2014 at 04:39:21PM +0100, Andy Lutomirski wrote:
> >> On Thu, Jul 3, 2014 at 3:24 AM, Will Deacon <will.deacon@arm.com> wrote:
> >> > On Thu, Jul 03, 2014 at 08:43:07AM +0100, AKASHI Takahiro wrote:
> >> >> So it seems to me that we can't change a system call by ptrace().
> >> >> Do I misunderstand anything?
> >> >
> >> > No, it looks like you have a point here. I don't think userspace has any
> >> > business with orig_x0, but changing syscallno is certainly useful. I can
> >> > think of two ways to fix this:
> >> >
> >> >   (1) Updating syscallno based on w8, but this ties us to the current ABI
> >> >       and could get messy if this register changes in the future.
> >> >
> >> >   (2) Adding a PTRACE_SET_SYSCALL request, like we have for arch/arm/,
> >> >       but that means adding arch-specific stuff to arch_ptrace (which
> >> >       currently goes straight to ptrace_request on arm64).
> >> >
> >> > It looks like x86 uses orig_ax, which I *think* means we would go with
> >> > (1) above if we followed their lead.
> >>
> >> w8 is a real register, right?  On x86, at least orig_ax isn't a real
> >> register, so it's quite unlikely to conflict with hardware stuff.
> >
> > Yeah, w8 is the hardware register which the Linux ABI uses for the system
> > call number. I was thinking We could allow the debugger/tracer to update
> > the syscall number by updating that register, or do you see an issue with
> > that? (other than tying us to the current ABI).
> 
> Not immediately, but I'm not super-familiar with ptrace.
> 
> Is w8 clobbered or otherwise changed by syscalls?  Using w8 for this
> has the odd effect that tracers can't force a return with a specific
> value of w8 without executing the corresponding syscall.  If that's a
> meaningful limitation, then presumably some other channel should be
> used.

Hmm, that's true. Currently w8 is preserved across a syscall by the kernel,
so it would be pretty bizarre for somebody to try and modify it but I guess
they could do it if they wanted to. However, they could just as easily
modify it on the syscall return path and have the same effect...

Furthermore, glibc unconditionally emits a mov into w8 prior to the svc
instruction, so from a user's perspective that register always contains
the system call number.

FWIW: the role of w8 in the PCS is `Indirect result location register', so
I'd expect it to be saved across the syscall anyway.

Will

[PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP

[Andy Lutomirski]

On Thu, Jul 3, 2014 at 9:32 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, Jul 03, 2014 at 05:13:50PM +0100, Andy Lutomirski wrote:
>> On Thu, Jul 3, 2014 at 9:11 AM, Will Deacon <will.deacon@arm.com> wrote:
>> > On Thu, Jul 03, 2014 at 04:39:21PM +0100, Andy Lutomirski wrote:
>> >> On Thu, Jul 3, 2014 at 3:24 AM, Will Deacon <will.deacon@arm.com> wrote:
>> >> > On Thu, Jul 03, 2014 at 08:43:07AM +0100, AKASHI Takahiro wrote:
>> >> >> So it seems to me that we can't change a system call by ptrace().
>> >> >> Do I misunderstand anything?
>> >> >
>> >> > No, it looks like you have a point here. I don't think userspace has any
>> >> > business with orig_x0, but changing syscallno is certainly useful. I can
>> >> > think of two ways to fix this:
>> >> >
>> >> >   (1) Updating syscallno based on w8, but this ties us to the current ABI
>> >> >       and could get messy if this register changes in the future.
>> >> >
>> >> >   (2) Adding a PTRACE_SET_SYSCALL request, like we have for arch/arm/,
>> >> >       but that means adding arch-specific stuff to arch_ptrace (which
>> >> >       currently goes straight to ptrace_request on arm64).
>> >> >
>> >> > It looks like x86 uses orig_ax, which I *think* means we would go with
>> >> > (1) above if we followed their lead.
>> >>
>> >> w8 is a real register, right?  On x86, at least orig_ax isn't a real
>> >> register, so it's quite unlikely to conflict with hardware stuff.
>> >
>> > Yeah, w8 is the hardware register which the Linux ABI uses for the system
>> > call number. I was thinking We could allow the debugger/tracer to update
>> > the syscall number by updating that register, or do you see an issue with
>> > that? (other than tying us to the current ABI).
>>
>> Not immediately, but I'm not super-familiar with ptrace.
>>
>> Is w8 clobbered or otherwise changed by syscalls?  Using w8 for this
>> has the odd effect that tracers can't force a return with a specific
>> value of w8 without executing the corresponding syscall.  If that's a
>> meaningful limitation, then presumably some other channel should be
>> used.
>
> Hmm, that's true. Currently w8 is preserved across a syscall by the kernel,
> so it would be pretty bizarre for somebody to try and modify it but I guess
> they could do it if they wanted to. However, they could just as easily
> modify it on the syscall return path and have the same effect...
>
> Furthermore, glibc unconditionally emits a mov into w8 prior to the svc
> instruction, so from a user's perspective that register always contains
> the system call number.

That means that, if someone uses a seccomp trace action to skip or
emulate a syscall by writing -1 to w8, then user code will see an
unexpected -1 in w8.  I don't know how much this matters.

>
> FWIW: the role of w8 in the PCS is `Indirect result location register', so
> I'd expect it to be saved across the syscall anyway.
>
> Will



-- 
Andy Lutomirski
AMA Capital Management, LLC