From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751764AbcFSWqs (ORCPT ); Sun, 19 Jun 2016 18:46:48 -0400 Received: from mail-vk0-f50.google.com ([209.85.213.50]:35444 "EHLO mail-vk0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751346AbcFSWqm (ORCPT ); Sun, 19 Jun 2016 18:46:42 -0400 MIME-Version: 1.0 In-Reply-To: References: <20160614175729.GA20816@www.outflux.net> From: Andy Lutomirski Date: Sun, 19 Jun 2016 15:45:27 -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 Sat, Jun 18, 2016 at 3:21 AM, Andy Lutomirski wrote: > 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. Hi Kees and James- On further consideration: (a) that TS_COMPAT thing is highly, highly buggy -- much buggier and more confusing than I thought, and not in the way I thought. (b) it doesn't interfere with seccomp at all, so fixing it is not at all a prerequisite for these changes. -- Andy Lutomirski AMA Capital Management, LLC