Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756819AbcDGQGw (ORCPT ); Thu, 7 Apr 2016 12:06:52 -0400 Received: from mail-ig0-f196.google.com ([209.85.213.196]:34991 "EHLO mail-ig0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750993AbcDGQGu (ORCPT ); Thu, 7 Apr 2016 12:06:50 -0400 MIME-Version: 1.0 In-Reply-To: <1459819769-30387-1-git-send-email-ebiederm@xmission.com> References: <878u0s3orx.fsf_-_@x220.int.ebiederm.org> <1459819769-30387-1-git-send-email-ebiederm@xmission.com> Date: Thu, 7 Apr 2016 09:06:44 -0700 X-Google-Sender-Auth: UO0UWLZbG2bPO3Mpi8q5YO59a3Q Message-ID: Subject: Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup From: Linus Torvalds To: "Eric W. Biederman" Cc: "H. Peter Anvin" , Peter Hurley , Greg KH , Jiri Slaby , Aurelien Jarno , Andy Lutomirski , Florian Weimer , Al Viro , Serge Hallyn , Jann Horn , "security@kernel.org" , "security@ubuntu.com >> security" , security@debian.org, Willy Tarreau , Linux Kernel Mailing 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 Content-Length: 2954 Lines: 71 On Mon, Apr 4, 2016 at 6:29 PM, Eric W. Biederman wrote: > > In practice I expect the permission checks are a non-issue as the > permissions on /dev/ptmx and /dev/pts/ptmx are always 0666. So I think this is still entirely wrongheaded, and thinking about the problem the wrong way around. The issue is *not* that the "permissions on /dev/ptmx and /dev/pts/ptmx are always 0666". Not at all. The permissions of /dev/ptmx and /dev/pts/ptmx are simply *irrelevant*. We're not interested in opening /dev/ptmx. We are interested in looking up *which* ptmx that pty is associated with. Those are two totally different issues. We never opened /dev./ptmx before either, and we never ever cared about the permiossions of it. We just hardcoded which superblock we were using, regardless of those permissions. We should basically continue to do the exact same thing. We don't care about the permissions of the ptmx entry, and we're not even interested in opening it (it's sufficient to just find the "pts" subdirectory), we are _purely_ asking "which superblock/mount am I associated with". In other words, we *could* do this by doing some insane parsing of /proc/mounts, but that would be stupid. My point is, talking about permissions of these nodes is _wrong_. It's actively misleading. It is exactly the wrong thing to do, because it confuses people into thinking that we somehow care, and that we somehow open the new node. We don't. We're opening the *old* pathname, the one whose permissions we already checked when we walked it, and we're just looking up the pts directory so that we don't hardcode which set of pty's we're talking about. So I think the part of the patch where you check permissions is wrong. I think the part of the commit message where you talk about this is confused. You should make this about looking up the superblock, and explicitly talk about how this is *not* about permissions. So get rid of all the pointless "inode_permission()" crap. We already checked that by virtue of us opening "/dev/ptmx". THOSE permissions matter, but they were already done. Now we're just saying "ok, the user has a right to open the ptmx node, now _which_ devpts is that ptmx node for?" So also get rid of this: + /* Advance path to the ptmx dentry */ + old = path.dentry; + path.dentry = dget(fsi->ptmx_dentry); + dput(old); entirely. It's wrong. It's entirely pointless. We don't even care about "what does pts/ptmx point to". We care about "which superblock do we get when we look up the "pts/" subdirectory in the dentry cache for this user (without permissions)"/ So get rid of all the pathname games. Just save the superblock pointer in file->f_private or somewhere like that, and make it really clear that what we are doing is making "/dev/ptmx" work sanely! The user is not looking up "/dev/pts/ptmx". They are looking up "/dev/ptmx". See the difference? Linus