Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965378AbcJXU3b (ORCPT ); Mon, 24 Oct 2016 16:29:31 -0400 Received: from mail-lf0-f65.google.com ([209.85.215.65]:33870 "EHLO mail-lf0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965298AbcJXU33 (ORCPT ); Mon, 24 Oct 2016 16:29:29 -0400 Date: Mon, 24 Oct 2016 23:29:25 +0300 From: Cyrill Gorcunov To: "Eric W. Biederman" Cc: Andrey Vagin , LKML , Pavel Emelyanov , Linux Containers , Kees Cook Subject: Re: [ISSUE] mm: Add a user_ns owner to mm_struct and fix ptrace_may_access Message-ID: <20161024202925.GS1847@uranus.lan> References: <20161024105959.GQ1847@uranus.lan> <8760oh8tbp.fsf@xmission.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8760oh8tbp.fsf@xmission.com> User-Agent: Mutt/1.7.0 (2016-08-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5056 Lines: 134 On Mon, Oct 24, 2016 at 02:01:30PM -0500, Eric W. Biederman wrote: > > Adding the containers list because that is the general place for these > kinds of discussions. Thanks! > Cyrill Gorcunov writes: > > > Hi Eric! A few days ago we've noticed that our zombie00 test case started > > failing: https://ci.openvz.org/job/CRIU/view/All/job/CRIU-linux-next/406/console > > > --- > > ======================== Run zdtm/static/zombie00 in h ========================= > > Start test > > ./zombie00 --pidfile=zombie00.pid --outfile=zombie00.out > > Run criu dump > > Run criu restore > > Send the 15 signal to 30 > > Wait for zdtm/static/zombie00(30) to die for 0.100000 > > ################ Test zdtm/static/zombie00 FAIL at result check ################ > > > > I've narrowed problem down to commit > > > > | From ce99dd5fd5f600f9f4f0d37bb8847c1cb7c6e4fc Mon Sep 17 00:00:00 2001 > > | From: "Eric W. Biederman" > > | Date: Thu, 13 Oct 2016 21:23:16 -0500 > > | Subject: [PATCH] mm: Add a user_ns owner to mm_struct and fix > > | ptrace_may_access > > | > > | During exec dumpable is cleared if the file that is being executed is > > | not readable by the user executing the file. A bug in > > | ptrace_may_access allows reading the file if the executable happens to > > | enter into a subordinate user namespace (aka clone(CLONE_NEWUSER), > > | unshare(CLONE_NEWUSER), or setns(fd, CLONE_NEWUSER). > > > > and the reason is that the zombie tasks do not have task::mm and in resut > > we're obtaining -EPERM when trying to read task->exit_code from > > /proc/pid/stat. > > Hmm. As I read the code exit_code should be returned to userspace as a > 0. It does not look to me as if userspace should see an error in > that case. I mean the ptrace-check returns -EPERM and we don't see @exit_code. Sorry for confusion. > > > Looking into commit I suspect when mm = NULL we've to move back the test > > ptrace_has_cap(__task_cred(task)->user_ns, mode)? > > Maybe. > > We might want to consider if these lines from do_task_stat make > any sense. > > if (permitted) > seq_put_decimal_ll(m, " ", task->exit_code); > else > seq_puts(m, " 0"); > > Looking at the code. Nothing changes behavior except for privileged > tasks looking at processes without an mm. So yes it may be sane > to tweak that part of the check. I think so, otherwise we might break api. > AKA in the in for-next branch the code currenty says: > mm = task->mm; > if (!mm || > ((get_dumpable(mm) != SUID_DUMP_USER) && > !ptrace_has_cap(mm->user_ns, mode))) > return -EPERM; > > And in the case there is no mm there is no way to get > past returning -EPERM. > > Looking at why we use ptrace_may_access in the exit_code case > I see a couple of relevant commits. ... > > The commit that added task->exit_code: > > commit 5b172087f99189416d5f47fd7ab5e6fb762a9ba3 > Author: Cyrill Gorcunov > Date: Thu May 31 16:26:44 2012 -0700 > > c/r: procfs: add arg_start/end, env_start/end and exit_code members to /proc/$pid/stat > > We would like to have an ability to restore command line arguments and > program environment pointers but first we need to obtain them somehow. > Thus we put these values into /proc/$pid/stat. The exit_code is needed to > restore zombie tasks. > > Signed-off-by: Cyrill Gorcunov > Acked-by: Kees Cook > Cc: Pavel Emelyanov > Cc: Serge Hallyn > Cc: KAMEZAWA Hiroyuki > Cc: Alexey Dobriyan > Cc: Tejun Heo > Cc: Andrew Vagin > Cc: Vasiliy Kulikov > Cc: Alexey Dobriyan > Cc: "Eric W. Biederman" > Signed-off-by: Andrew Morton > Signed-off-by: Linus Torvalds > Yes, I've been adding it for criu sake. > Looking at do_task_stat everything else that requires permitted > in do_tack_stat is an address. exit_code is something else so > I am not at all certain the ptrace_may_access permission check > makes sense. Well, I suspect @exit_code may be suitable for attacker to find out if some address accessed cause sigsevg or something like that. > > A process without an mm is fundamentally undumpable so an error should > be returned in any case. So I don't see any harm in failing > ptrace_may_access in general. At the same time I can see how not > preserving the existing behavior is problematic. > > So I am probably going to tweak the !mm case so that instead of failing > we perform the old capable check in that case. That seems the mot > certain way to avoid regressions. With that said, why is exit_code > behind a ptrace_may_access permission check? Yes, this would be great! And as to @exit_code I think better ask Kees, CC'ed. Cyrill