Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup

[Linus Torvalds]

On Mon, Apr 4, 2016 at 6:29 PM, Eric W. Biederman <ebiederm@xxxxxxxxxxxx> 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