From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756591AbbAPQEj (ORCPT ); Fri, 16 Jan 2015 11:04:39 -0500 Received: from mail-wi0-f169.google.com ([209.85.212.169]:37074 "EHLO mail-wi0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751717AbbAPQEh (ORCPT ); Fri, 16 Jan 2015 11:04:37 -0500 MIME-Version: 1.0 In-Reply-To: References: <1420986751-30364-1-git-send-email-r.peniaev@gmail.com> <1420986751-30364-2-git-send-email-r.peniaev@gmail.com> <20150112183955.GO13360@arm.com> Date: Sat, 17 Jan 2015 00:57:02 +0900 Message-ID: Subject: Re: [PATCH 1/2] ARM: entry-common: fix forgotten set of thread_info->syscall From: Roman Peniaev To: Kees Cook Cc: Will Deacon , Russell King , Stefano Stabellini , Marc Zyngier , Catalin Marinas , Sekhar Nori , "linux-kernel@vger.kernel.org" , "stable@vger.kernel.org" , Christoffer Dall , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 16, 2015 at 7:54 AM, Kees Cook wrote: > On Wed, Jan 14, 2015 at 5:54 PM, Roman Peniaev wrote: >> On Thu, Jan 15, 2015 at 5:51 AM, Kees Cook wrote: >>> On Tue, Jan 13, 2015 at 12:35 AM, Roman Peniaev wrote: >>>> On Tue, Jan 13, 2015 at 3:39 AM, Will Deacon wrote: >>>>> On Sun, Jan 11, 2015 at 02:32:30PM +0000, Roman Pen wrote: >>>>>> thread_info->syscall is used only for ptrace, but syscall number >>>>>> is also used by syscall_get_nr and returned to userspace by the >>>>>> following proc file access: >>>>>> >>>>>> $ cat /proc/self/syscall >>>>>> 0 0x3 0xbe928bd8 0x1000 0x0 0xac9e0 0x3 0xbe928bb4 0xb6f5dfbc >>>>>> ^ >>>>>> The first number is the syscall number, currently it is zero. >>>>>> Patch fixes this: >>>>>> >>>>>> $ cat /proc/self/syscall >>>>>> 3 0x3 0xbefc7bd8 0x1000 0x0 0xac9e0 0x3 0xbefc7bb4 0xb6e82fbc >>>>>> ^ >>>>>> Right, read syscall >>>>> >>>>> Yes, it seems that despite requiring CONFIG_HAVE_ARCH_TRACEHOOK, >>>>> the /proc code requires syscall_get_nr to work regardless of >>>>> TIF_SYSCALL_TRACE. >>>>> >>>>>> Signed-off-by: Roman Pen >>>>>> Cc: Russell King >>>>>> Cc: Marc Zyngier >>>>>> Cc: Catalin Marinas >>>>>> Cc: Christoffer Dall >>>>>> Cc: Stefano Stabellini >>>>>> Cc: Sekhar Nori >>>>>> Cc: linux-arm-kernel@lists.infradead.org >>>>>> Cc: linux-kernel@vger.kernel.org >>>>>> Cc: stable@vger.kernel.org >>>>>> --- >>>>>> arch/arm/kernel/asm-offsets.c | 1 + >>>>>> arch/arm/kernel/entry-common.S | 1 + >>>>>> 2 files changed, 2 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c >>>>>> index 2d2d608..6911bad 100644 >>>>>> --- a/arch/arm/kernel/asm-offsets.c >>>>>> +++ b/arch/arm/kernel/asm-offsets.c >>>>>> @@ -70,6 +70,7 @@ int main(void) >>>>>> DEFINE(TI_CPU, offsetof(struct thread_info, cpu)); >>>>>> DEFINE(TI_CPU_DOMAIN, offsetof(struct thread_info, cpu_domain)); >>>>>> DEFINE(TI_CPU_SAVE, offsetof(struct thread_info, cpu_context)); >>>>>> + DEFINE(TI_SYSCALL, offsetof(struct thread_info, syscall)); >>>>>> DEFINE(TI_USED_CP, offsetof(struct thread_info, used_cp)); >>>>>> DEFINE(TI_TP_VALUE, offsetof(struct thread_info, tp_value)); >>>>>> DEFINE(TI_FPSTATE, offsetof(struct thread_info, fpstate)); >>>>>> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S >>>>>> index f8ccc21..89452ff 100644 >>>>>> --- a/arch/arm/kernel/entry-common.S >>>>>> +++ b/arch/arm/kernel/entry-common.S >>>>>> @@ -189,6 +189,7 @@ ENTRY(vector_swi) >>>>>> #endif >>>>>> >>>>>> local_restart: >>>>>> + str scno, [tsk, #TI_SYSCALL] @ set syscall number >>>>>> ldr r10, [tsk, #TI_FLAGS] @ check for syscall tracing >>>>>> stmdb sp!, {r4, r5} @ push fifth and sixth args >>>>> >>>>> Do we definitely want to update scno on syscall restarting? >>>> >>>> >>>> Good question. >>>> >>>> First thing to mention is __sys_trace will trace 'restart_syscall', >>>> not the real syscall we are going to restart. >>>> >>>> E.g. in test application we do infinite poll and then send STOP and >>>> CONT to this app: >>>> >>>> test-243 [002] ...1 1792.067726: sys_enter: NR 168 (0, 0, >>>> ffffffff, 0, 0, 0) >>>> test-243 [002] ...1 1802.299073: sys_exit: NR 168 = -516 >>>> test-243 [004] ...1 1814.716264: sys_enter: NR 0 (0, 0, >>>> ffffffff, 0, 0, 0) >>>> test-243 [004] ...1 2183.687225: sys_exit: NR 0 = -516 >>>> >>>> the poll was restarted and trace shows that we are in restart_syscall. >>>> >>>> Is that expected? >>>> >>>> And the second thing is that my next patch did some tweaks in >>>> 'syscall_trace_enter', where we take scno not from param we passed, >>>> but from thread_info->syscall we previously set. >>>> >>>> So, regarding your question, if I set scno only once - I will break >>>> previous behavior, and __sys_trace will trace the syscall we restarted. >>>> >>>> And I think this is what we need, because according to the >>>> 'syscall_trace_enter' code we do 'secure_computing' and >>>> 'audit_syscall_entry', which definitely expect original syscall, not >>>> the 'restart_syscall'. >>> >>> Seccomp expects to see the __NR_restart_syscall syscall, since it >>> interposes the syscall entry points. >> >> >> Aha, thanks. So I should not break anything. > > I've tested on ARM now, seccomp doesn't see any change in behavior. > Please consider both patches: > > Tested-by: Kees Cook > Thanks. > One interesting thing I noticed (which is unchanged by this series), > but pulling ARM_r7 during the seccomp ptrace event shows __NR_poll, > not __NR_restart_syscall, even though it was a __NR_restart_syscall > trap from seccomp. Is there a better place to see the actual syscall? As I understand we do not push new r7 to the stack, and ptrace uses the old value. I checked x86, x86-64 - ptrace returns __NR_restart_syscall. So, probably, this should be fixed for ARM. -- Roman From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1420986751-30364-1-git-send-email-r.peniaev@gmail.com> <1420986751-30364-2-git-send-email-r.peniaev@gmail.com> <20150112183955.GO13360@arm.com> Date: Sat, 17 Jan 2015 00:57:02 +0900 Message-ID: Subject: Re: [PATCH 1/2] ARM: entry-common: fix forgotten set of thread_info->syscall From: Roman Peniaev To: Kees Cook Cc: Will Deacon , Russell King , Stefano Stabellini , Marc Zyngier , Catalin Marinas , Sekhar Nori , "linux-kernel@vger.kernel.org" , "stable@vger.kernel.org" , Christoffer Dall , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: On Fri, Jan 16, 2015 at 7:54 AM, Kees Cook wrote: > On Wed, Jan 14, 2015 at 5:54 PM, Roman Peniaev wrote: >> On Thu, Jan 15, 2015 at 5:51 AM, Kees Cook wrote: >>> On Tue, Jan 13, 2015 at 12:35 AM, Roman Peniaev wrote: >>>> On Tue, Jan 13, 2015 at 3:39 AM, Will Deacon wrote: >>>>> On Sun, Jan 11, 2015 at 02:32:30PM +0000, Roman Pen wrote: >>>>>> thread_info->syscall is used only for ptrace, but syscall number >>>>>> is also used by syscall_get_nr and returned to userspace by the >>>>>> following proc file access: >>>>>> >>>>>> $ cat /proc/self/syscall >>>>>> 0 0x3 0xbe928bd8 0x1000 0x0 0xac9e0 0x3 0xbe928bb4 0xb6f5dfbc >>>>>> ^ >>>>>> The first number is the syscall number, currently it is zero. >>>>>> Patch fixes this: >>>>>> >>>>>> $ cat /proc/self/syscall >>>>>> 3 0x3 0xbefc7bd8 0x1000 0x0 0xac9e0 0x3 0xbefc7bb4 0xb6e82fbc >>>>>> ^ >>>>>> Right, read syscall >>>>> >>>>> Yes, it seems that despite requiring CONFIG_HAVE_ARCH_TRACEHOOK, >>>>> the /proc code requires syscall_get_nr to work regardless of >>>>> TIF_SYSCALL_TRACE. >>>>> >>>>>> Signed-off-by: Roman Pen >>>>>> Cc: Russell King >>>>>> Cc: Marc Zyngier >>>>>> Cc: Catalin Marinas >>>>>> Cc: Christoffer Dall >>>>>> Cc: Stefano Stabellini >>>>>> Cc: Sekhar Nori >>>>>> Cc: linux-arm-kernel@lists.infradead.org >>>>>> Cc: linux-kernel@vger.kernel.org >>>>>> Cc: stable@vger.kernel.org >>>>>> --- >>>>>> arch/arm/kernel/asm-offsets.c | 1 + >>>>>> arch/arm/kernel/entry-common.S | 1 + >>>>>> 2 files changed, 2 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c >>>>>> index 2d2d608..6911bad 100644 >>>>>> --- a/arch/arm/kernel/asm-offsets.c >>>>>> +++ b/arch/arm/kernel/asm-offsets.c >>>>>> @@ -70,6 +70,7 @@ int main(void) >>>>>> DEFINE(TI_CPU, offsetof(struct thread_info, cpu)); >>>>>> DEFINE(TI_CPU_DOMAIN, offsetof(struct thread_info, cpu_domain)); >>>>>> DEFINE(TI_CPU_SAVE, offsetof(struct thread_info, cpu_context)); >>>>>> + DEFINE(TI_SYSCALL, offsetof(struct thread_info, syscall)); >>>>>> DEFINE(TI_USED_CP, offsetof(struct thread_info, used_cp)); >>>>>> DEFINE(TI_TP_VALUE, offsetof(struct thread_info, tp_value)); >>>>>> DEFINE(TI_FPSTATE, offsetof(struct thread_info, fpstate)); >>>>>> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S >>>>>> index f8ccc21..89452ff 100644 >>>>>> --- a/arch/arm/kernel/entry-common.S >>>>>> +++ b/arch/arm/kernel/entry-common.S >>>>>> @@ -189,6 +189,7 @@ ENTRY(vector_swi) >>>>>> #endif >>>>>> >>>>>> local_restart: >>>>>> + str scno, [tsk, #TI_SYSCALL] @ set syscall number >>>>>> ldr r10, [tsk, #TI_FLAGS] @ check for syscall tracing >>>>>> stmdb sp!, {r4, r5} @ push fifth and sixth args >>>>> >>>>> Do we definitely want to update scno on syscall restarting? >>>> >>>> >>>> Good question. >>>> >>>> First thing to mention is __sys_trace will trace 'restart_syscall', >>>> not the real syscall we are going to restart. >>>> >>>> E.g. in test application we do infinite poll and then send STOP and >>>> CONT to this app: >>>> >>>> test-243 [002] ...1 1792.067726: sys_enter: NR 168 (0, 0, >>>> ffffffff, 0, 0, 0) >>>> test-243 [002] ...1 1802.299073: sys_exit: NR 168 = -516 >>>> test-243 [004] ...1 1814.716264: sys_enter: NR 0 (0, 0, >>>> ffffffff, 0, 0, 0) >>>> test-243 [004] ...1 2183.687225: sys_exit: NR 0 = -516 >>>> >>>> the poll was restarted and trace shows that we are in restart_syscall. >>>> >>>> Is that expected? >>>> >>>> And the second thing is that my next patch did some tweaks in >>>> 'syscall_trace_enter', where we take scno not from param we passed, >>>> but from thread_info->syscall we previously set. >>>> >>>> So, regarding your question, if I set scno only once - I will break >>>> previous behavior, and __sys_trace will trace the syscall we restarted. >>>> >>>> And I think this is what we need, because according to the >>>> 'syscall_trace_enter' code we do 'secure_computing' and >>>> 'audit_syscall_entry', which definitely expect original syscall, not >>>> the 'restart_syscall'. >>> >>> Seccomp expects to see the __NR_restart_syscall syscall, since it >>> interposes the syscall entry points. >> >> >> Aha, thanks. So I should not break anything. > > I've tested on ARM now, seccomp doesn't see any change in behavior. > Please consider both patches: > > Tested-by: Kees Cook > Thanks. > One interesting thing I noticed (which is unchanged by this series), > but pulling ARM_r7 during the seccomp ptrace event shows __NR_poll, > not __NR_restart_syscall, even though it was a __NR_restart_syscall > trap from seccomp. Is there a better place to see the actual syscall? As I understand we do not push new r7 to the stack, and ptrace uses the old value. I checked x86, x86-64 - ptrace returns __NR_restart_syscall. So, probably, this should be fixed for ARM. -- Roman From mboxrd@z Thu Jan 1 00:00:00 1970 From: r.peniaev@gmail.com (Roman Peniaev) Date: Sat, 17 Jan 2015 00:57:02 +0900 Subject: [PATCH 1/2] ARM: entry-common: fix forgotten set of thread_info->syscall In-Reply-To: References: <1420986751-30364-1-git-send-email-r.peniaev@gmail.com> <1420986751-30364-2-git-send-email-r.peniaev@gmail.com> <20150112183955.GO13360@arm.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Fri, Jan 16, 2015 at 7:54 AM, Kees Cook wrote: > On Wed, Jan 14, 2015 at 5:54 PM, Roman Peniaev wrote: >> On Thu, Jan 15, 2015 at 5:51 AM, Kees Cook wrote: >>> On Tue, Jan 13, 2015 at 12:35 AM, Roman Peniaev wrote: >>>> On Tue, Jan 13, 2015 at 3:39 AM, Will Deacon wrote: >>>>> On Sun, Jan 11, 2015 at 02:32:30PM +0000, Roman Pen wrote: >>>>>> thread_info->syscall is used only for ptrace, but syscall number >>>>>> is also used by syscall_get_nr and returned to userspace by the >>>>>> following proc file access: >>>>>> >>>>>> $ cat /proc/self/syscall >>>>>> 0 0x3 0xbe928bd8 0x1000 0x0 0xac9e0 0x3 0xbe928bb4 0xb6f5dfbc >>>>>> ^ >>>>>> The first number is the syscall number, currently it is zero. >>>>>> Patch fixes this: >>>>>> >>>>>> $ cat /proc/self/syscall >>>>>> 3 0x3 0xbefc7bd8 0x1000 0x0 0xac9e0 0x3 0xbefc7bb4 0xb6e82fbc >>>>>> ^ >>>>>> Right, read syscall >>>>> >>>>> Yes, it seems that despite requiring CONFIG_HAVE_ARCH_TRACEHOOK, >>>>> the /proc code requires syscall_get_nr to work regardless of >>>>> TIF_SYSCALL_TRACE. >>>>> >>>>>> Signed-off-by: Roman Pen >>>>>> Cc: Russell King >>>>>> Cc: Marc Zyngier >>>>>> Cc: Catalin Marinas >>>>>> Cc: Christoffer Dall >>>>>> Cc: Stefano Stabellini >>>>>> Cc: Sekhar Nori >>>>>> Cc: linux-arm-kernel at lists.infradead.org >>>>>> Cc: linux-kernel at vger.kernel.org >>>>>> Cc: stable at vger.kernel.org >>>>>> --- >>>>>> arch/arm/kernel/asm-offsets.c | 1 + >>>>>> arch/arm/kernel/entry-common.S | 1 + >>>>>> 2 files changed, 2 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c >>>>>> index 2d2d608..6911bad 100644 >>>>>> --- a/arch/arm/kernel/asm-offsets.c >>>>>> +++ b/arch/arm/kernel/asm-offsets.c >>>>>> @@ -70,6 +70,7 @@ int main(void) >>>>>> DEFINE(TI_CPU, offsetof(struct thread_info, cpu)); >>>>>> DEFINE(TI_CPU_DOMAIN, offsetof(struct thread_info, cpu_domain)); >>>>>> DEFINE(TI_CPU_SAVE, offsetof(struct thread_info, cpu_context)); >>>>>> + DEFINE(TI_SYSCALL, offsetof(struct thread_info, syscall)); >>>>>> DEFINE(TI_USED_CP, offsetof(struct thread_info, used_cp)); >>>>>> DEFINE(TI_TP_VALUE, offsetof(struct thread_info, tp_value)); >>>>>> DEFINE(TI_FPSTATE, offsetof(struct thread_info, fpstate)); >>>>>> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S >>>>>> index f8ccc21..89452ff 100644 >>>>>> --- a/arch/arm/kernel/entry-common.S >>>>>> +++ b/arch/arm/kernel/entry-common.S >>>>>> @@ -189,6 +189,7 @@ ENTRY(vector_swi) >>>>>> #endif >>>>>> >>>>>> local_restart: >>>>>> + str scno, [tsk, #TI_SYSCALL] @ set syscall number >>>>>> ldr r10, [tsk, #TI_FLAGS] @ check for syscall tracing >>>>>> stmdb sp!, {r4, r5} @ push fifth and sixth args >>>>> >>>>> Do we definitely want to update scno on syscall restarting? >>>> >>>> >>>> Good question. >>>> >>>> First thing to mention is __sys_trace will trace 'restart_syscall', >>>> not the real syscall we are going to restart. >>>> >>>> E.g. in test application we do infinite poll and then send STOP and >>>> CONT to this app: >>>> >>>> test-243 [002] ...1 1792.067726: sys_enter: NR 168 (0, 0, >>>> ffffffff, 0, 0, 0) >>>> test-243 [002] ...1 1802.299073: sys_exit: NR 168 = -516 >>>> test-243 [004] ...1 1814.716264: sys_enter: NR 0 (0, 0, >>>> ffffffff, 0, 0, 0) >>>> test-243 [004] ...1 2183.687225: sys_exit: NR 0 = -516 >>>> >>>> the poll was restarted and trace shows that we are in restart_syscall. >>>> >>>> Is that expected? >>>> >>>> And the second thing is that my next patch did some tweaks in >>>> 'syscall_trace_enter', where we take scno not from param we passed, >>>> but from thread_info->syscall we previously set. >>>> >>>> So, regarding your question, if I set scno only once - I will break >>>> previous behavior, and __sys_trace will trace the syscall we restarted. >>>> >>>> And I think this is what we need, because according to the >>>> 'syscall_trace_enter' code we do 'secure_computing' and >>>> 'audit_syscall_entry', which definitely expect original syscall, not >>>> the 'restart_syscall'. >>> >>> Seccomp expects to see the __NR_restart_syscall syscall, since it >>> interposes the syscall entry points. >> >> >> Aha, thanks. So I should not break anything. > > I've tested on ARM now, seccomp doesn't see any change in behavior. > Please consider both patches: > > Tested-by: Kees Cook > Thanks. > One interesting thing I noticed (which is unchanged by this series), > but pulling ARM_r7 during the seccomp ptrace event shows __NR_poll, > not __NR_restart_syscall, even though it was a __NR_restart_syscall > trap from seccomp. Is there a better place to see the actual syscall? As I understand we do not push new r7 to the stack, and ptrace uses the old value. I checked x86, x86-64 - ptrace returns __NR_restart_syscall. So, probably, this should be fixed for ARM. -- Roman