From patchwork Fri Jun 20 17:23:30 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Will Deacon X-Patchwork-Id: 32287 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-pa0-f71.google.com (mail-pa0-f71.google.com [209.85.220.71]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 4C2B2203F4 for ; Fri, 20 Jun 2014 17:24:38 +0000 (UTC) Received: by mail-pa0-f71.google.com with SMTP id eu11sf14384196pac.10 for ; Fri, 20 Jun 2014 10:24:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:date:from:to:cc:subject:message-id :references:mime-version:in-reply-to:user-agent:sender:precedence :list-id:x-original-sender:x-original-authentication-results :mailing-list:list-post:list-help:list-archive:list-unsubscribe :content-type:content-disposition; bh=7MIW8ywZrRTa9aH7tyObN5BqwYHL7mHqyzLr1/ItbyM=; b=grkFJTkLz1KkXWU93clrwycBxLqN3W/POFkjMikvJXaI/JiuQ9ttOhVSPROR8wmI/y AFEW6K5qiGBWxOTgm8LT5/coUBWjzUYdkj2ymy2czh2fDCbgEBU+asjpivnd3HfnNApB ITbEpQBro9Kf5OL2yjr2KnBkaNnbUIyY1mHvTK+pvYs5ojM7ukmg1V8kp2mWLFfXJlwa fXbFcSkfwV/v+zwHmLTGloYs/OmLseU967yDAsOraDCAqX/iTxqJzheyQdHhZdMa7tE5 F6KsRTeUPVMbngpYg5c/gXg6Jkj31m9EEvOVLvGtTwTxeGB4pR2AGfflFqu7w8g+wwYc QuCA== X-Gm-Message-State: ALoCoQnt1Xbq1AtpEAylZca2YBRgSXBzv08koKJTqP6HYVZ6G43LJ2loc7C3lxoKjocc5eG40URq X-Received: by 10.66.66.109 with SMTP id e13mr1850293pat.1.1403285077548; Fri, 20 Jun 2014 10:24:37 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.48.112 with SMTP id n103ls1022132qga.86.gmail; Fri, 20 Jun 2014 10:24:37 -0700 (PDT) X-Received: by 10.58.24.38 with SMTP id r6mr2321774vef.41.1403285077383; Fri, 20 Jun 2014 10:24:37 -0700 (PDT) Received: from mail-vc0-f169.google.com (mail-vc0-f169.google.com [209.85.220.169]) by mx.google.com with ESMTPS id hs4si2325020vdb.61.2014.06.20.10.24.37 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Fri, 20 Jun 2014 10:24:37 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.169 as permitted sender) client-ip=209.85.220.169; Received: by mail-vc0-f169.google.com with SMTP id la4so3725686vcb.14 for ; Fri, 20 Jun 2014 10:24:37 -0700 (PDT) X-Received: by 10.52.249.41 with SMTP id yr9mr1630245vdc.51.1403285077299; Fri, 20 Jun 2014 10:24:37 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.221.37.5 with SMTP id tc5csp38651vcb; Fri, 20 Jun 2014 10:24:36 -0700 (PDT) X-Received: by 10.68.171.229 with SMTP id ax5mr6489361pbc.125.1403285076537; Fri, 20 Jun 2014 10:24:36 -0700 (PDT) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id cd2si10592048pbb.90.2014.06.20.10.24.35; Fri, 20 Jun 2014 10:24:35 -0700 (PDT) Received-SPF: none (google.com: linux-kernel-owner@vger.kernel.org does not designate permitted sender hosts) client-ip=209.132.180.67; Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757151AbaFTRYd (ORCPT + 12 others); Fri, 20 Jun 2014 13:24:33 -0400 Received: from cam-admin0.cambridge.arm.com ([217.140.96.50]:50006 "EHLO cam-admin0.cambridge.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756856AbaFTRYb (ORCPT ); Fri, 20 Jun 2014 13:24:31 -0400 Received: from arm.com (edgewater-inn.cambridge.arm.com [10.1.203.161]) by cam-admin0.cambridge.arm.com (8.12.6/8.12.6) with ESMTP id s5KHNSwo027622; Fri, 20 Jun 2014 18:23:28 +0100 (BST) Date: Fri, 20 Jun 2014 18:23:30 +0100 From: Will Deacon To: Kees Cook Cc: "linux-kernel@vger.kernel.org" , Russell King , Andy Lutomirski , Andrew Morton , Jonathan Austin , =?iso-8859-1?Q?Andr=E9?= Hentschel , Oleg Nesterov , "linux-arm-kernel@lists.infradead.org" , Ricky Zhou Subject: Re: [PATCH] arm: ptrace: fix syscall modification under PTRACE_O_TRACESECCOMP Message-ID: <20140620172330.GA30656@arm.com> References: <20140618202748.GA9022@www.outflux.net> <20140620102258.GA26626@arm.com> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: will.deacon@arm.com X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.169 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , Content-Disposition: inline On Fri, Jun 20, 2014 at 05:44:52PM +0100, Kees Cook wrote: > On Fri, Jun 20, 2014 at 3:22 AM, Will Deacon 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 Tested-by: Kees Cook --- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ 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);