On Wed, Jan 14, 2015 at 5:54 PM, Roman Peniaev <r.peniaev@xxxxxxxxx> wrote: > On Thu, Jan 15, 2015 at 5:51 AM, Kees Cook <keescook@xxxxxxxxxxxx> wrote: >> On Tue, Jan 13, 2015 at 12:35 AM, Roman Peniaev <r.peniaev@xxxxxxxxx> wrote: >>> On Tue, Jan 13, 2015 at 3:39 AM, Will Deacon <will.deacon@xxxxxxx> 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 <r.peniaev@xxxxxxxxx> >>>>> Cc: Russell King <linux@xxxxxxxxxxxxxxxx> >>>>> Cc: Marc Zyngier <marc.zyngier@xxxxxxx> >>>>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx> >>>>> Cc: Christoffer Dall <christoffer.dall@xxxxxxxxxx> >>>>> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx> >>>>> Cc: Sekhar Nori <nsekhar@xxxxxx> >>>>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx >>>>> Cc: linux-kernel@xxxxxxxxxxxxxxx >>>>> Cc: stable@xxxxxxxxxxxxxxx >>>>> --- >>>>> 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 <keescook@xxxxxxxxxxxx> 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? -Kees > > -- > Roman > > >> >> -Kees >> >>> >>> >>> -- >>> Roman >>> >>> _______________________________________________ >>> linux-arm-kernel mailing list >>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> >> >> >> -- >> Kees Cook >> Chrome OS Security -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html