From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751457AbcFRKV1 (ORCPT ); Sat, 18 Jun 2016 06:21:27 -0400 Received: from mail-vk0-f53.google.com ([209.85.213.53]:36273 "EHLO mail-vk0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751176AbcFRKVZ (ORCPT ); Sat, 18 Jun 2016 06:21:25 -0400 MIME-Version: 1.0 In-Reply-To: References: <20160614175729.GA20816@www.outflux.net> From: Andy Lutomirski Date: Sat, 18 Jun 2016 03:21:04 -0700 Message-ID: Subject: Re: [PULL] seccomp update (next) To: Kees Cook Cc: "linux-kernel@vger.kernel.org" , James Morris , LSM List 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 Jun 18, 2016 12:02 AM, "Kees Cook" wrote: > > On Fri, Jun 17, 2016 at 11:55 AM, Andy Lutomirski wrote: > > On Fri, Jun 17, 2016 at 12:15 AM, James Morris wrote: > >> On Tue, 14 Jun 2016, Kees Cook wrote: > >> > >>> Hi, > >>> > >>> Please pull these seccomp changes for next. These have been tested by > >>> myself and Andy, and close a long-standing issue with seccomp where tracers > >>> could change the syscall out from under seccomp. > >> > >> Pulled to security -next. > > > > As a heads up: I think this doesn't quite close the hole on x86. Consider: > > > > 64-bit task arranges to be traced by a 32-bit task (or presumably a > > 64-bit task that calls ptrace via int80). > > > > Tracer does PTRACE_SYSCALL. > > > > Tracee does a normal syscall. > > > > Tracer writes tracee's orig_ax, thus invoking this thing in > > arch/x86/kernel/ptrace.c: > > > > if (syscall_get_nr(child, regs) >= 0) > > child->thread.status |= TS_COMPAT; > > > > Tracer resumes and gets confused. > > > > I think the right fix is to just delete: > > > > if (syscall_get_nr(child, regs) >= 0) > > child->thread.status |= TS_COMPAT; > > > > from ptrace.c. The comment above it is garbage, too. > > I'm perfectly happy to see it removed. I can't make sense of the comment. :) > > That said, the only confusion I see is pretty minor. The arch is saved > before the tracer could force TS_COMPAT, so nothing confused is handed > to seccomp (the first time). And the syscall will continue to be > looked up on sys_call_table not ia32_sys_call_table. Hmm, right, but... > > The only thing I see is if the tracer has also added a > SECCOMP_RET_TRACE filter, after which the recheck will reload all the > seccomp info, including the arch. And at this point, a sensible filter > will reject a non-matching architecture. > Yes for some filters, but others might have different logic for different arches, at which point there's a minor bypass. > Maybe I'm missing something more? > You can also use this to do a 64-bit syscall with in_compat_syscall() returning true, which could cause issues for audit and maybe some ioctl handlers irrespective of this patch series. I'll see about getting it fixed in x86/urgent.