Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753918AbcJYJCc (ORCPT ); Tue, 25 Oct 2016 05:02:32 -0400 Received: from mail-lf0-f68.google.com ([209.85.215.68]:33465 "EHLO mail-lf0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755903AbcJYJCR (ORCPT ); Tue, 25 Oct 2016 05:02:17 -0400 Date: Tue, 25 Oct 2016 12:02:13 +0300 From: Cyrill Gorcunov To: "Eric W. Biederman" Cc: Kees Cook , Andrey Vagin , LKML , Pavel Emelyanov , Linux Containers Subject: Re: [ISSUE] mm: Add a user_ns owner to mm_struct and fix ptrace_may_access Message-ID: <20161025090213.GX1847@uranus.lan> References: <20161024105959.GQ1847@uranus.lan> <8760oh8tbp.fsf@xmission.com> <20161024202925.GS1847@uranus.lan> <8760oh737b.fsf@xmission.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8760oh737b.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: 2453 Lines: 54 On Mon, Oct 24, 2016 at 06:11:04PM -0500, Eric W. Biederman wrote: > Kees Cook writes: > > > On Mon, Oct 24, 2016 at 1:29 PM, Cyrill Gorcunov wrote: > >> On Mon, Oct 24, 2016 at 02:01:30PM -0500, Eric W. Biederman wrote: > >>> 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. > > > > My concern was that this was an exposure in the sense that it is > > internal program state that isn't visible through other means (without > > being the parent, for example). Under the ptrace check, it has an > > equivalency that seemed correct at the time. > > > > As already covered, I'd agree: it looks like ce99dd5fd5f6 accidentally > > added a dependency on task->mm where it didn't before. That section of > > logic was entirely around dumpability, not an mm existing. It should > > be "EPERM if mm and dumpable != SUID_DUMP_USER". > > > > That said, I'd also agree that ptrace against no mm is crazy (though I > > suppose that should return EINVAL or ESRCH or something), so perhaps a > > better access control on @exit_code is needed here. > > I traced down the original logic of why we had that dumpable > variable, and it was ancient conservative on my part when we started > using the ptrace permission checks for proc files. > > That same conservatism has resulted in the regression under > discussion. > > Given that we already have a very full set of permission checks > separate from dumpable in ptrace_may_access I think it makes sense > to simply ignore dumpable when there is no mm. > AKA: > mm = task->mm; > if (mm && > ((get_dumpable(mm) != SUID_DUMP_USER) && > !ptrace_has_cap(mm->user_ns, mode))) > return -EPERM; > > Because while it has been used for other things dumpable is > fundamentally about do you have permission to read the mm. > So there is no real point in permission checks that protect > the mm if the mm has gone away. > > It also looks like I may need to update the check that sets > PT_PTRACE_CAP to look at mm->user_ns as well. Thanks a lot for informative explanations, Eric and Kees! Eric, if you make some patch please ping me to test it.