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

[H. Peter Anvin]

On 04/12/16 11:12, Linus Torvalds wrote:


So what I want to happen is to "just make /dev/ptmx work".  Get rid of
the broken "single instance" crap. The only reason it exists is
exactly because /dev/ptmx does not work.

I think the current situation is completely and utterly broken. We
should never have done what we did. I want to *fix* the kernel, not
add random new magic crap.



Agreed.  As far as I'm concerned, there seem to be two realistic 
variants, talking semantically as opposed to implementation-wise:



1. Change the default mode of /dev/pts/ptmx to default to 0666, and make 
/dev/ptmx have the effective semantics of the symlink which userspace 
and userdev/devramfs should have provided all along.


2. Make /dev/ptmx simply look up the pts superblock from its path and 
then act like /dev/pts/ptmx.  In that case we can probably remove the 
ptmx device node unless the ptmxmode mount option is given (in which 
case user space probably enabled the symlink.)


-hpa

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

[H. Peter Anvin]

On 04/12/16 11:12, Linus Torvalds wrote:


So what I want to happen is to "just make /dev/ptmx work".  Get rid of
the broken "single instance" crap. The only reason it exists is
exactly because /dev/ptmx does not work.

I think the current situation is completely and utterly broken. We
should never have done what we did. I want to *fix* the kernel, not
add random new magic crap.



Agreed.  As far as I'm concerned, there seem to be two realistic 
variants, talking semantically as opposed to implementation-wise:



1. Change the default mode of /dev/pts/ptmx to default to 0666, and make 
/dev/ptmx have the effective semantics of the symlink which userspace 
and userdev/devramfs should have provided all along.


2. Make /dev/ptmx simply look up the pts superblock from its path and 
then act like /dev/pts/ptmx.  In that case we can probably remove the 
ptmx device node unless the ptmxmode mount option is given (in which 
case user space probably enabled the symlink.)


-hpa

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

[Linus Torvalds]

On Tue, Apr 12, 2016 at 10:44 AM, Andy Lutomirski  wrote:
> Linus, you said that people who want to protect their pts should deny
> execute.  So I set it up:
>
> # ls -l
> total 0
> crw---. 1 root root 5, 2 Apr 12 10:38 ptmx
> drwx--. 2 root root0 Apr  2 11:35 pts

No you didn't. You're root, and you still have access to /dev/ptmx.

> And there goes your protection.  So the whole /dev directory would
> have to deny execute to protect against this.

Exactly. That's what I'm saying. If you want your ptmx to be private,
you need to make your /dev private.

Now, you can avoid the other attack that was talked about (which
involved bind-mounting the pts/ directory somewhere else) by making
just the pts/ directory non-execute, because afaik bind mount requires
the ability to do the lookup.

> But I think that gating this on mount options might be fine.  If
> devpts is mounted with newinstance, then /dev/ptmx *already doesn't
> work for it*, right?  So can we just say that the magic ptmx ->
> pts/ptmx redirect doesn't work if the pts filesystem in question is
> mounted with newinstance?

No, the problem that started this whole discussion is that

 (a) newinstance should go the f*ck away, because this whole duality is broken.

 (b) people wanted single instances and we couldn't even enable
default kernel support for DEVPTS_MULTIPLE_INSTANCES, because multiple
instances just don't work with /dev/ptmx.

So what I want to happen is to "just make /dev/ptmx work".  Get rid of
the broken "single instance" crap. The only reason it exists is
exactly because /dev/ptmx does not work.

I think the current situation is completely and utterly broken. We
should never have done what we did. I want to *fix* the kernel, not
add random new magic crap.

And I think we _can_ fix the kernel. Not add new mount options that
people already don't use (because they are broken for the normal
situation).

Linus

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

[Linus Torvalds]

On Tue, Apr 12, 2016 at 10:44 AM, Andy Lutomirski  wrote:
> Linus, you said that people who want to protect their pts should deny
> execute.  So I set it up:
>
> # ls -l
> total 0
> crw---. 1 root root 5, 2 Apr 12 10:38 ptmx
> drwx--. 2 root root0 Apr  2 11:35 pts

No you didn't. You're root, and you still have access to /dev/ptmx.

> And there goes your protection.  So the whole /dev directory would
> have to deny execute to protect against this.

Exactly. That's what I'm saying. If you want your ptmx to be private,
you need to make your /dev private.

Now, you can avoid the other attack that was talked about (which
involved bind-mounting the pts/ directory somewhere else) by making
just the pts/ directory non-execute, because afaik bind mount requires
the ability to do the lookup.

> But I think that gating this on mount options might be fine.  If
> devpts is mounted with newinstance, then /dev/ptmx *already doesn't
> work for it*, right?  So can we just say that the magic ptmx ->
> pts/ptmx redirect doesn't work if the pts filesystem in question is
> mounted with newinstance?

No, the problem that started this whole discussion is that

 (a) newinstance should go the f*ck away, because this whole duality is broken.

 (b) people wanted single instances and we couldn't even enable
default kernel support for DEVPTS_MULTIPLE_INSTANCES, because multiple
instances just don't work with /dev/ptmx.

So what I want to happen is to "just make /dev/ptmx work".  Get rid of
the broken "single instance" crap. The only reason it exists is
exactly because /dev/ptmx does not work.

I think the current situation is completely and utterly broken. We
should never have done what we did. I want to *fix* the kernel, not
add random new magic crap.

And I think we _can_ fix the kernel. Not add new mount options that
people already don't use (because they are broken for the normal
situation).

Linus

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

[Andy Lutomirski]

On Mon, Apr 11, 2016 at 1:12 PM, Andy Lutomirski  wrote:
> On Sat, Apr 9, 2016 at 6:27 PM, Linus Torvalds
>  wrote:
>>
>> On Apr 9, 2016 5:45 PM, "Andy Lutomirski"  wrote:
>>>
>>>
>>> What we *do* want to do, though, is to prevent the following:
>>
>> I don't see the point. Why do you bring up this insane scenario that nobody
>> can possibly care about?
>>
>> So you actually have any reason to believe somebody does that?
>>
>> I already asked about that earlier, and the silence was deafening.
>
> I have no idea, but I'm generally uncomfortable with magical things
> that bypass normal security policy.
>
> That being said, here's an idea for fixing this, at least in the long
> run.  Add a new devpts mount option "no_ptmx_redirect" that turns off
> this behavior for the super in question.  That is, opening /dev/ptmx
> if "pts/ptmx" points to something with no_ptmx_redirect set will fail.
> Distros shipping new kernels could be encouraged to (finally!) make
> /dev/ptmx a symlink and set this option.
>
> We just might be able to get away with spelling that option "newinstance".

Linus, you said that people who want to protect their pts should deny
execute.  So I set it up:

# ls -l
total 0
crw---. 1 root root 5, 2 Apr 12 10:38 ptmx
drwx--. 2 root root0 Apr  2 11:35 pts

$ unshare -urm
# ls -l
total 0
crw---. 1 nfsnobody nfsnobody 5, 2 Apr 12 10:38 ptmx
drwx--. 2 nfsnobody nfsnobody0 Apr  2 11:35 pts
# mount --bind /dev/ptmx ptmx
# ls -l
total 0
crw-rw-rw-. 1 nfsnobody nfsnobody 5, 2 Apr 12 10:42 ptmx
drwx--. 2 nfsnobody nfsnobody0 Apr  2 11:35 pts

And there goes your protection.  So the whole /dev directory would
have to deny execute to protect against this.

But I think that gating this on mount options might be fine.  If
devpts is mounted with newinstance, then /dev/ptmx *already doesn't
work for it*, right?  So can we just say that the magic ptmx ->
pts/ptmx redirect doesn't work if the pts filesystem in question is
mounted with newinstance?

--Andy

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

[Andy Lutomirski]

On Mon, Apr 11, 2016 at 1:12 PM, Andy Lutomirski  wrote:
> On Sat, Apr 9, 2016 at 6:27 PM, Linus Torvalds
>  wrote:
>>
>> On Apr 9, 2016 5:45 PM, "Andy Lutomirski"  wrote:
>>>
>>>
>>> What we *do* want to do, though, is to prevent the following:
>>
>> I don't see the point. Why do you bring up this insane scenario that nobody
>> can possibly care about?
>>
>> So you actually have any reason to believe somebody does that?
>>
>> I already asked about that earlier, and the silence was deafening.
>
> I have no idea, but I'm generally uncomfortable with magical things
> that bypass normal security policy.
>
> That being said, here's an idea for fixing this, at least in the long
> run.  Add a new devpts mount option "no_ptmx_redirect" that turns off
> this behavior for the super in question.  That is, opening /dev/ptmx
> if "pts/ptmx" points to something with no_ptmx_redirect set will fail.
> Distros shipping new kernels could be encouraged to (finally!) make
> /dev/ptmx a symlink and set this option.
>
> We just might be able to get away with spelling that option "newinstance".

Linus, you said that people who want to protect their pts should deny
execute.  So I set it up:

# ls -l
total 0
crw---. 1 root root 5, 2 Apr 12 10:38 ptmx
drwx--. 2 root root0 Apr  2 11:35 pts

$ unshare -urm
# ls -l
total 0
crw---. 1 nfsnobody nfsnobody 5, 2 Apr 12 10:38 ptmx
drwx--. 2 nfsnobody nfsnobody0 Apr  2 11:35 pts
# mount --bind /dev/ptmx ptmx
# ls -l
total 0
crw-rw-rw-. 1 nfsnobody nfsnobody 5, 2 Apr 12 10:42 ptmx
drwx--. 2 nfsnobody nfsnobody0 Apr  2 11:35 pts

And there goes your protection.  So the whole /dev directory would
have to deny execute to protect against this.

But I think that gating this on mount options might be fine.  If
devpts is mounted with newinstance, then /dev/ptmx *already doesn't
work for it*, right?  So can we just say that the magic ptmx ->
pts/ptmx redirect doesn't work if the pts filesystem in question is
mounted with newinstance?

--Andy

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

[Eric W. Biederman]

Al Viro  writes:

> On Mon, Apr 11, 2016 at 07:10:47PM -0500, Eric W. Biederman wrote:
>> Actually for me this is about keeping the semantics simpler, and coming
>> up with a higher performance implementation.
>> 
>> A dentry that does an automount is already well defined.
>> 
>> Making the rule that accessing /dev/ptmx causes an automount of
>> /dev/pts/ptmx on top of the device node at /dev/ptmx is really simple,
>> with no special games.  It also makes it more obvious to userspace what
>> is going on.  AKA allows userspace to know which superblock does an open
>> ptmx master tty belongs to (and it happens in a backwards and forwards
>> compatible way).
>
> _What_ dentry?  Which filesystem would that be done to?  Whatever we have
> on /dev?  Or we suddenly get the fucking dentry operations change when
> dentry is attached to magical cdev inode?

Which dentry?  Any dentry that corresponds to the /dev/ptmx inode.
No filesystem changes just magic in init_special_inode that I have
not completely figured out yet.

If we can get an automount method in follow_automount from somewhere
cdev specific then a cdev can perform an automount comparitively
cleanly.  file_operations is attractive I am have not yet figured out a
clean method for passing the automount method yet.

For my proof of concept I am hardcoding things based on i_rdev.  Ugly
but servicable for testing out the idea.

A snip of my proof of concept code that seems to be working:

diff --git a/fs/inode.c b/fs/inode.c
index 69b8b526c194..d3de77b01a84 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -18,6 +18,7 @@
 #include  /* for inode_has_buffers */
 #include 
 #include 
+#include 
 #include 
 #include "internal.h"
 
@@ -1917,6 +1918,11 @@ void init_special_inode(struct inode *inode, umode_t 
mode, dev_t rdev)
if (S_ISCHR(mode)) {
inode->i_fop = _chr_fops;
inode->i_rdev = rdev;
+#if CONFIG_UNIX98_PTYS
+   if (rdev == MKDEV(TTYAUX_MAJOR, PTMX_MINOR)) {
+   inode->i_flags |= S_AUTOMOUNT;
+   }
+#endif
} else if (S_ISBLK(mode)) {
inode->i_fop = _blk_fops;
inode->i_rdev = rdev;

diff --git a/fs/namei.c b/fs/namei.c
index afb5137ca199..8894cf5fb43e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -35,6 +35,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 
 #include "internal.h"
@@ -1087,10 +1089,19 @@ EXPORT_SYMBOL(follow_up);
 static int follow_automount(struct path *path, struct nameidata *nd,
bool *need_mntput)
 {
+   struct vfsmount *(*automount)(struct path *) = NULL;
struct vfsmount *mnt;
int err;
 
-   if (!path->dentry->d_op || !path->dentry->d_op->d_automount)
+   if (path->dentry->d_op)
+   automount = path->dentry->d_op->d_automount;
+#if CONFIG_UNIX98_PTYS
+   if (path->dentry->d_inode &&
+   path->dentry->d_inode->i_rdev == MKDEV(TTYAUX_MAJOR, PTMX_MINOR)) {
+   automount = ptmx_automount;
+   }
+#endif
+   if (!automount)
return -EREMOTE;
 
/* We don't want to mount if someone's just doing a stat -
@@ -1113,7 +1124,7 @@ static int follow_automount(struct path *path, struct 
nameidata *nd,
if (nd->total_link_count >= 40)
return -ELOOP;
 
-   mnt = path->dentry->d_op->d_automount(path);
+   mnt = automount(path);
if (IS_ERR(mnt)) {
/*
 * The filesystem is allowed to return -EISDIR here to indicate

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

[Eric W. Biederman]

Al Viro  writes:

> On Mon, Apr 11, 2016 at 07:10:47PM -0500, Eric W. Biederman wrote:
>> Actually for me this is about keeping the semantics simpler, and coming
>> up with a higher performance implementation.
>> 
>> A dentry that does an automount is already well defined.
>> 
>> Making the rule that accessing /dev/ptmx causes an automount of
>> /dev/pts/ptmx on top of the device node at /dev/ptmx is really simple,
>> with no special games.  It also makes it more obvious to userspace what
>> is going on.  AKA allows userspace to know which superblock does an open
>> ptmx master tty belongs to (and it happens in a backwards and forwards
>> compatible way).
>
> _What_ dentry?  Which filesystem would that be done to?  Whatever we have
> on /dev?  Or we suddenly get the fucking dentry operations change when
> dentry is attached to magical cdev inode?

Which dentry?  Any dentry that corresponds to the /dev/ptmx inode.
No filesystem changes just magic in init_special_inode that I have
not completely figured out yet.

If we can get an automount method in follow_automount from somewhere
cdev specific then a cdev can perform an automount comparitively
cleanly.  file_operations is attractive I am have not yet figured out a
clean method for passing the automount method yet.

For my proof of concept I am hardcoding things based on i_rdev.  Ugly
but servicable for testing out the idea.

A snip of my proof of concept code that seems to be working:

diff --git a/fs/inode.c b/fs/inode.c
index 69b8b526c194..d3de77b01a84 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -18,6 +18,7 @@
 #include  /* for inode_has_buffers */
 #include 
 #include 
+#include 
 #include 
 #include "internal.h"
 
@@ -1917,6 +1918,11 @@ void init_special_inode(struct inode *inode, umode_t 
mode, dev_t rdev)
if (S_ISCHR(mode)) {
inode->i_fop = _chr_fops;
inode->i_rdev = rdev;
+#if CONFIG_UNIX98_PTYS
+   if (rdev == MKDEV(TTYAUX_MAJOR, PTMX_MINOR)) {
+   inode->i_flags |= S_AUTOMOUNT;
+   }
+#endif
} else if (S_ISBLK(mode)) {
inode->i_fop = _blk_fops;
inode->i_rdev = rdev;

diff --git a/fs/namei.c b/fs/namei.c
index afb5137ca199..8894cf5fb43e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -35,6 +35,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 
 #include "internal.h"
@@ -1087,10 +1089,19 @@ EXPORT_SYMBOL(follow_up);
 static int follow_automount(struct path *path, struct nameidata *nd,
bool *need_mntput)
 {
+   struct vfsmount *(*automount)(struct path *) = NULL;
struct vfsmount *mnt;
int err;
 
-   if (!path->dentry->d_op || !path->dentry->d_op->d_automount)
+   if (path->dentry->d_op)
+   automount = path->dentry->d_op->d_automount;
+#if CONFIG_UNIX98_PTYS
+   if (path->dentry->d_inode &&
+   path->dentry->d_inode->i_rdev == MKDEV(TTYAUX_MAJOR, PTMX_MINOR)) {
+   automount = ptmx_automount;
+   }
+#endif
+   if (!automount)
return -EREMOTE;
 
/* We don't want to mount if someone's just doing a stat -
@@ -1113,7 +1124,7 @@ static int follow_automount(struct path *path, struct 
nameidata *nd,
if (nd->total_link_count >= 40)
return -ELOOP;
 
-   mnt = path->dentry->d_op->d_automount(path);
+   mnt = automount(path);
if (IS_ERR(mnt)) {
/*
 * The filesystem is allowed to return -EISDIR here to indicate

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

[Al Viro]

On Mon, Apr 11, 2016 at 08:23:00PM -0500, Eric W. Biederman wrote:

> Perhaps I am reading the code wrong but as I read it that information is
> simply not available in get_super.

Correct.  For a very good reason - the same superblock can bloody well end up
in many places; having it tied to _the_ mountpoint is just plain wrong.

You know how it would look done right?
a) mount --after support
b) devpts containing both /ptmx and /pts/1,...
c) mount --type devpts --after none /dev

That's it.  And this would be way, way more useful that overlay-style unions;
it would *NOT* recurse into subdirectories.  Just a search list done right...

Not an option, unfortunately, since it obviously breaks userland setups -
even if we go ahead and implement that kind of non-recursive unions.  But
if we had a chance to design it from scratch, that would've been an interesting
option.

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

[Al Viro]

On Mon, Apr 11, 2016 at 08:23:00PM -0500, Eric W. Biederman wrote:

> Perhaps I am reading the code wrong but as I read it that information is
> simply not available in get_super.

Correct.  For a very good reason - the same superblock can bloody well end up
in many places; having it tied to _the_ mountpoint is just plain wrong.

You know how it would look done right?
a) mount --after support
b) devpts containing both /ptmx and /pts/1,...
c) mount --type devpts --after none /dev

That's it.  And this would be way, way more useful that overlay-style unions;
it would *NOT* recurse into subdirectories.  Just a search list done right...

Not an option, unfortunately, since it obviously breaks userland setups -
even if we go ahead and implement that kind of non-recursive unions.  But
if we had a chance to design it from scratch, that would've been an interesting
option.

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

[Al Viro]

On Mon, Apr 11, 2016 at 07:10:47PM -0500, Eric W. Biederman wrote:
> Actually for me this is about keeping the semantics simpler, and coming
> up with a higher performance implementation.
> 
> A dentry that does an automount is already well defined.
> 
> Making the rule that accessing /dev/ptmx causes an automount of
> /dev/pts/ptmx on top of the device node at /dev/ptmx is really simple,
> with no special games.  It also makes it more obvious to userspace what
> is going on.  AKA allows userspace to know which superblock does an open
> ptmx master tty belongs to (and it happens in a backwards and forwards
> compatible way).

_What_ dentry?  Which filesystem would that be done to?  Whatever we have
on /dev?  Or we suddenly get the fucking dentry operations change when
dentry is attached to magical cdev inode?

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

[Al Viro]

On Mon, Apr 11, 2016 at 07:10:47PM -0500, Eric W. Biederman wrote:
> Actually for me this is about keeping the semantics simpler, and coming
> up with a higher performance implementation.
> 
> A dentry that does an automount is already well defined.
> 
> Making the rule that accessing /dev/ptmx causes an automount of
> /dev/pts/ptmx on top of the device node at /dev/ptmx is really simple,
> with no special games.  It also makes it more obvious to userspace what
> is going on.  AKA allows userspace to know which superblock does an open
> ptmx master tty belongs to (and it happens in a backwards and forwards
> compatible way).

_What_ dentry?  Which filesystem would that be done to?  Whatever we have
on /dev?  Or we suddenly get the fucking dentry operations change when
dentry is attached to magical cdev inode?

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

[Eric W. Biederman]

"H. Peter Anvin"  writes:

> On April 11, 2016 5:10:47 PM PDT, ebied...@xmission.com wrote:
>>Linus Torvalds  writes:
>>
>>> On Mon, Apr 11, 2016 at 4:37 PM, Eric W. Biederman
>>>  wrote:

 My practical concern if we worked through the implementation details
 would be how would it interact with people who bind mount
>>/dev/pts/ptmx
 on top of /dev/ptmx.  We might get some strange new errors.
>>>
>>> Yes, please don't let's play "clever" games. The semantics should be
>>> fairly straightforward.
>>
>>Actually for me this is about keeping the semantics simpler, and coming
>>up with a higher performance implementation.
>>
>>A dentry that does an automount is already well defined.
>>
>>Making the rule that accessing /dev/ptmx causes an automount of
>>/dev/pts/ptmx on top of the device node at /dev/ptmx is really simple,
>>with no special games.  It also makes it more obvious to userspace what
>>is going on.  AKA allows userspace to know which superblock does an
>>open
>>ptmx master tty belongs to (and it happens in a backwards and forwards
>>compatible way).
>>
>>My only concern is with this very minor change in semantics will
>>anything care.  I need to implement and test to find out.
>>
>>I think I see an implementation that Al won't grumble too loudly about.
>>
>>Anyway I am going to try this and see what I can see.
>>
>>Eric
>
> Why bother with an automount?  You can look up ../ptmx from the devpts
> get_super method and just do the bind mount once.  No fuss, no muss.
> What's wrong with that?

Perhaps I am reading the code wrong but as I read it that information is
simply not available in get_super.

Eric

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

[Eric W. Biederman]

"H. Peter Anvin"  writes:

> On April 11, 2016 5:10:47 PM PDT, ebied...@xmission.com wrote:
>>Linus Torvalds  writes:
>>
>>> On Mon, Apr 11, 2016 at 4:37 PM, Eric W. Biederman
>>>  wrote:

 My practical concern if we worked through the implementation details
 would be how would it interact with people who bind mount
>>/dev/pts/ptmx
 on top of /dev/ptmx.  We might get some strange new errors.
>>>
>>> Yes, please don't let's play "clever" games. The semantics should be
>>> fairly straightforward.
>>
>>Actually for me this is about keeping the semantics simpler, and coming
>>up with a higher performance implementation.
>>
>>A dentry that does an automount is already well defined.
>>
>>Making the rule that accessing /dev/ptmx causes an automount of
>>/dev/pts/ptmx on top of the device node at /dev/ptmx is really simple,
>>with no special games.  It also makes it more obvious to userspace what
>>is going on.  AKA allows userspace to know which superblock does an
>>open
>>ptmx master tty belongs to (and it happens in a backwards and forwards
>>compatible way).
>>
>>My only concern is with this very minor change in semantics will
>>anything care.  I need to implement and test to find out.
>>
>>I think I see an implementation that Al won't grumble too loudly about.
>>
>>Anyway I am going to try this and see what I can see.
>>
>>Eric
>
> Why bother with an automount?  You can look up ../ptmx from the devpts
> get_super method and just do the bind mount once.  No fuss, no muss.
> What's wrong with that?

Perhaps I am reading the code wrong but as I read it that information is
simply not available in get_super.

Eric

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

[Al Viro]

On Mon, Apr 11, 2016 at 07:48:28AM -0700, H. Peter Anvin wrote:
 
> Here is an entire different approach, I don't know if it is sane or not: when 
> *mounting* the devpts filesystem, it could automagically create the bins 
> mount for ptmx in the parent of its mount point.  Presumably the would be a 
> mount option to disable that behavior.
> 
> Does anyone see an obvious problem with that?

Yes.  ->mount() doesn't (and fucking *shouldn't*) know anything about the
mountpoint to be.  Not to mention that the same superblock can easily end
up being visible in many places, etc.  This is insane.

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

[Al Viro]

On Mon, Apr 11, 2016 at 07:48:28AM -0700, H. Peter Anvin wrote:
 
> Here is an entire different approach, I don't know if it is sane or not: when 
> *mounting* the devpts filesystem, it could automagically create the bins 
> mount for ptmx in the parent of its mount point.  Presumably the would be a 
> mount option to disable that behavior.
> 
> Does anyone see an obvious problem with that?

Yes.  ->mount() doesn't (and fucking *shouldn't*) know anything about the
mountpoint to be.  Not to mention that the same superblock can easily end
up being visible in many places, etc.  This is insane.

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

[Linus Torvalds]

On Mon, Apr 11, 2016 at 6:06 PM, H. Peter Anvin  wrote:
>
> Why bother with an automount?  You can look up ../ptmx from the devpts 
> get_super method and just do the bind mount once.  No fuss, no muss.  What's 
> wrong with that?

Ehh. What if somebody wants to mount the same devpts in multiple
places? So now you need to do the bind mount every time devpts is
bindmounted?

None of this makes sense.

Let's just take Eric's patch, and strip out the permission check, and
strip out the code that fakes a new path for it.

That gets rid of 90% of devpts_path_ptmx(): all that remains is pretty
much the "are we already in devpts" and the call to "path_pts()"
thing.

No update_file_path(), no inode_permissions, no fsi->ptmx_dentry
games. Just get a reference to the "pts_fs_info", and it's all done.

(Getting a ref on the pts_fs_info might require us to have a ref to
the superblock, I didn't check that part. But rather than updating the
file path, just save it off in the file data).

   Linus

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

[Linus Torvalds]

On Mon, Apr 11, 2016 at 6:06 PM, H. Peter Anvin  wrote:
>
> Why bother with an automount?  You can look up ../ptmx from the devpts 
> get_super method and just do the bind mount once.  No fuss, no muss.  What's 
> wrong with that?

Ehh. What if somebody wants to mount the same devpts in multiple
places? So now you need to do the bind mount every time devpts is
bindmounted?

None of this makes sense.

Let's just take Eric's patch, and strip out the permission check, and
strip out the code that fakes a new path for it.

That gets rid of 90% of devpts_path_ptmx(): all that remains is pretty
much the "are we already in devpts" and the call to "path_pts()"
thing.

No update_file_path(), no inode_permissions, no fsi->ptmx_dentry
games. Just get a reference to the "pts_fs_info", and it's all done.

(Getting a ref on the pts_fs_info might require us to have a ref to
the superblock, I didn't check that part. But rather than updating the
file path, just save it off in the file data).

   Linus

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

[H. Peter Anvin]

On April 11, 2016 5:10:47 PM PDT, ebied...@xmission.com wrote:
>Linus Torvalds  writes:
>
>> On Mon, Apr 11, 2016 at 4:37 PM, Eric W. Biederman
>>  wrote:
>>>
>>> My practical concern if we worked through the implementation details
>>> would be how would it interact with people who bind mount
>/dev/pts/ptmx
>>> on top of /dev/ptmx.  We might get some strange new errors.
>>
>> Yes, please don't let's play "clever" games. The semantics should be
>> fairly straightforward.
>
>Actually for me this is about keeping the semantics simpler, and coming
>up with a higher performance implementation.
>
>A dentry that does an automount is already well defined.
>
>Making the rule that accessing /dev/ptmx causes an automount of
>/dev/pts/ptmx on top of the device node at /dev/ptmx is really simple,
>with no special games.  It also makes it more obvious to userspace what
>is going on.  AKA allows userspace to know which superblock does an
>open
>ptmx master tty belongs to (and it happens in a backwards and forwards
>compatible way).
>
>My only concern is with this very minor change in semantics will
>anything care.  I need to implement and test to find out.
>
>I think I see an implementation that Al won't grumble too loudly about.
>
>Anyway I am going to try this and see what I can see.
>
>Eric

Why bother with an automount?  You can look up ../ptmx from the devpts 
get_super method and just do the bind mount once.  No fuss, no muss.  What's 
wrong with that?
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

[H. Peter Anvin]

On April 11, 2016 5:10:47 PM PDT, ebied...@xmission.com wrote:
>Linus Torvalds  writes:
>
>> On Mon, Apr 11, 2016 at 4:37 PM, Eric W. Biederman
>>  wrote:
>>>
>>> My practical concern if we worked through the implementation details
>>> would be how would it interact with people who bind mount
>/dev/pts/ptmx
>>> on top of /dev/ptmx.  We might get some strange new errors.
>>
>> Yes, please don't let's play "clever" games. The semantics should be
>> fairly straightforward.
>
>Actually for me this is about keeping the semantics simpler, and coming
>up with a higher performance implementation.
>
>A dentry that does an automount is already well defined.
>
>Making the rule that accessing /dev/ptmx causes an automount of
>/dev/pts/ptmx on top of the device node at /dev/ptmx is really simple,
>with no special games.  It also makes it more obvious to userspace what
>is going on.  AKA allows userspace to know which superblock does an
>open
>ptmx master tty belongs to (and it happens in a backwards and forwards
>compatible way).
>
>My only concern is with this very minor change in semantics will
>anything care.  I need to implement and test to find out.
>
>I think I see an implementation that Al won't grumble too loudly about.
>
>Anyway I am going to try this and see what I can see.
>
>Eric

Why bother with an automount?  You can look up ../ptmx from the devpts 
get_super method and just do the bind mount once.  No fuss, no muss.  What's 
wrong with that?
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

[Linus Torvalds]

On Mon, Apr 11, 2016 at 5:22 PM, Eric W. Biederman
 wrote:
>
> I meant the one where I conceded that the only think that it could
> possible protect against was a denial of service attack, from which we
> probably don't care.

Yeah, that's the same email I was talking about, I was just quoting
another part.

> As I agreed with you that it was unnecessary I was just puzzled why you
> called what was essentially agreement with you deafening silence.

The "deafening silence" was about _why_ this all would be a problem,
and why the security checks would be needed.

Basically, I think that if /dev/pts/ is accessible, we should just say
"ok, you can open a pty on it".

The fact that you could open a pty by bind-mounting it somewhere else,
and then adding a "ptmx" node to the same directory is not a security
issue: it's simply how devpts works.

In no actual sane and relevant situation is that a problem, for the
simple fact that there will *already* be a ptmx node that is
world-accessible in the same directory that has that /dev/pts/ mount.

Anything else is insane and irrelevant. This is *literally* what POSIX
says. Sure, POSIX also has that whole language about "posix_openpt()",
but that's just BS and irrelevant. The very page that mentions
"posix_openpt()" also says

  "On implementations supporting the /dev/ptmx clone device, opening
the master device of a pseudo-terminal is simply:

mfdp = open("/dev/ptmx", oflag );
if (mfdp < 0)
return -1;"

and Linux unquestionably falls in that "supports /dev/ptmx" camp.

So I claim that the _only_ sane use of devpts is to already have a
world-accessible ptmx node there, and nothing else makes sense.

And if you want to be private, you had better make the whole /dev/
subdirectory private (which also takes care of any bind mount issues)

Linus

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

[Linus Torvalds]

On Mon, Apr 11, 2016 at 5:22 PM, Eric W. Biederman
 wrote:
>
> I meant the one where I conceded that the only think that it could
> possible protect against was a denial of service attack, from which we
> probably don't care.

Yeah, that's the same email I was talking about, I was just quoting
another part.

> As I agreed with you that it was unnecessary I was just puzzled why you
> called what was essentially agreement with you deafening silence.

The "deafening silence" was about _why_ this all would be a problem,
and why the security checks would be needed.

Basically, I think that if /dev/pts/ is accessible, we should just say
"ok, you can open a pty on it".

The fact that you could open a pty by bind-mounting it somewhere else,
and then adding a "ptmx" node to the same directory is not a security
issue: it's simply how devpts works.

In no actual sane and relevant situation is that a problem, for the
simple fact that there will *already* be a ptmx node that is
world-accessible in the same directory that has that /dev/pts/ mount.

Anything else is insane and irrelevant. This is *literally* what POSIX
says. Sure, POSIX also has that whole language about "posix_openpt()",
but that's just BS and irrelevant. The very page that mentions
"posix_openpt()" also says

  "On implementations supporting the /dev/ptmx clone device, opening
the master device of a pseudo-terminal is simply:

mfdp = open("/dev/ptmx", oflag );
if (mfdp < 0)
return -1;"

and Linux unquestionably falls in that "supports /dev/ptmx" camp.

So I claim that the _only_ sane use of devpts is to already have a
world-accessible ptmx node there, and nothing else makes sense.

And if you want to be private, you had better make the whole /dev/
subdirectory private (which also takes care of any bind mount issues)

Linus

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

[Eric W. Biederman]

Linus Torvalds  writes:

> On Mon, Apr 11, 2016 at 4:49 PM, Eric W. Biederman
>  wrote:
>>
>> I replied earlier.  Did you not see my reply?
>
>
> Are you talking about the one where you agreed that the scenario was
> made up and insane? The one where you said that you're worried about
> breaking out "extension" where ptmx is non-0666?

I meant the one where I conceded that the only think that it could
possible protect against was a denial of service attack, from which we
probably don't care.

I just want to be certain that the emails are getting through.  As the
meaning certainly has not been.

I do think I called a permision check in posix_open aka on /dev/ptmx
a linux specific extension in that email.   But seriously it was all
about reducing the scope of the change.  Reducing the size of the test
matrix.  I simply had not looked far enough to see if there was anything
you could reasonable protect with those permissions.

As I agreed with you that it was unnecessary I was just puzzled why you
called what was essentially agreement with you deafening silence.

Eric

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

[Eric W. Biederman]

Linus Torvalds  writes:

> On Mon, Apr 11, 2016 at 4:49 PM, Eric W. Biederman
>  wrote:
>>
>> I replied earlier.  Did you not see my reply?
>
>
> Are you talking about the one where you agreed that the scenario was
> made up and insane? The one where you said that you're worried about
> breaking out "extension" where ptmx is non-0666?

I meant the one where I conceded that the only think that it could
possible protect against was a denial of service attack, from which we
probably don't care.

I just want to be certain that the emails are getting through.  As the
meaning certainly has not been.

I do think I called a permision check in posix_open aka on /dev/ptmx
a linux specific extension in that email.   But seriously it was all
about reducing the scope of the change.  Reducing the size of the test
matrix.  I simply had not looked far enough to see if there was anything
you could reasonable protect with those permissions.

As I agreed with you that it was unnecessary I was just puzzled why you
called what was essentially agreement with you deafening silence.

Eric

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

[Eric W. Biederman]

Linus Torvalds  writes:

> On Mon, Apr 11, 2016 at 4:37 PM, Eric W. Biederman
>  wrote:
>>
>> My practical concern if we worked through the implementation details
>> would be how would it interact with people who bind mount /dev/pts/ptmx
>> on top of /dev/ptmx.  We might get some strange new errors.
>
> Yes, please don't let's play "clever" games. The semantics should be
> fairly straightforward.

Actually for me this is about keeping the semantics simpler, and coming
up with a higher performance implementation.

A dentry that does an automount is already well defined.

Making the rule that accessing /dev/ptmx causes an automount of
/dev/pts/ptmx on top of the device node at /dev/ptmx is really simple,
with no special games.  It also makes it more obvious to userspace what
is going on.  AKA allows userspace to know which superblock does an open
ptmx master tty belongs to (and it happens in a backwards and forwards
compatible way).

My only concern is with this very minor change in semantics will
anything care.  I need to implement and test to find out.

I think I see an implementation that Al won't grumble too loudly about.

Anyway I am going to try this and see what I can see.

Eric

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

[Eric W. Biederman]

Linus Torvalds  writes:

> On Mon, Apr 11, 2016 at 4:37 PM, Eric W. Biederman
>  wrote:
>>
>> My practical concern if we worked through the implementation details
>> would be how would it interact with people who bind mount /dev/pts/ptmx
>> on top of /dev/ptmx.  We might get some strange new errors.
>
> Yes, please don't let's play "clever" games. The semantics should be
> fairly straightforward.

Actually for me this is about keeping the semantics simpler, and coming
up with a higher performance implementation.

A dentry that does an automount is already well defined.

Making the rule that accessing /dev/ptmx causes an automount of
/dev/pts/ptmx on top of the device node at /dev/ptmx is really simple,
with no special games.  It also makes it more obvious to userspace what
is going on.  AKA allows userspace to know which superblock does an open
ptmx master tty belongs to (and it happens in a backwards and forwards
compatible way).

My only concern is with this very minor change in semantics will
anything care.  I need to implement and test to find out.

I think I see an implementation that Al won't grumble too loudly about.

Anyway I am going to try this and see what I can see.

Eric

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

[Linus Torvalds]

On Mon, Apr 11, 2016 at 4:49 PM, Eric W. Biederman
 wrote:
>
> I replied earlier.  Did you not see my reply?

Are you talking about the one where you agreed that the scenario was
made up and insane? The one where you said that you're worried about
breaking out "extension" where ptmx is non-0666?

That was never an extension. It was a simple situation of people (a)
not knowing what the tty group should be in the kernel and (b) then
thinking that using a permission model of "no permission" somehow made
it saner.

What it actually resulted in was that most distros just ignore it
entirely, and just use /dev/ptmx.

Yes, you *can* then chmod it in user space and use a symlink, but so
what? Nobody who actually uses that node uses anythinig but 0666.
Because that would break pretty much everything that uses pty's.

So the whole "we need to worry about permission " is complete and
uttter garbage. We really don't. The situation doesn't come up, and
it's not relevant. The standard part to access ptmx is /dev/ptmx, and
no amount of wishing it were otherwise will make it any different.

Seriously. Just look at the opengroup documentation. It talks about
/dev/ptmx. The whole /dev/pts/ptmx thing was a mistake. WE SHOULD NOT
EXTEND ON THAT MISTAKE.

We should just FIX the mistake. Ignore /dev/pts/ptmx, because that
node is non-standard SHIT.

Really. Really really.

 Linus

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

[Linus Torvalds]

On Mon, Apr 11, 2016 at 4:49 PM, Eric W. Biederman
 wrote:
>
> I replied earlier.  Did you not see my reply?

Are you talking about the one where you agreed that the scenario was
made up and insane? The one where you said that you're worried about
breaking out "extension" where ptmx is non-0666?

That was never an extension. It was a simple situation of people (a)
not knowing what the tty group should be in the kernel and (b) then
thinking that using a permission model of "no permission" somehow made
it saner.

What it actually resulted in was that most distros just ignore it
entirely, and just use /dev/ptmx.

Yes, you *can* then chmod it in user space and use a symlink, but so
what? Nobody who actually uses that node uses anythinig but 0666.
Because that would break pretty much everything that uses pty's.

So the whole "we need to worry about permission " is complete and
uttter garbage. We really don't. The situation doesn't come up, and
it's not relevant. The standard part to access ptmx is /dev/ptmx, and
no amount of wishing it were otherwise will make it any different.

Seriously. Just look at the opengroup documentation. It talks about
/dev/ptmx. The whole /dev/pts/ptmx thing was a mistake. WE SHOULD NOT
EXTEND ON THAT MISTAKE.

We should just FIX the mistake. Ignore /dev/pts/ptmx, because that
node is non-standard SHIT.

Really. Really really.

 Linus

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

[Linus Torvalds]

On Mon, Apr 11, 2016 at 4:37 PM, Eric W. Biederman
 wrote:
>
> My practical concern if we worked through the implementation details
> would be how would it interact with people who bind mount /dev/pts/ptmx
> on top of /dev/ptmx.  We might get some strange new errors.

Yes, please don't let's play "clever" games. The semantics should be
fairly straightforward.

I still don't understand why people think that you shouldn't be able
to access a 'pts' subsystem that is accessible to others. If you can
bind-mount the pts directory somewhere, then you can damn well already
see that pts mount, claiming that somehow it should be sacred ground
and you shouldn't be able to access it with a ptmx node outside of it
is just insane.

So people have been bringing that up as an issue, but nobody has ever
actually been able to articulate why anybody should ever care.

Now people are just making up random odd semantics. Nobody has ever
explained why the _simple_ "ptmx binds to the pts directory next to
it" is actually problem. Even for a bind mount, you have to be able to
open the point you're mounting, so we know that the "attacker" already
had access to the pts subdirectory.

If somebody wants to keep the pts mount private, they should damn well
keep it _private_. I don't understand peoples "oh, you can access it
but you can't access it".excuses.

  Linus

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

[Linus Torvalds]

On Mon, Apr 11, 2016 at 4:37 PM, Eric W. Biederman
 wrote:
>
> My practical concern if we worked through the implementation details
> would be how would it interact with people who bind mount /dev/pts/ptmx
> on top of /dev/ptmx.  We might get some strange new errors.

Yes, please don't let's play "clever" games. The semantics should be
fairly straightforward.

I still don't understand why people think that you shouldn't be able
to access a 'pts' subsystem that is accessible to others. If you can
bind-mount the pts directory somewhere, then you can damn well already
see that pts mount, claiming that somehow it should be sacred ground
and you shouldn't be able to access it with a ptmx node outside of it
is just insane.

So people have been bringing that up as an issue, but nobody has ever
actually been able to articulate why anybody should ever care.

Now people are just making up random odd semantics. Nobody has ever
explained why the _simple_ "ptmx binds to the pts directory next to
it" is actually problem. Even for a bind mount, you have to be able to
open the point you're mounting, so we know that the "attacker" already
had access to the pts subdirectory.

If somebody wants to keep the pts mount private, they should damn well
keep it _private_. I don't understand peoples "oh, you can access it
but you can't access it".excuses.

  Linus

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

[Eric W. Biederman]

Linus Torvalds  writes:

> On Apr 9, 2016 5:45 PM, "Andy Lutomirski"  wrote:
>>
>>
>> What we *do* want to do, though, is to prevent the following:
>
> I don't see the point. Why do you bring up this insane scenario that
> nobody can possibly care about?
>
> So you actually have any reason to believe somebody does that?
>
> I already asked about that earlier, and the silence was deafening.

I replied earlier.  Did you not see my reply?

Eric

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

[Eric W. Biederman]

Linus Torvalds  writes:

> On Apr 9, 2016 5:45 PM, "Andy Lutomirski"  wrote:
>>
>>
>> What we *do* want to do, though, is to prevent the following:
>
> I don't see the point. Why do you bring up this insane scenario that
> nobody can possibly care about?
>
> So you actually have any reason to believe somebody does that?
>
> I already asked about that earlier, and the silence was deafening.

I replied earlier.  Did you not see my reply?

Eric

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

[Eric W. Biederman]

"H. Peter Anvin"  writes:

> On April 11, 2016 1:12:22 PM PDT, Andy Lutomirski  wrote:
>>On Sat, Apr 9, 2016 at 6:27 PM, Linus Torvalds
>> wrote:
>>>
>>> On Apr 9, 2016 5:45 PM, "Andy Lutomirski" 
>>wrote:


 What we *do* want to do, though, is to prevent the following:
>>>
>>> I don't see the point. Why do you bring up this insane scenario that
>>nobody
>>> can possibly care about?
>>>
>>> So you actually have any reason to believe somebody does that?
>>>
>>> I already asked about that earlier, and the silence was deafening.
>>
>>I have no idea, but I'm generally uncomfortable with magical things
>>that bypass normal security policy.
>>
>>That being said, here's an idea for fixing this, at least in the long
>>run.  Add a new devpts mount option "no_ptmx_redirect" that turns off
>>this behavior for the super in question.  That is, opening /dev/ptmx
>>if "pts/ptmx" points to something with no_ptmx_redirect set will fail.
>>Distros shipping new kernels could be encouraged to (finally!) make
>>/dev/ptmx a symlink and set this option.
>>
>>We just might be able to get away with spelling that option
>>"newinstance".
>
> What about the idea of making the bind mount automatic?

We could almost do that cleanly by playing with the /dev/ptmx dentry
and implementing a d_automount method.  That still needs the crazy path
based lookup without permission checks.

Unfortunately the filesystem not the device owns the dentry operations.

My practical concern if we worked through the implementation details
would be how would it interact with people who bind mount /dev/pts/ptmx
on top of /dev/ptmx.  We might get some strange new errors.

Eric

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

[Eric W. Biederman]

"H. Peter Anvin"  writes:

> On April 11, 2016 1:12:22 PM PDT, Andy Lutomirski  wrote:
>>On Sat, Apr 9, 2016 at 6:27 PM, Linus Torvalds
>> wrote:
>>>
>>> On Apr 9, 2016 5:45 PM, "Andy Lutomirski" 
>>wrote:


 What we *do* want to do, though, is to prevent the following:
>>>
>>> I don't see the point. Why do you bring up this insane scenario that
>>nobody
>>> can possibly care about?
>>>
>>> So you actually have any reason to believe somebody does that?
>>>
>>> I already asked about that earlier, and the silence was deafening.
>>
>>I have no idea, but I'm generally uncomfortable with magical things
>>that bypass normal security policy.
>>
>>That being said, here's an idea for fixing this, at least in the long
>>run.  Add a new devpts mount option "no_ptmx_redirect" that turns off
>>this behavior for the super in question.  That is, opening /dev/ptmx
>>if "pts/ptmx" points to something with no_ptmx_redirect set will fail.
>>Distros shipping new kernels could be encouraged to (finally!) make
>>/dev/ptmx a symlink and set this option.
>>
>>We just might be able to get away with spelling that option
>>"newinstance".
>
> What about the idea of making the bind mount automatic?

We could almost do that cleanly by playing with the /dev/ptmx dentry
and implementing a d_automount method.  That still needs the crazy path
based lookup without permission checks.

Unfortunately the filesystem not the device owns the dentry operations.

My practical concern if we worked through the implementation details
would be how would it interact with people who bind mount /dev/pts/ptmx
on top of /dev/ptmx.  We might get some strange new errors.

Eric

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

[H. Peter Anvin]

On April 11, 2016 1:12:22 PM PDT, Andy Lutomirski  wrote:
>On Sat, Apr 9, 2016 at 6:27 PM, Linus Torvalds
> wrote:
>>
>> On Apr 9, 2016 5:45 PM, "Andy Lutomirski" 
>wrote:
>>>
>>>
>>> What we *do* want to do, though, is to prevent the following:
>>
>> I don't see the point. Why do you bring up this insane scenario that
>nobody
>> can possibly care about?
>>
>> So you actually have any reason to believe somebody does that?
>>
>> I already asked about that earlier, and the silence was deafening.
>
>I have no idea, but I'm generally uncomfortable with magical things
>that bypass normal security policy.
>
>That being said, here's an idea for fixing this, at least in the long
>run.  Add a new devpts mount option "no_ptmx_redirect" that turns off
>this behavior for the super in question.  That is, opening /dev/ptmx
>if "pts/ptmx" points to something with no_ptmx_redirect set will fail.
>Distros shipping new kernels could be encouraged to (finally!) make
>/dev/ptmx a symlink and set this option.
>
>We just might be able to get away with spelling that option
>"newinstance".

What about the idea of making the bind mount automatic?
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

[H. Peter Anvin]

On April 11, 2016 1:12:22 PM PDT, Andy Lutomirski  wrote:
>On Sat, Apr 9, 2016 at 6:27 PM, Linus Torvalds
> wrote:
>>
>> On Apr 9, 2016 5:45 PM, "Andy Lutomirski" 
>wrote:
>>>
>>>
>>> What we *do* want to do, though, is to prevent the following:
>>
>> I don't see the point. Why do you bring up this insane scenario that
>nobody
>> can possibly care about?
>>
>> So you actually have any reason to believe somebody does that?
>>
>> I already asked about that earlier, and the silence was deafening.
>
>I have no idea, but I'm generally uncomfortable with magical things
>that bypass normal security policy.
>
>That being said, here's an idea for fixing this, at least in the long
>run.  Add a new devpts mount option "no_ptmx_redirect" that turns off
>this behavior for the super in question.  That is, opening /dev/ptmx
>if "pts/ptmx" points to something with no_ptmx_redirect set will fail.
>Distros shipping new kernels could be encouraged to (finally!) make
>/dev/ptmx a symlink and set this option.
>
>We just might be able to get away with spelling that option
>"newinstance".

What about the idea of making the bind mount automatic?
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

[Eric W. Biederman]

Andy Lutomirski  writes:

> On Sat, Apr 9, 2016 at 6:27 PM, Linus Torvalds
>  wrote:
>>
>> On Apr 9, 2016 5:45 PM, "Andy Lutomirski"  wrote:
>>>
>>>
>>> What we *do* want to do, though, is to prevent the following:
>>
>> I don't see the point. Why do you bring up this insane scenario that nobody
>> can possibly care about?
>>
>> So you actually have any reason to believe somebody does that?
>>
>> I already asked about that earlier, and the silence was deafening.
>
> I have no idea, but I'm generally uncomfortable with magical things
> that bypass normal security policy.
>
> That being said, here's an idea for fixing this, at least in the long
> run.  Add a new devpts mount option "no_ptmx_redirect" that turns off
> this behavior for the super in question.  That is, opening /dev/ptmx
> if "pts/ptmx" points to something with no_ptmx_redirect set will fail.
> Distros shipping new kernels could be encouraged to (finally!) make
> /dev/ptmx a symlink and set this option.
>
> We just might be able to get away with spelling that option "newinstance".

Interesting point.  Very interesting point.  At this point I don't know
that it is worth it, but that would trivially prevent any non-sense,
that might possibly happen.   The downside would be that the semantics
of /dev/ptmx would be more complicated.

Eric

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

[Eric W. Biederman]

Andy Lutomirski  writes:

> On Sat, Apr 9, 2016 at 6:27 PM, Linus Torvalds
>  wrote:
>>
>> On Apr 9, 2016 5:45 PM, "Andy Lutomirski"  wrote:
>>>
>>>
>>> What we *do* want to do, though, is to prevent the following:
>>
>> I don't see the point. Why do you bring up this insane scenario that nobody
>> can possibly care about?
>>
>> So you actually have any reason to believe somebody does that?
>>
>> I already asked about that earlier, and the silence was deafening.
>
> I have no idea, but I'm generally uncomfortable with magical things
> that bypass normal security policy.
>
> That being said, here's an idea for fixing this, at least in the long
> run.  Add a new devpts mount option "no_ptmx_redirect" that turns off
> this behavior for the super in question.  That is, opening /dev/ptmx
> if "pts/ptmx" points to something with no_ptmx_redirect set will fail.
> Distros shipping new kernels could be encouraged to (finally!) make
> /dev/ptmx a symlink and set this option.
>
> We just might be able to get away with spelling that option "newinstance".

Interesting point.  Very interesting point.  At this point I don't know
that it is worth it, but that would trivially prevent any non-sense,
that might possibly happen.   The downside would be that the semantics
of /dev/ptmx would be more complicated.

Eric

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

[Eric W. Biederman]

Linus Torvalds  writes:

> On Sat, Apr 9, 2016 at 3:37 PM, H. Peter Anvin  wrote:
>>
>> On the flipside, if we were to allow ourselves to break userspace, at this 
>> point I would suggest making /dev/pts/ptmx have a different device number 
>> and make the legacy /dev/ptmx print a warning message, after which it can at 
>> least eventually be deleted.
>
> You don't need a different device number.
>
> The /dev/pts/ptmx file may look like it's the same node as /dev/ptmx,
> but it is trivial to recognize as the pts one:
>
>  if (dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC)
>
> and you're done.

We can actually do better and set f_ops in devpts and bypass the whole
lookup by device number.  In my pile of cleanups that are waiting for
the mess to resolve I actually have a patch that does that for the slave
ttys.

>
> But nobody actually uses /dev/pts/ptmx, because it has never had sane
> permissions.

Now that is an interesting misconception to see.  There is actually a
lot more software that uses /dev/pts/ptmx (with a symlink from /dev/ptmx
or a bind mount to /dev/ptmx) than there is that actually needs the new
compatibility behavior.

Carefully written and maintained container software like lxc and docker
do use "-o newinstance".  Fixing the permissions and redirecting the
/dev/ptmx path to /dev/pts/ptmx are not a problem when you know you are
setting up a special environment.

It is the one off sloppily created automation scripts like
xen-create-image that gets this wrong.  I say sloppily created as in
practice every mount of devpts needs "-o gid=5,mode=620".  If
xen-create-image and related software had gotting those mount options
correct pt_chown could have been done away with, with no one noticing a
long time ago.

Those sloppy pieces of code are probably the things we want to
break least as they work after their fasion and whoever wrote them
is likely long gone.  So I would be surprised if there was anyone to
pick up the pieces if we break them.

> But when we fix bad semantics (and always just looking up the initial
> pts mount really is crazy semantics) that doesn't mean that we have to
> bend over backwards to not make the changed semantics visible. We
> don't _break_ user space, but we also don't care about some random
> test-program that checks for particular semantics.

No.  Bending over backwards as you call it just makes the software test
matrix smaller.

That part is now settled in my book and those extra permission checks
will not be in the next version of this patchset.

Eric

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

[Eric W. Biederman]

Linus Torvalds  writes:

> On Sat, Apr 9, 2016 at 3:37 PM, H. Peter Anvin  wrote:
>>
>> On the flipside, if we were to allow ourselves to break userspace, at this 
>> point I would suggest making /dev/pts/ptmx have a different device number 
>> and make the legacy /dev/ptmx print a warning message, after which it can at 
>> least eventually be deleted.
>
> You don't need a different device number.
>
> The /dev/pts/ptmx file may look like it's the same node as /dev/ptmx,
> but it is trivial to recognize as the pts one:
>
>  if (dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC)
>
> and you're done.

We can actually do better and set f_ops in devpts and bypass the whole
lookup by device number.  In my pile of cleanups that are waiting for
the mess to resolve I actually have a patch that does that for the slave
ttys.

>
> But nobody actually uses /dev/pts/ptmx, because it has never had sane
> permissions.

Now that is an interesting misconception to see.  There is actually a
lot more software that uses /dev/pts/ptmx (with a symlink from /dev/ptmx
or a bind mount to /dev/ptmx) than there is that actually needs the new
compatibility behavior.

Carefully written and maintained container software like lxc and docker
do use "-o newinstance".  Fixing the permissions and redirecting the
/dev/ptmx path to /dev/pts/ptmx are not a problem when you know you are
setting up a special environment.

It is the one off sloppily created automation scripts like
xen-create-image that gets this wrong.  I say sloppily created as in
practice every mount of devpts needs "-o gid=5,mode=620".  If
xen-create-image and related software had gotting those mount options
correct pt_chown could have been done away with, with no one noticing a
long time ago.

Those sloppy pieces of code are probably the things we want to
break least as they work after their fasion and whoever wrote them
is likely long gone.  So I would be surprised if there was anyone to
pick up the pieces if we break them.

> But when we fix bad semantics (and always just looking up the initial
> pts mount really is crazy semantics) that doesn't mean that we have to
> bend over backwards to not make the changed semantics visible. We
> don't _break_ user space, but we also don't care about some random
> test-program that checks for particular semantics.

No.  Bending over backwards as you call it just makes the software test
matrix smaller.

That part is now settled in my book and those extra permission checks
will not be in the next version of this patchset.

Eric

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

[Andy Lutomirski]

On Sat, Apr 9, 2016 at 6:27 PM, Linus Torvalds
 wrote:
>
> On Apr 9, 2016 5:45 PM, "Andy Lutomirski"  wrote:
>>
>>
>> What we *do* want to do, though, is to prevent the following:
>
> I don't see the point. Why do you bring up this insane scenario that nobody
> can possibly care about?
>
> So you actually have any reason to believe somebody does that?
>
> I already asked about that earlier, and the silence was deafening.

I have no idea, but I'm generally uncomfortable with magical things
that bypass normal security policy.

That being said, here's an idea for fixing this, at least in the long
run.  Add a new devpts mount option "no_ptmx_redirect" that turns off
this behavior for the super in question.  That is, opening /dev/ptmx
if "pts/ptmx" points to something with no_ptmx_redirect set will fail.
Distros shipping new kernels could be encouraged to (finally!) make
/dev/ptmx a symlink and set this option.

We just might be able to get away with spelling that option "newinstance".

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

[Andy Lutomirski]

On Sat, Apr 9, 2016 at 6:27 PM, Linus Torvalds
 wrote:
>
> On Apr 9, 2016 5:45 PM, "Andy Lutomirski"  wrote:
>>
>>
>> What we *do* want to do, though, is to prevent the following:
>
> I don't see the point. Why do you bring up this insane scenario that nobody
> can possibly care about?
>
> So you actually have any reason to believe somebody does that?
>
> I already asked about that earlier, and the silence was deafening.

I have no idea, but I'm generally uncomfortable with magical things
that bypass normal security policy.

That being said, here's an idea for fixing this, at least in the long
run.  Add a new devpts mount option "no_ptmx_redirect" that turns off
this behavior for the super in question.  That is, opening /dev/ptmx
if "pts/ptmx" points to something with no_ptmx_redirect set will fail.
Distros shipping new kernels could be encouraged to (finally!) make
/dev/ptmx a symlink and set this option.

We just might be able to get away with spelling that option "newinstance".

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

[H. Peter Anvin]

On April 9, 2016 6:27:36 PM PDT, Linus Torvalds  
wrote:
>On Apr 9, 2016 5:45 PM, "Andy Lutomirski"  wrote:
>>
>>
>> What we *do* want to do, though, is to prevent the following:
>
>I don't see the point. Why do you bring up this insane scenario that
>nobody
>can possibly care about?
>
>So you actually have any reason to believe somebody does that?
>
>I already asked about that earlier, and the silence was deafening.
>
>Linus

Here is an entire different approach, I don't know if it is sane or not: when 
*mounting* the devpts filesystem, it could automagically create the bins mount 
for ptmx in the parent of its mount point.  Presumably the would be a mount 
option to disable that behavior.

Does anyone see an obvious problem with that?
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

[H. Peter Anvin]

On April 9, 2016 6:27:36 PM PDT, Linus Torvalds  
wrote:
>On Apr 9, 2016 5:45 PM, "Andy Lutomirski"  wrote:
>>
>>
>> What we *do* want to do, though, is to prevent the following:
>
>I don't see the point. Why do you bring up this insane scenario that
>nobody
>can possibly care about?
>
>So you actually have any reason to believe somebody does that?
>
>I already asked about that earlier, and the silence was deafening.
>
>Linus

Here is an entire different approach, I don't know if it is sane or not: when 
*mounting* the devpts filesystem, it could automagically create the bins mount 
for ptmx in the parent of its mount point.  Presumably the would be a mount 
option to disable that behavior.

Does anyone see an obvious problem with that?
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

[Andy Lutomirski]

On Sat, Apr 9, 2016 at 5:16 PM, Linus Torvalds
 wrote:
> On Sat, Apr 9, 2016 at 5:06 PM, H. Peter Anvin  wrote:
>>
>> Fixing the default permissions is trivial, of course.  The intent from the 
>> beginning was to make a ptmx -> pts/ptmx, but user space never did...
>
> That wasn't my point.
>
> Because the permissions have never been usable, I pretty much
> guarantee that no current user space uses /dev/pts/ptmx.
>
> So that node is almost entirely irrelevant. Us fixing the permissions
> at this point isn't going to make it any more relevant, we might as
> well ignore it.
>
> Which all means that the way forward really is to just make /dev/ptmx
> work. It's not going away, and it _is_ fairly easy to fix.
>
> But I don't think the fix should care about permissions - and we might
> as well leave the existing pts/ptmx node with broken permissions.
> Because we've never been actually interested in looking up
> /dev/pts/ptmx - all we actually care about is to look up which devpts
> instance it is.
>
> And that's not about the ptmx node, that's really about the
> mount-point. So the right thing to do - conceptually - is *literally*
> to just say "ok, what is mounted at 'pts'". Note how at no point do we
> want to _open_ anything.
>
> That's why I said that conceptually we could just open /proc/mounts.
> Because *that* is really the operation we care about. We don't care
> about lookup, and we don't care about permissions on the ptmx node.
> Those are completely and utterly irrelevant to what we're actually
> after.
>
> So I think the permission thing is not just extra code with more
> failure points. I think it's conceptually entirely the wrong thing to
> do, and just confuses people into thinking that we're doing something
> that we aren't.

What we *do* want to do, though, is to prevent the following:

Root (or a container manager or whatever) does:

mknod /foo/ptmx c 5 2
chmod 600 /foo/ptmx
chmod 666 /dev/ptmx
mount -t devpts -o newinstance none /foo/pts

Evil user does:

$ unshare -urm
# mount --bind /dev /mnt/foo
# mount --bind /foo/pts /mnt/foo/pts
# open /mnt/foo/ptmx

The issue is that the evil user has the ability to open /mnt/foo/ptmx
(because it's 666), and the relative path 'pts' points to /foo/pts,
which the evil user should *not* be able to access.  IOW, with a naive
implementation, we can match up the ptmx node with the wrong devpts
instance because the evil user unshared their mount namespace and
screwed around.

I don't immediately see how to fix this without playing permission games.

--Andy

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

[Andy Lutomirski]

On Sat, Apr 9, 2016 at 5:16 PM, Linus Torvalds
 wrote:
> On Sat, Apr 9, 2016 at 5:06 PM, H. Peter Anvin  wrote:
>>
>> Fixing the default permissions is trivial, of course.  The intent from the 
>> beginning was to make a ptmx -> pts/ptmx, but user space never did...
>
> That wasn't my point.
>
> Because the permissions have never been usable, I pretty much
> guarantee that no current user space uses /dev/pts/ptmx.
>
> So that node is almost entirely irrelevant. Us fixing the permissions
> at this point isn't going to make it any more relevant, we might as
> well ignore it.
>
> Which all means that the way forward really is to just make /dev/ptmx
> work. It's not going away, and it _is_ fairly easy to fix.
>
> But I don't think the fix should care about permissions - and we might
> as well leave the existing pts/ptmx node with broken permissions.
> Because we've never been actually interested in looking up
> /dev/pts/ptmx - all we actually care about is to look up which devpts
> instance it is.
>
> And that's not about the ptmx node, that's really about the
> mount-point. So the right thing to do - conceptually - is *literally*
> to just say "ok, what is mounted at 'pts'". Note how at no point do we
> want to _open_ anything.
>
> That's why I said that conceptually we could just open /proc/mounts.
> Because *that* is really the operation we care about. We don't care
> about lookup, and we don't care about permissions on the ptmx node.
> Those are completely and utterly irrelevant to what we're actually
> after.
>
> So I think the permission thing is not just extra code with more
> failure points. I think it's conceptually entirely the wrong thing to
> do, and just confuses people into thinking that we're doing something
> that we aren't.

What we *do* want to do, though, is to prevent the following:

Root (or a container manager or whatever) does:

mknod /foo/ptmx c 5 2
chmod 600 /foo/ptmx
chmod 666 /dev/ptmx
mount -t devpts -o newinstance none /foo/pts

Evil user does:

$ unshare -urm
# mount --bind /dev /mnt/foo
# mount --bind /foo/pts /mnt/foo/pts
# open /mnt/foo/ptmx

The issue is that the evil user has the ability to open /mnt/foo/ptmx
(because it's 666), and the relative path 'pts' points to /foo/pts,
which the evil user should *not* be able to access.  IOW, with a naive
implementation, we can match up the ptmx node with the wrong devpts
instance because the evil user unshared their mount namespace and
screwed around.

I don't immediately see how to fix this without playing permission games.

--Andy

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

[Linus Torvalds]

On Sat, Apr 9, 2016 at 5:06 PM, H. Peter Anvin  wrote:
>
> Fixing the default permissions is trivial, of course.  The intent from the 
> beginning was to make a ptmx -> pts/ptmx, but user space never did...

That wasn't my point.

Because the permissions have never been usable, I pretty much
guarantee that no current user space uses /dev/pts/ptmx.

So that node is almost entirely irrelevant. Us fixing the permissions
at this point isn't going to make it any more relevant, we might as
well ignore it.

Which all means that the way forward really is to just make /dev/ptmx
work. It's not going away, and it _is_ fairly easy to fix.

But I don't think the fix should care about permissions - and we might
as well leave the existing pts/ptmx node with broken permissions.
Because we've never been actually interested in looking up
/dev/pts/ptmx - all we actually care about is to look up which devpts
instance it is.

And that's not about the ptmx node, that's really about the
mount-point. So the right thing to do - conceptually - is *literally*
to just say "ok, what is mounted at 'pts'". Note how at no point do we
want to _open_ anything.

That's why I said that conceptually we could just open /proc/mounts.
Because *that* is really the operation we care about. We don't care
about lookup, and we don't care about permissions on the ptmx node.
Those are completely and utterly irrelevant to what we're actually
after.

So I think the permission thing is not just extra code with more
failure points. I think it's conceptually entirely the wrong thing to
do, and just confuses people into thinking that we're doing something
that we aren't.

Linus

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

[Linus Torvalds]

On Sat, Apr 9, 2016 at 5:06 PM, H. Peter Anvin  wrote:
>
> Fixing the default permissions is trivial, of course.  The intent from the 
> beginning was to make a ptmx -> pts/ptmx, but user space never did...

That wasn't my point.

Because the permissions have never been usable, I pretty much
guarantee that no current user space uses /dev/pts/ptmx.

So that node is almost entirely irrelevant. Us fixing the permissions
at this point isn't going to make it any more relevant, we might as
well ignore it.

Which all means that the way forward really is to just make /dev/ptmx
work. It's not going away, and it _is_ fairly easy to fix.

But I don't think the fix should care about permissions - and we might
as well leave the existing pts/ptmx node with broken permissions.
Because we've never been actually interested in looking up
/dev/pts/ptmx - all we actually care about is to look up which devpts
instance it is.

And that's not about the ptmx node, that's really about the
mount-point. So the right thing to do - conceptually - is *literally*
to just say "ok, what is mounted at 'pts'". Note how at no point do we
want to _open_ anything.

That's why I said that conceptually we could just open /proc/mounts.
Because *that* is really the operation we care about. We don't care
about lookup, and we don't care about permissions on the ptmx node.
Those are completely and utterly irrelevant to what we're actually
after.

So I think the permission thing is not just extra code with more
failure points. I think it's conceptually entirely the wrong thing to
do, and just confuses people into thinking that we're doing something
that we aren't.

Linus

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

[H. Peter Anvin]

On April 9, 2016 5:01:27 PM PDT, Linus Torvalds  
wrote:
>On Sat, Apr 9, 2016 at 3:37 PM, H. Peter Anvin  wrote:
>>
>> On the flipside, if we were to allow ourselves to break userspace, at
>this point I would suggest making /dev/pts/ptmx have a different device
>number and make the legacy /dev/ptmx print a warning message, after
>which it can at least eventually be deleted.
>
>You don't need a different device number.
>
>The /dev/pts/ptmx file may look like it's the same node as /dev/ptmx,
>but it is trivial to recognize as the pts one:
>
> if (dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC)
>
>and you're done.
>
>But nobody actually uses /dev/pts/ptmx, because it has never had sane
>permissions.
>
>So the fact is, /dev/ptmx is what people use, and we're not breaking
>userspace.
>
>But when we fix bad semantics (and always just looking up the initial
>pts mount really is crazy semantics) that doesn't mean that we have to
>bend over backwards to not make the changed semantics visible. We
>don't _break_ user space, but we also don't care about some random
>test-program that checks for particular semantics.
>
>And I can pretty much _guarantee_ that nobody has ever done the "let's
>bind-mount a 'ptmx' node in a /dev directory, and then expect that to
>bind to some _other_ pts thing than the one in /dev/pts/".
>
>Except as a test-program, or possibly as a "why the f*ck doesn't this
>work? Oh, I need to use the single-instance thing because the
>multi-instance pts thing is broken. Damn shitty implementation".
>
>Linus

Fixing the default permissions is trivial, of course.  The intent from the 
beginning was to make a ptmx -> pts/ptmx, but user space never did...
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

[H. Peter Anvin]

On April 9, 2016 5:01:27 PM PDT, Linus Torvalds  
wrote:
>On Sat, Apr 9, 2016 at 3:37 PM, H. Peter Anvin  wrote:
>>
>> On the flipside, if we were to allow ourselves to break userspace, at
>this point I would suggest making /dev/pts/ptmx have a different device
>number and make the legacy /dev/ptmx print a warning message, after
>which it can at least eventually be deleted.
>
>You don't need a different device number.
>
>The /dev/pts/ptmx file may look like it's the same node as /dev/ptmx,
>but it is trivial to recognize as the pts one:
>
> if (dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC)
>
>and you're done.
>
>But nobody actually uses /dev/pts/ptmx, because it has never had sane
>permissions.
>
>So the fact is, /dev/ptmx is what people use, and we're not breaking
>userspace.
>
>But when we fix bad semantics (and always just looking up the initial
>pts mount really is crazy semantics) that doesn't mean that we have to
>bend over backwards to not make the changed semantics visible. We
>don't _break_ user space, but we also don't care about some random
>test-program that checks for particular semantics.
>
>And I can pretty much _guarantee_ that nobody has ever done the "let's
>bind-mount a 'ptmx' node in a /dev directory, and then expect that to
>bind to some _other_ pts thing than the one in /dev/pts/".
>
>Except as a test-program, or possibly as a "why the f*ck doesn't this
>work? Oh, I need to use the single-instance thing because the
>multi-instance pts thing is broken. Damn shitty implementation".
>
>Linus

Fixing the default permissions is trivial, of course.  The intent from the 
beginning was to make a ptmx -> pts/ptmx, but user space never did...
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

[Linus Torvalds]

On Sat, Apr 9, 2016 at 3:37 PM, H. Peter Anvin  wrote:
>
> On the flipside, if we were to allow ourselves to break userspace, at this 
> point I would suggest making /dev/pts/ptmx have a different device number and 
> make the legacy /dev/ptmx print a warning message, after which it can at 
> least eventually be deleted.

You don't need a different device number.

The /dev/pts/ptmx file may look like it's the same node as /dev/ptmx,
but it is trivial to recognize as the pts one:

 if (dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC)

and you're done.

But nobody actually uses /dev/pts/ptmx, because it has never had sane
permissions.

So the fact is, /dev/ptmx is what people use, and we're not breaking userspace.

But when we fix bad semantics (and always just looking up the initial
pts mount really is crazy semantics) that doesn't mean that we have to
bend over backwards to not make the changed semantics visible. We
don't _break_ user space, but we also don't care about some random
test-program that checks for particular semantics.

And I can pretty much _guarantee_ that nobody has ever done the "let's
bind-mount a 'ptmx' node in a /dev directory, and then expect that to
bind to some _other_ pts thing than the one in /dev/pts/".

Except as a test-program, or possibly as a "why the f*ck doesn't this
work? Oh, I need to use the single-instance thing because the
multi-instance pts thing is broken. Damn shitty implementation".

Linus

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

[Linus Torvalds]

On Sat, Apr 9, 2016 at 3:37 PM, H. Peter Anvin  wrote:
>
> On the flipside, if we were to allow ourselves to break userspace, at this 
> point I would suggest making /dev/pts/ptmx have a different device number and 
> make the legacy /dev/ptmx print a warning message, after which it can at 
> least eventually be deleted.

You don't need a different device number.

The /dev/pts/ptmx file may look like it's the same node as /dev/ptmx,
but it is trivial to recognize as the pts one:

 if (dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC)

and you're done.

But nobody actually uses /dev/pts/ptmx, because it has never had sane
permissions.

So the fact is, /dev/ptmx is what people use, and we're not breaking userspace.

But when we fix bad semantics (and always just looking up the initial
pts mount really is crazy semantics) that doesn't mean that we have to
bend over backwards to not make the changed semantics visible. We
don't _break_ user space, but we also don't care about some random
test-program that checks for particular semantics.

And I can pretty much _guarantee_ that nobody has ever done the "let's
bind-mount a 'ptmx' node in a /dev directory, and then expect that to
bind to some _other_ pts thing than the one in /dev/pts/".

Except as a test-program, or possibly as a "why the f*ck doesn't this
work? Oh, I need to use the single-instance thing because the
multi-instance pts thing is broken. Damn shitty implementation".

Linus

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

[H. Peter Anvin]

On April 9, 2016 7:45:46 AM PDT, ebied...@xmission.com wrote:
>"H. Peter Anvin"  writes:
>
>> On April 9, 2016 6:09:09 AM PDT, One Thousand Gnomes
> wrote:
>>>
 If anyone has a better idea on how userspace should connect the
>>>master
 pty file descriptor the slave file descriptor, I would be willing
>to
 implement that instead.
>>>
>>>If we are willing to go away from the existing mess of a tty
>interface
>>>inflicted on us by BSD and then mashed up by POSIX then a syscall of
>>>
>>>  int err = ptypair(int fd[2], int perms, int flags);
>>>
>>>[where flags is the O_ ones we usually need to cover (CLOEXEC etc)
>and
>>>maybe even some kind of "private" flag to say don't even expose it
>via
>>>devpts).
>>>
>>>would do remarkably sane things to the majoirty of use cases as it
>>>breaks
>>>the dependence on grantpt and also the historic screwup that pty
>pairs
>>>aren't allocated atomically with both file handles returned as pipe()
>>>does.
>>>
>>>Alan
>>
>> We don't even need to do that if we'd be willing to change the user
>> space interface... if we could rely on the POSIX interface then
>> posix_openpt() could simply open /dev/pts/ptmx and everything would
>> just work.
>
>At a quick skim it does look like userspace uses posix_openpt for the
>most part.  Certainly portable apps that can run on FreeBSD do.
>And just grepping through binaries all of the ones I have checked so
>far
>are calling posix_openpt.
>
>Peter if you or someone could start updating the userspace version of
>posix_openpt to use /dev/pts/ptmx when available over /dev/ptmx in
>parallel to the kernel work to always allow mount of devpts to give
>distinct instances that would be great.
>
>> The trick here is how to make it work in the presence of some
>> extremely bad practices in existing userspace.
>
>Yeah.  I am going to look and see if I can move this controversial bit
>to a separate patch so we can discuss it more conviniently.
>
>Eric

On the flipside, if we were to allow ourselves to break userspace, at this 
point I would suggest making /dev/pts/ptmx have a different device number and 
make the legacy /dev/ptmx print a warning message, after which it can at least 
eventually be deleted.

This might not be a bad idea anyway.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

[H. Peter Anvin]

On April 9, 2016 7:45:46 AM PDT, ebied...@xmission.com wrote:
>"H. Peter Anvin"  writes:
>
>> On April 9, 2016 6:09:09 AM PDT, One Thousand Gnomes
> wrote:
>>>
 If anyone has a better idea on how userspace should connect the
>>>master
 pty file descriptor the slave file descriptor, I would be willing
>to
 implement that instead.
>>>
>>>If we are willing to go away from the existing mess of a tty
>interface
>>>inflicted on us by BSD and then mashed up by POSIX then a syscall of
>>>
>>>  int err = ptypair(int fd[2], int perms, int flags);
>>>
>>>[where flags is the O_ ones we usually need to cover (CLOEXEC etc)
>and
>>>maybe even some kind of "private" flag to say don't even expose it
>via
>>>devpts).
>>>
>>>would do remarkably sane things to the majoirty of use cases as it
>>>breaks
>>>the dependence on grantpt and also the historic screwup that pty
>pairs
>>>aren't allocated atomically with both file handles returned as pipe()
>>>does.
>>>
>>>Alan
>>
>> We don't even need to do that if we'd be willing to change the user
>> space interface... if we could rely on the POSIX interface then
>> posix_openpt() could simply open /dev/pts/ptmx and everything would
>> just work.
>
>At a quick skim it does look like userspace uses posix_openpt for the
>most part.  Certainly portable apps that can run on FreeBSD do.
>And just grepping through binaries all of the ones I have checked so
>far
>are calling posix_openpt.
>
>Peter if you or someone could start updating the userspace version of
>posix_openpt to use /dev/pts/ptmx when available over /dev/ptmx in
>parallel to the kernel work to always allow mount of devpts to give
>distinct instances that would be great.
>
>> The trick here is how to make it work in the presence of some
>> extremely bad practices in existing userspace.
>
>Yeah.  I am going to look and see if I can move this controversial bit
>to a separate patch so we can discuss it more conviniently.
>
>Eric

On the flipside, if we were to allow ourselves to break userspace, at this 
point I would suggest making /dev/pts/ptmx have a different device number and 
make the legacy /dev/ptmx print a warning message, after which it can at least 
eventually be deleted.

This might not be a bad idea anyway.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

[Eric W. Biederman]

"H. Peter Anvin"  writes:

> On April 9, 2016 6:09:09 AM PDT, One Thousand Gnomes 
>  wrote:
>>
>>> If anyone has a better idea on how userspace should connect the
>>master
>>> pty file descriptor the slave file descriptor, I would be willing to
>>> implement that instead.
>>
>>If we are willing to go away from the existing mess of a tty interface
>>inflicted on us by BSD and then mashed up by POSIX then a syscall of
>>
>>  int err = ptypair(int fd[2], int perms, int flags);
>>
>>[where flags is the O_ ones we usually need to cover (CLOEXEC etc) and
>>maybe even some kind of "private" flag to say don't even expose it via
>>devpts).
>>
>>would do remarkably sane things to the majoirty of use cases as it
>>breaks
>>the dependence on grantpt and also the historic screwup that pty pairs
>>aren't allocated atomically with both file handles returned as pipe()
>>does.
>>
>>Alan
>
> We don't even need to do that if we'd be willing to change the user
> space interface... if we could rely on the POSIX interface then
> posix_openpt() could simply open /dev/pts/ptmx and everything would
> just work.

At a quick skim it does look like userspace uses posix_openpt for the
most part.  Certainly portable apps that can run on FreeBSD do.
And just grepping through binaries all of the ones I have checked so far
are calling posix_openpt.

Peter if you or someone could start updating the userspace version of
posix_openpt to use /dev/pts/ptmx when available over /dev/ptmx in
parallel to the kernel work to always allow mount of devpts to give
distinct instances that would be great.

> The trick here is how to make it work in the presence of some
> extremely bad practices in existing userspace.

Yeah.  I am going to look and see if I can move this controversial bit
to a separate patch so we can discuss it more conviniently.

Eric

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

[Eric W. Biederman]

"H. Peter Anvin"  writes:

> On April 9, 2016 6:09:09 AM PDT, One Thousand Gnomes 
>  wrote:
>>
>>> If anyone has a better idea on how userspace should connect the
>>master
>>> pty file descriptor the slave file descriptor, I would be willing to
>>> implement that instead.
>>
>>If we are willing to go away from the existing mess of a tty interface
>>inflicted on us by BSD and then mashed up by POSIX then a syscall of
>>
>>  int err = ptypair(int fd[2], int perms, int flags);
>>
>>[where flags is the O_ ones we usually need to cover (CLOEXEC etc) and
>>maybe even some kind of "private" flag to say don't even expose it via
>>devpts).
>>
>>would do remarkably sane things to the majoirty of use cases as it
>>breaks
>>the dependence on grantpt and also the historic screwup that pty pairs
>>aren't allocated atomically with both file handles returned as pipe()
>>does.
>>
>>Alan
>
> We don't even need to do that if we'd be willing to change the user
> space interface... if we could rely on the POSIX interface then
> posix_openpt() could simply open /dev/pts/ptmx and everything would
> just work.

At a quick skim it does look like userspace uses posix_openpt for the
most part.  Certainly portable apps that can run on FreeBSD do.
And just grepping through binaries all of the ones I have checked so far
are calling posix_openpt.

Peter if you or someone could start updating the userspace version of
posix_openpt to use /dev/pts/ptmx when available over /dev/ptmx in
parallel to the kernel work to always allow mount of devpts to give
distinct instances that would be great.

> The trick here is how to make it work in the presence of some
> extremely bad practices in existing userspace.

Yeah.  I am going to look and see if I can move this controversial bit
to a separate patch so we can discuss it more conviniently.

Eric

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

[H. Peter Anvin]

On April 9, 2016 6:09:09 AM PDT, One Thousand Gnomes 
 wrote:
>
>> If anyone has a better idea on how userspace should connect the
>master
>> pty file descriptor the slave file descriptor, I would be willing to
>> implement that instead.
>
>If we are willing to go away from the existing mess of a tty interface
>inflicted on us by BSD and then mashed up by POSIX then a syscall of
>
>  int err = ptypair(int fd[2], int perms, int flags);
>
>[where flags is the O_ ones we usually need to cover (CLOEXEC etc) and
>maybe even some kind of "private" flag to say don't even expose it via
>devpts).
>
>would do remarkably sane things to the majoirty of use cases as it
>breaks
>the dependence on grantpt and also the historic screwup that pty pairs
>aren't allocated atomically with both file handles returned as pipe()
>does.
>
>Alan

We don't even need to do that if we'd be willing to change the user space 
interface... if we could rely on the POSIX interface then posix_openpt() could 
simply open /dev/pts/ptmx and everything would just work.

The trick here is how to make it work in the presence of some extremely bad 
practices in existing userspace.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

[H. Peter Anvin]

On April 9, 2016 6:09:09 AM PDT, One Thousand Gnomes 
 wrote:
>
>> If anyone has a better idea on how userspace should connect the
>master
>> pty file descriptor the slave file descriptor, I would be willing to
>> implement that instead.
>
>If we are willing to go away from the existing mess of a tty interface
>inflicted on us by BSD and then mashed up by POSIX then a syscall of
>
>  int err = ptypair(int fd[2], int perms, int flags);
>
>[where flags is the O_ ones we usually need to cover (CLOEXEC etc) and
>maybe even some kind of "private" flag to say don't even expose it via
>devpts).
>
>would do remarkably sane things to the majoirty of use cases as it
>breaks
>the dependence on grantpt and also the historic screwup that pty pairs
>aren't allocated atomically with both file handles returned as pipe()
>does.
>
>Alan

We don't even need to do that if we'd be willing to change the user space 
interface... if we could rely on the POSIX interface then posix_openpt() could 
simply open /dev/pts/ptmx and everything would just work.

The trick here is how to make it work in the presence of some extremely bad 
practices in existing userspace.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

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

[One Thousand Gnomes]

> If anyone has a better idea on how userspace should connect the master
> pty file descriptor the slave file descriptor, I would be willing to
> implement that instead.

If we are willing to go away from the existing mess of a tty interface
inflicted on us by BSD and then mashed up by POSIX then a syscall of

  int err = ptypair(int fd[2], int perms, int flags);

[where flags is the O_ ones we usually need to cover (CLOEXEC etc) and
maybe even some kind of "private" flag to say don't even expose it via
devpts).

would do remarkably sane things to the majoirty of use cases as it breaks
the dependence on grantpt and also the historic screwup that pty pairs
aren't allocated atomically with both file handles returned as pipe()
does.

Alan

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

[One Thousand Gnomes]

> If anyone has a better idea on how userspace should connect the master
> pty file descriptor the slave file descriptor, I would be willing to
> implement that instead.

If we are willing to go away from the existing mess of a tty interface
inflicted on us by BSD and then mashed up by POSIX then a syscall of

  int err = ptypair(int fd[2], int perms, int flags);

[where flags is the O_ ones we usually need to cover (CLOEXEC etc) and
maybe even some kind of "private" flag to say don't even expose it via
devpts).

would do remarkably sane things to the majoirty of use cases as it breaks
the dependence on grantpt and also the historic screwup that pty pairs
aren't allocated atomically with both file handles returned as pipe()
does.

Alan

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

[Eric W. Biederman]

Linus Torvalds  writes:

> But more fundamentally I still don't actually understand why you even
> really care.

At this point I care because there is a failure of communication.
Until this email no one has ever said:  "Ok that actually could happen
but we don't actually care."

Right now I am a bit paranoid because I have seen a few too many cases
where some little detail was glossed over and someone clever turned it
into a great big CVE they could drive a truck through.  So I am once
bitten twice shy and all of that.

> We get the wrong pts case *today*. We'd get a different wrong pts
> namespace when somebody tries to do odd things. Why would we care? It
> would be a _better_ guess.
>
> I don't see the security issue. If you do tricks to get pty's in
> another group, what's the problem? You have to do it consciously, and
> I don't see what the downside is. You get what you ask for, and I
> don't see a new attack surface.
>
> The whole "somebody used chmod on /dev/pts/" argument sounds bogus.
> That's an insane thing to do. If you want a private namespace, you
> make *all* of /dev private, you don't go "oh, I'll just make the pts
> subdirectory private".

Oh I pretty much agree it is an insane thing to do.  At the same time I
know that people can make a lot of little sane decisions that can lead
to an insane situation, so just because it is insane I can't rule
it out automatically.

The actual sane thing to do, and what I think most of userspace does
at this point is to create it's own mount namespace so nothing is
visible to outsiders.

> In other words, your whole scenario sounds totally made up to begin
> with. And even if it happens, I don't see what would be so disastrous
> about it.

In general I agree.  The scenario is made up.  I would be surprised if
it happens.

> I mean, right now, /dev/ptmx is world read-write in the root container
> and everybody gets access to the same underlying set of ptys. And
> that's not some horrible security issue. It's how things are
> *supposed* to work.

I agree.

> So I really don't see the argument. You guys are just making shit up.

I don't see why we have the linux extension of supporting anything
except mode 0666 on /dev/ptmx or /dev/pts/ptmx.  This is really about
not breaking that linux extension by overlooking some little detail.

On the attack analysis front the worst thing I can see happening is a
denial of service attack.  I see two possible denial of service attacks.
One possible attack creates a pty and prevents devpts from being
unmounted.  Another possible attack creates all possible ptys on a
devpts instance, and prevents legitimate tty creations from happening.

At the end of the day as you say it would be a pretty crazy person who
isolated a mount of devpts with just the permissions of /dev/pts/ptmx.
So if we don't want to care knowing those stupid attacks above are
possible I am happy not to care.  They don't look all that serious to
me.

Eric

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

[Eric W. Biederman]

Linus Torvalds  writes:

> But more fundamentally I still don't actually understand why you even
> really care.

At this point I care because there is a failure of communication.
Until this email no one has ever said:  "Ok that actually could happen
but we don't actually care."

Right now I am a bit paranoid because I have seen a few too many cases
where some little detail was glossed over and someone clever turned it
into a great big CVE they could drive a truck through.  So I am once
bitten twice shy and all of that.

> We get the wrong pts case *today*. We'd get a different wrong pts
> namespace when somebody tries to do odd things. Why would we care? It
> would be a _better_ guess.
>
> I don't see the security issue. If you do tricks to get pty's in
> another group, what's the problem? You have to do it consciously, and
> I don't see what the downside is. You get what you ask for, and I
> don't see a new attack surface.
>
> The whole "somebody used chmod on /dev/pts/" argument sounds bogus.
> That's an insane thing to do. If you want a private namespace, you
> make *all* of /dev private, you don't go "oh, I'll just make the pts
> subdirectory private".

Oh I pretty much agree it is an insane thing to do.  At the same time I
know that people can make a lot of little sane decisions that can lead
to an insane situation, so just because it is insane I can't rule
it out automatically.

The actual sane thing to do, and what I think most of userspace does
at this point is to create it's own mount namespace so nothing is
visible to outsiders.

> In other words, your whole scenario sounds totally made up to begin
> with. And even if it happens, I don't see what would be so disastrous
> about it.

In general I agree.  The scenario is made up.  I would be surprised if
it happens.

> I mean, right now, /dev/ptmx is world read-write in the root container
> and everybody gets access to the same underlying set of ptys. And
> that's not some horrible security issue. It's how things are
> *supposed* to work.

I agree.

> So I really don't see the argument. You guys are just making shit up.

I don't see why we have the linux extension of supporting anything
except mode 0666 on /dev/ptmx or /dev/pts/ptmx.  This is really about
not breaking that linux extension by overlooking some little detail.

On the attack analysis front the worst thing I can see happening is a
denial of service attack.  I see two possible denial of service attacks.
One possible attack creates a pty and prevents devpts from being
unmounted.  Another possible attack creates all possible ptys on a
devpts instance, and prevents legitimate tty creations from happening.

At the end of the day as you say it would be a pretty crazy person who
isolated a mount of devpts with just the permissions of /dev/pts/ptmx.
So if we don't want to care knowing those stupid attacks above are
possible I am happy not to care.  They don't look all that serious to
me.

Eric

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

[Andy Lutomirski]

On Fri, Apr 8, 2016 at 2:29 PM, Eric W. Biederman  wrote:
> Andy Lutomirski  writes:
>
>> On Apr 8, 2016 12:05 PM, "Linus Torvalds"  
>> wrote:
>>>
>>> On Fri, Apr 8, 2016 at 11:51 AM, Eric W. Biederman
>>>  wrote:
>>> >
>>> > Given that concern under the rule we don't break userspace we have to
>>> > check the permissions of /dev/pts/ptmx when we are creating a new pty,
>>> > on a instance of devpts that was created with newinstance.
>>>
>>> The rule is that we don't break existing installations.
>>>
>>> If somebody has root and installs a "ptmx" node in an existing mount
>>> space next to a pts subdirectory, that's not a security issue, nor is
>>> it going to break any existing installation.
>>
>> What Eric's saying is that you don't have to be root for this.
>>
>> But Eric, I think there might be a better mitigation.  For a ptmx
>> chardev, just fail the open if the chardev's vfsmount or the devpts's
>> vfsmount doesn't belong to the same userns as the devpts's superblock.
>> After all, setting this attack up requires the caps on one of the
>> vfsmounts, and if you have those caps you could attack your own devpts
>> instance quite easily.  Would that work?
>
> I don't think so.  For one it depends on getting s_user_ns which should
> happen but is not there yet.  For another the way you describe
> it you would break the case of
>
> unshare(CLONE_NEWUSER);
> unshare(CLONE_NEWNS);
> open("/dev/ptmx");
>
> Which is actually more likely to break userspace than anything else we
> have considered.  I know people actually do that.
>

Hmm, you're right.  Never mind.

--Andy

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

[Andy Lutomirski]

On Fri, Apr 8, 2016 at 2:29 PM, Eric W. Biederman  wrote:
> Andy Lutomirski  writes:
>
>> On Apr 8, 2016 12:05 PM, "Linus Torvalds"  
>> wrote:
>>>
>>> On Fri, Apr 8, 2016 at 11:51 AM, Eric W. Biederman
>>>  wrote:
>>> >
>>> > Given that concern under the rule we don't break userspace we have to
>>> > check the permissions of /dev/pts/ptmx when we are creating a new pty,
>>> > on a instance of devpts that was created with newinstance.
>>>
>>> The rule is that we don't break existing installations.
>>>
>>> If somebody has root and installs a "ptmx" node in an existing mount
>>> space next to a pts subdirectory, that's not a security issue, nor is
>>> it going to break any existing installation.
>>
>> What Eric's saying is that you don't have to be root for this.
>>
>> But Eric, I think there might be a better mitigation.  For a ptmx
>> chardev, just fail the open if the chardev's vfsmount or the devpts's
>> vfsmount doesn't belong to the same userns as the devpts's superblock.
>> After all, setting this attack up requires the caps on one of the
>> vfsmounts, and if you have those caps you could attack your own devpts
>> instance quite easily.  Would that work?
>
> I don't think so.  For one it depends on getting s_user_ns which should
> happen but is not there yet.  For another the way you describe
> it you would break the case of
>
> unshare(CLONE_NEWUSER);
> unshare(CLONE_NEWNS);
> open("/dev/ptmx");
>
> Which is actually more likely to break userspace than anything else we
> have considered.  I know people actually do that.
>

Hmm, you're right.  Never mind.

--Andy

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

[Linus Torvalds]

On Fri, Apr 8, 2016 at 2:29 PM, Eric W. Biederman  wrote:
>
> I don't think so.  For one it depends on getting s_user_ns which should
> happen but is not there yet.  For another the way you describe
> it you would break the case of
>
> unshare(CLONE_NEWUSER);
> unshare(CLONE_NEWNS);
> open("/dev/ptmx");
>
> Which is actually more likely to break userspace than anything else we
> have considered.  I know people actually do that.

.. but you could just check that the ptmx node is actually the same
superblock that the pts directory is mounted on. If it's a bind mount,
that wouldn't be true.

But more fundamentally I still don't actually understand why you even
really care.

We get the wrong pts case *today*. We'd get a different wrong pts
namespace when somebody tries to do odd things. Why would we care? It
would be a _better_ guess.

I don't see the security issue. If you do tricks to get pty's in
another group, what's the problem? You have to do it consciously, and
I don't see what the downside is. You get what you ask for, and I
don't see a new attack surface.

The whole "somebody used chmod on /dev/pts/" argument sounds bogus.
That's an insane thing to do. If you want a private namespace, you
make *all* of /dev private, you don't go "oh, I'll just make the pts
subdirectory private".

In other words, your whole scenario sounds totally made up to begin
with. And even if it happens, I don't see what would be so disastrous
about it.

I mean, right now, /dev/ptmx is world read-write in the root container
and everybody gets access to the same underlying set of ptys. And
that's not some horrible security issue. It's how things are
*supposed* to work.

So I really don't see the argument. You guys are just making shit up.

Linus

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

[Linus Torvalds]

On Fri, Apr 8, 2016 at 2:29 PM, Eric W. Biederman  wrote:
>
> I don't think so.  For one it depends on getting s_user_ns which should
> happen but is not there yet.  For another the way you describe
> it you would break the case of
>
> unshare(CLONE_NEWUSER);
> unshare(CLONE_NEWNS);
> open("/dev/ptmx");
>
> Which is actually more likely to break userspace than anything else we
> have considered.  I know people actually do that.

.. but you could just check that the ptmx node is actually the same
superblock that the pts directory is mounted on. If it's a bind mount,
that wouldn't be true.

But more fundamentally I still don't actually understand why you even
really care.

We get the wrong pts case *today*. We'd get a different wrong pts
namespace when somebody tries to do odd things. Why would we care? It
would be a _better_ guess.

I don't see the security issue. If you do tricks to get pty's in
another group, what's the problem? You have to do it consciously, and
I don't see what the downside is. You get what you ask for, and I
don't see a new attack surface.

The whole "somebody used chmod on /dev/pts/" argument sounds bogus.
That's an insane thing to do. If you want a private namespace, you
make *all* of /dev private, you don't go "oh, I'll just make the pts
subdirectory private".

In other words, your whole scenario sounds totally made up to begin
with. And even if it happens, I don't see what would be so disastrous
about it.

I mean, right now, /dev/ptmx is world read-write in the root container
and everybody gets access to the same underlying set of ptys. And
that's not some horrible security issue. It's how things are
*supposed* to work.

So I really don't see the argument. You guys are just making shit up.

Linus

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

[Eric W. Biederman]

Andy Lutomirski  writes:

> On Apr 8, 2016 12:05 PM, "Linus Torvalds"  
> wrote:
>>
>> On Fri, Apr 8, 2016 at 11:51 AM, Eric W. Biederman
>>  wrote:
>> >
>> > Given that concern under the rule we don't break userspace we have to
>> > check the permissions of /dev/pts/ptmx when we are creating a new pty,
>> > on a instance of devpts that was created with newinstance.
>>
>> The rule is that we don't break existing installations.
>>
>> If somebody has root and installs a "ptmx" node in an existing mount
>> space next to a pts subdirectory, that's not a security issue, nor is
>> it going to break any existing installation.
>
> What Eric's saying is that you don't have to be root for this.
>
> But Eric, I think there might be a better mitigation.  For a ptmx
> chardev, just fail the open if the chardev's vfsmount or the devpts's
> vfsmount doesn't belong to the same userns as the devpts's superblock.
> After all, setting this attack up requires the caps on one of the
> vfsmounts, and if you have those caps you could attack your own devpts
> instance quite easily.  Would that work?

I don't think so.  For one it depends on getting s_user_ns which should
happen but is not there yet.  For another the way you describe
it you would break the case of

unshare(CLONE_NEWUSER);
unshare(CLONE_NEWNS);
open("/dev/ptmx");

Which is actually more likely to break userspace than anything else we
have considered.  I know people actually do that.


Also using any property from a mount namespace or a vfs mount is usually
an error, as it is an inconsistent model.

Plus I don't think what you are suggesting would make anything simpler
or easier to reason about.  It only costs me about 3 lines of code to
perform the permission checks.  The complaint is that they exist at all.

Eric

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

[Eric W. Biederman]

Andy Lutomirski  writes:

> On Apr 8, 2016 12:05 PM, "Linus Torvalds"  
> wrote:
>>
>> On Fri, Apr 8, 2016 at 11:51 AM, Eric W. Biederman
>>  wrote:
>> >
>> > Given that concern under the rule we don't break userspace we have to
>> > check the permissions of /dev/pts/ptmx when we are creating a new pty,
>> > on a instance of devpts that was created with newinstance.
>>
>> The rule is that we don't break existing installations.
>>
>> If somebody has root and installs a "ptmx" node in an existing mount
>> space next to a pts subdirectory, that's not a security issue, nor is
>> it going to break any existing installation.
>
> What Eric's saying is that you don't have to be root for this.
>
> But Eric, I think there might be a better mitigation.  For a ptmx
> chardev, just fail the open if the chardev's vfsmount or the devpts's
> vfsmount doesn't belong to the same userns as the devpts's superblock.
> After all, setting this attack up requires the caps on one of the
> vfsmounts, and if you have those caps you could attack your own devpts
> instance quite easily.  Would that work?

I don't think so.  For one it depends on getting s_user_ns which should
happen but is not there yet.  For another the way you describe
it you would break the case of

unshare(CLONE_NEWUSER);
unshare(CLONE_NEWNS);
open("/dev/ptmx");

Which is actually more likely to break userspace than anything else we
have considered.  I know people actually do that.


Also using any property from a mount namespace or a vfs mount is usually
an error, as it is an inconsistent model.

Plus I don't think what you are suggesting would make anything simpler
or easier to reason about.  It only costs me about 3 lines of code to
perform the permission checks.  The complaint is that they exist at all.

Eric

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

[Andy Lutomirski]

On Apr 8, 2016 12:05 PM, "Linus Torvalds"  wrote:
>
> On Fri, Apr 8, 2016 at 11:51 AM, Eric W. Biederman
>  wrote:
> >
> > Given that concern under the rule we don't break userspace we have to
> > check the permissions of /dev/pts/ptmx when we are creating a new pty,
> > on a instance of devpts that was created with newinstance.
>
> The rule is that we don't break existing installations.
>
> If somebody has root and installs a "ptmx" node in an existing mount
> space next to a pts subdirectory, that's not a security issue, nor is
> it going to break any existing installation.

What Eric's saying is that you don't have to be root for this.

But Eric, I think there might be a better mitigation.  For a ptmx
chardev, just fail the open if the chardev's vfsmount or the devpts's
vfsmount doesn't belong to the same userns as the devpts's superblock.
After all, setting this attack up requires the caps on one of the
vfsmounts, and if you have those caps you could attack your own devpts
instance quite easily.  Would that work?

>
> The whole point of the patch is that yes, we change semantics. A
> change of semantics means that people will see situations where the
> behavior is different. But that's not "breaking user space", that's
> just "ok, you can see a difference".
>
> Linus

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

[Andy Lutomirski]

On Apr 8, 2016 12:05 PM, "Linus Torvalds"  wrote:
>
> On Fri, Apr 8, 2016 at 11:51 AM, Eric W. Biederman
>  wrote:
> >
> > Given that concern under the rule we don't break userspace we have to
> > check the permissions of /dev/pts/ptmx when we are creating a new pty,
> > on a instance of devpts that was created with newinstance.
>
> The rule is that we don't break existing installations.
>
> If somebody has root and installs a "ptmx" node in an existing mount
> space next to a pts subdirectory, that's not a security issue, nor is
> it going to break any existing installation.

What Eric's saying is that you don't have to be root for this.

But Eric, I think there might be a better mitigation.  For a ptmx
chardev, just fail the open if the chardev's vfsmount or the devpts's
vfsmount doesn't belong to the same userns as the devpts's superblock.
After all, setting this attack up requires the caps on one of the
vfsmounts, and if you have those caps you could attack your own devpts
instance quite easily.  Would that work?

>
> The whole point of the patch is that yes, we change semantics. A
> change of semantics means that people will see situations where the
> behavior is different. But that's not "breaking user space", that's
> just "ok, you can see a difference".
>
> Linus

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

[Eric W. Biederman]

Linus Torvalds  writes:

> On Fri, Apr 8, 2016 at 11:51 AM, Eric W. Biederman
>  wrote:
>>
>> Given that concern under the rule we don't break userspace we have to
>> check the permissions of /dev/pts/ptmx when we are creating a new pty,
>> on a instance of devpts that was created with newinstance.
>
> The rule is that we don't break existing installations.
>
> If somebody has root and installs a "ptmx" node in an existing mount
> space next to a pts subdirectory, that's not a security issue, nor is
> it going to break any existing installation.

Anyone can do that with "mount --bind".  All it takes is root in a user
namespace.  I can get root in a user namespace as no one special.

So someone may have set such a thing up, and it may now be possible
to defeat such a regime as anyone.

In practice I suspect all such cases are handled by actually hiding the
mount of devpts in another mount namespace.

> The whole point of the patch is that yes, we change semantics. A
> change of semantics means that people will see situations where the
> behavior is different. But that's not "breaking user space", that's
> just "ok, you can see a difference".

If we don't want to care about this case, and if someone complains about
a security regression readd my permission checks I am fine with that.

But I don't want to let a possibility of breaking someone (that I don't
know how to test for, and would be silent breakage) slip through.

Eric

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

[Eric W. Biederman]

Linus Torvalds  writes:

> On Fri, Apr 8, 2016 at 11:51 AM, Eric W. Biederman
>  wrote:
>>
>> Given that concern under the rule we don't break userspace we have to
>> check the permissions of /dev/pts/ptmx when we are creating a new pty,
>> on a instance of devpts that was created with newinstance.
>
> The rule is that we don't break existing installations.
>
> If somebody has root and installs a "ptmx" node in an existing mount
> space next to a pts subdirectory, that's not a security issue, nor is
> it going to break any existing installation.

Anyone can do that with "mount --bind".  All it takes is root in a user
namespace.  I can get root in a user namespace as no one special.

So someone may have set such a thing up, and it may now be possible
to defeat such a regime as anyone.

In practice I suspect all such cases are handled by actually hiding the
mount of devpts in another mount namespace.

> The whole point of the patch is that yes, we change semantics. A
> change of semantics means that people will see situations where the
> behavior is different. But that's not "breaking user space", that's
> just "ok, you can see a difference".

If we don't want to care about this case, and if someone complains about
a security regression readd my permission checks I am fine with that.

But I don't want to let a possibility of breaking someone (that I don't
know how to test for, and would be silent breakage) slip through.

Eric

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

[Linus Torvalds]

On Fri, Apr 8, 2016 at 11:51 AM, Eric W. Biederman
 wrote:
>
> Given that concern under the rule we don't break userspace we have to
> check the permissions of /dev/pts/ptmx when we are creating a new pty,
> on a instance of devpts that was created with newinstance.

The rule is that we don't break existing installations.

If somebody has root and installs a "ptmx" node in an existing mount
space next to a pts subdirectory, that's not a security issue, nor is
it going to break any existing installation.

The whole point of the patch is that yes, we change semantics. A
change of semantics means that people will see situations where the
behavior is different. But that's not "breaking user space", that's
just "ok, you can see a difference".

Linus

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

[Linus Torvalds]

On Fri, Apr 8, 2016 at 11:51 AM, Eric W. Biederman
 wrote:
>
> Given that concern under the rule we don't break userspace we have to
> check the permissions of /dev/pts/ptmx when we are creating a new pty,
> on a instance of devpts that was created with newinstance.

The rule is that we don't break existing installations.

If somebody has root and installs a "ptmx" node in an existing mount
space next to a pts subdirectory, that's not a security issue, nor is
it going to break any existing installation.

The whole point of the patch is that yes, we change semantics. A
change of semantics means that people will see situations where the
behavior is different. But that's not "breaking user space", that's
just "ok, you can see a difference".

Linus

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

[Eric W. Biederman]

Al Viro  writes:

> On Tue, Apr 05, 2016 at 03:54:25AM +0100, Al Viro wrote:
>
>> That, I take it, is a lookup for .. and buggering off if it fails *or* if
>> we had been in caller's root or something that overmount it?  Not that the
>> latter had been possible - root is a directory and can be overmounted only
>> by another such, and we are called from ->open() of a device node.
>> 
>> > +  /* Remember the result of this permission check for later */
>> > +  ret = inode_permission(path.dentry->d_inode, MAY_EXEC);
>> > +  if (path_pts())
>> > +  goto fail;
>> 
>> Egads, man - you've just introduced a special function for looking up
>> something named "pts" in a given directory!
>> 
>> The reason not to use kern_path() would be what, the fact that it doesn't
>> allow starting at given location?  So let's make a variant that would - and
>> rather than bothering with RCU, just go for something like (completely
>> untested)
>
> Ah...  Right, that would demand exec permissions on the starting point.
> Still, this is incredibly ugly ;-/  I'll try to come up with something
> more tolerable, but this "path_pts" thing is too ugly to live.
> Seriously.

Given that I can think of no other reason than this special case to ever
want to use this code.  I figured having something incredibily special
case and obviously so was the way to go.  Then at least no one would
mistake it for a general purpose facility.

Eric

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

[Eric W. Biederman]

Al Viro  writes:

> On Tue, Apr 05, 2016 at 03:54:25AM +0100, Al Viro wrote:
>
>> That, I take it, is a lookup for .. and buggering off if it fails *or* if
>> we had been in caller's root or something that overmount it?  Not that the
>> latter had been possible - root is a directory and can be overmounted only
>> by another such, and we are called from ->open() of a device node.
>> 
>> > +  /* Remember the result of this permission check for later */
>> > +  ret = inode_permission(path.dentry->d_inode, MAY_EXEC);
>> > +  if (path_pts())
>> > +  goto fail;
>> 
>> Egads, man - you've just introduced a special function for looking up
>> something named "pts" in a given directory!
>> 
>> The reason not to use kern_path() would be what, the fact that it doesn't
>> allow starting at given location?  So let's make a variant that would - and
>> rather than bothering with RCU, just go for something like (completely
>> untested)
>
> Ah...  Right, that would demand exec permissions on the starting point.
> Still, this is incredibly ugly ;-/  I'll try to come up with something
> more tolerable, but this "path_pts" thing is too ugly to live.
> Seriously.

Given that I can think of no other reason than this special case to ever
want to use this code.  I figured having something incredibily special
case and obviously so was the way to go.  Then at least no one would
mistake it for a general purpose facility.

Eric

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

[Eric W. Biederman]

Linus Torvalds  writes:

> 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.

No. You are missing my concern.

My concern is that I suspect someone somewhere has created a chroot
environment.  That chroot environment has devpts mounted with
"-o newinstance" and has set the permissions of /dev/pts/ptmx such
that only users in that container can create ptys on that instance
of devpts.

Being a mischevious user outside the container I can create a new user
namespace and a new mount namespace and bind mount our new and improved
version of /dev/ptmx right next to the chroot's /dev/pts mount.

Then because the permissions on /dev/ptmx are different than on the
chroots /dev/pts/ptmx I can create ptys that I could not have before
hand.  Bypassing the existing permissions.

Given that concern under the rule we don't break userspace we have to
check the permissions of /dev/pts/ptmx when we are creating a new pty,
on a instance of devpts that was created with newinstance.

Short of saying we simply don't care about such users I don't see a way
we can allow bypassing the existing permission check.



Now I do think we can remove the permission check altogether.  At this
point POSIX does not even require the existince of any files or device
nodes, and FreeBSD proves out that users of ptys don't care by
implementing a dedicated system call to create ptys that does contain
any permission checks.  So only the small set of linux specific
chroot/container creating applications might care.  As the permissions
were not in any way the focus of this patchset I choose not to tackle
a possible user visible change like this.

>
> 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?"

I wish I could in conscience do that.  But unless we decide that
permission are irrelevant we are adding a permission bypass for an
existing operation.  Typically that is called a security bug.  I am not
comfortable doing that unless we simply decide we don't care.

If we decide we don't care I will add a patch at the front of the
patchset that implements don't care before all of the rest of this.

> 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)"/

Actually it is not pointless.  There is a second issue in all of this.
Right now it is possible to confuse the pt_chown setuid root binary
about which instance of devpts it should be calling chmod on.  I am not
certain it even ensures it is calling chown on a devpts entry.  It is
just hard coded paths today.

Right now userspace does something like:

masterfd = posix_openpt(O_RDWR);
grantpt(masterfd);
char *slave_name = ptsname(masterfd);
slavefd = open(slave_name);

Furthemore invoked by grantpt execs the pt_chown binary which does:

int pty_number;
ioctl(masterfd, TIOCGPTN, _number)
sprintf(slave_name, "/dev/pty/%u", pty_number);
chown(slave_name, some_uid, some_gid);

It would be very nice if we could have a way to close the races
in this mess and allow a program like pt_chown to actually only affect
the pty it cares about.  There is only one way I know to implement this
in a backwards compatible way and that is to have
readlink("/proc/self/fd/${masterfd}"), stat("/proc/self/fd/${masterfd}",
and open("/proc/self/fd/${masterfd}") talk about "/dev/pts/ptmx"
for a non-system instance of devpts

That will at least allow ptsname to find the proper instance of devpts.

I suppose it becomes hameless if grantpt stops calling a setuid root
exectuable if the connection between the master and the slave pty
gets confused and pt_chown, does not exist anymore.  But it feels
wrong to allow userspace no way to ask the question which mounted
instance of devpts does this masterfd belong to.  Especially
as readlink("/proc/self/fd/${masterfd}") is the natural way to ask that.

If anyone has a better idea on how userspace should connect the master
pty file descriptor the slave file descriptor, I would be willing to
implement that instead.

> So get rid of all the pathname games. Just save the superblock pointer
> in file->f_private or somewhere like 

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

[Eric W. Biederman]

Linus Torvalds  writes:

> 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.

No. You are missing my concern.

My concern is that I suspect someone somewhere has created a chroot
environment.  That chroot environment has devpts mounted with
"-o newinstance" and has set the permissions of /dev/pts/ptmx such
that only users in that container can create ptys on that instance
of devpts.

Being a mischevious user outside the container I can create a new user
namespace and a new mount namespace and bind mount our new and improved
version of /dev/ptmx right next to the chroot's /dev/pts mount.

Then because the permissions on /dev/ptmx are different than on the
chroots /dev/pts/ptmx I can create ptys that I could not have before
hand.  Bypassing the existing permissions.

Given that concern under the rule we don't break userspace we have to
check the permissions of /dev/pts/ptmx when we are creating a new pty,
on a instance of devpts that was created with newinstance.

Short of saying we simply don't care about such users I don't see a way
we can allow bypassing the existing permission check.



Now I do think we can remove the permission check altogether.  At this
point POSIX does not even require the existince of any files or device
nodes, and FreeBSD proves out that users of ptys don't care by
implementing a dedicated system call to create ptys that does contain
any permission checks.  So only the small set of linux specific
chroot/container creating applications might care.  As the permissions
were not in any way the focus of this patchset I choose not to tackle
a possible user visible change like this.

>
> 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?"

I wish I could in conscience do that.  But unless we decide that
permission are irrelevant we are adding a permission bypass for an
existing operation.  Typically that is called a security bug.  I am not
comfortable doing that unless we simply decide we don't care.

If we decide we don't care I will add a patch at the front of the
patchset that implements don't care before all of the rest of this.

> 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)"/

Actually it is not pointless.  There is a second issue in all of this.
Right now it is possible to confuse the pt_chown setuid root binary
about which instance of devpts it should be calling chmod on.  I am not
certain it even ensures it is calling chown on a devpts entry.  It is
just hard coded paths today.

Right now userspace does something like:

masterfd = posix_openpt(O_RDWR);
grantpt(masterfd);
char *slave_name = ptsname(masterfd);
slavefd = open(slave_name);

Furthemore invoked by grantpt execs the pt_chown binary which does:

int pty_number;
ioctl(masterfd, TIOCGPTN, _number)
sprintf(slave_name, "/dev/pty/%u", pty_number);
chown(slave_name, some_uid, some_gid);

It would be very nice if we could have a way to close the races
in this mess and allow a program like pt_chown to actually only affect
the pty it cares about.  There is only one way I know to implement this
in a backwards compatible way and that is to have
readlink("/proc/self/fd/${masterfd}"), stat("/proc/self/fd/${masterfd}",
and open("/proc/self/fd/${masterfd}") talk about "/dev/pts/ptmx"
for a non-system instance of devpts

That will at least allow ptsname to find the proper instance of devpts.

I suppose it becomes hameless if grantpt stops calling a setuid root
exectuable if the connection between the master and the slave pty
gets confused and pt_chown, does not exist anymore.  But it feels
wrong to allow userspace no way to ask the question which mounted
instance of devpts does this masterfd belong to.  Especially
as readlink("/proc/self/fd/${masterfd}") is the natural way to ask that.

If anyone has a better idea on how userspace should connect the master
pty file descriptor the slave file descriptor, I would be willing to
implement that instead.

> 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 

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  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

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  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

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

[Al Viro]

On Tue, Apr 05, 2016 at 03:54:25AM +0100, Al Viro wrote:

> That, I take it, is a lookup for .. and buggering off if it fails *or* if
> we had been in caller's root or something that overmount it?  Not that the
> latter had been possible - root is a directory and can be overmounted only
> by another such, and we are called from ->open() of a device node.
> 
> > +   /* Remember the result of this permission check for later */
> > +   ret = inode_permission(path.dentry->d_inode, MAY_EXEC);
> > +   if (path_pts())
> > +   goto fail;
> 
> Egads, man - you've just introduced a special function for looking up
> something named "pts" in a given directory!
> 
> The reason not to use kern_path() would be what, the fact that it doesn't
> allow starting at given location?  So let's make a variant that would - and
> rather than bothering with RCU, just go for something like (completely
> untested)

Ah...  Right, that would demand exec permissions on the starting point.
Still, this is incredibly ugly ;-/  I'll try to come up with something
more tolerable, but this "path_pts" thing is too ugly to live.  Seriously.

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

[Al Viro]

On Tue, Apr 05, 2016 at 03:54:25AM +0100, Al Viro wrote:

> That, I take it, is a lookup for .. and buggering off if it fails *or* if
> we had been in caller's root or something that overmount it?  Not that the
> latter had been possible - root is a directory and can be overmounted only
> by another such, and we are called from ->open() of a device node.
> 
> > +   /* Remember the result of this permission check for later */
> > +   ret = inode_permission(path.dentry->d_inode, MAY_EXEC);
> > +   if (path_pts())
> > +   goto fail;
> 
> Egads, man - you've just introduced a special function for looking up
> something named "pts" in a given directory!
> 
> The reason not to use kern_path() would be what, the fact that it doesn't
> allow starting at given location?  So let's make a variant that would - and
> rather than bothering with RCU, just go for something like (completely
> untested)

Ah...  Right, that would demand exec permissions on the starting point.
Still, this is incredibly ugly ;-/  I'll try to come up with something
more tolerable, but this "path_pts" thing is too ugly to live.  Seriously.

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

[Al Viro]

On Mon, Apr 04, 2016 at 08:29:17PM -0500, Eric W. Biederman wrote:

> +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> +static int devpts_path_ptmx(struct file *filp)
> +{
> + struct pts_fs_info *fsi;
> + struct path root, path;
> + struct dentry *old;
> + int err = -ENOENT;
> + int ret;
> +
> + /* Can the pts filesystem be found with a path walk? */
> + path = filp->f_path;
> + path_get();
> + get_fs_root(current->fs, );
> + ret = path_parent(, );
> + path_put();
> + if (ret != 1)
> + goto fail;

That, I take it, is a lookup for .. and buggering off if it fails *or* if
we had been in caller's root or something that overmount it?  Not that the
latter had been possible - root is a directory and can be overmounted only
by another such, and we are called from ->open() of a device node.

> + /* Remember the result of this permission check for later */
> + ret = inode_permission(path.dentry->d_inode, MAY_EXEC);
> + if (path_pts())
> + goto fail;

Egads, man - you've just introduced a special function for looking up
something named "pts" in a given directory!

The reason not to use kern_path() would be what, the fact that it doesn't
allow starting at given location?  So let's make a variant that would - and
rather than bothering with RCU, just go for something like (completely
untested)

/* on success overwrite *path with the result of walk; do _not_ drop the
   reference to old contents - let the caller arrange that */
int kern_path_relative(struct path *path, const char *s, int flags)
{
int err;
struct nameidata nd = {.path = *path};
struct filename *name;

if (!*s || *s == '/' || flags & (LOOKUP_ROOT | LOOKUP_RCU))
return -EINVAL;

name = getname_kernel(s);
if (IS_ERR(name))
return PTR_ERR(name);

set_nameidata(, AT_FDCWD, name);  

nd.last_type = LAST_ROOT;
nd.flags = flags | LOOKUP_REVAL | LOOKUP_JUMPED | LOOKUP_PARENT;
nd.m_seq = read_seqbegin(_lock);
path_get();
nd.inode = nd.path.dentry->d_inode;

while (!(err = link_path_walk(s, )) 
&& ((err = lookup_last()) > 0)) {
s = trailing_symlink();
if (IS_ERR(s)) {
err = PTR_ERR(s);
break;
}
}
if (!err)
err = complete_walk();

if (!err && flags & LOOKUP_DIRECTORY)
if (!d_can_lookup(nd.path.dentry))
err = -ENOTDIR;
if (!err) {
*path = nd.path;
nd.path.mnt = NULL;
nd.path.dentry = NULL;
}
terminate_walk();
restore_nameidata();
putname(name);
return err;
}

and use it as

path = filp->f_path;
err = kern_path_relative(, "../pts", LOOKUP_DIRECTORY);
if (err)
return err;
/* from here on we need to path_put() it */
if (path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC)
goto fail;
/* must be its root; no other directories on that puppy */

> + fsi = DEVPTS_SB(path.mnt->mnt_sb);
> +
> + /* Get out if the path walk resulted in the default devpts instance */
> + if (devpts_mnt->mnt_sb == path.mnt->mnt_sb)
> + goto fail;
> +
> + /* Don't allow bypassing the existing /dev/pts/ptmx permission check */

err = inode_permission(path.dentry->d_inode, MAY_EXEC);
if (err)
goto fail;
err = inode_permission(fsi->ptmx_dentry->d_inode,
   ACC_MODE(filp->f_flags));
if (err)
goto fail;

> + /* Advance path to the ptmx dentry */
> + old = path.dentry;
> + path.dentry = dget(fsi->ptmx_dentry);
> + dput(old);
> +
> + /* Make it look like /dev/pts/ptmx was opened */
> + err = update_file_path(filp, );
> + if (err)
> + goto fail;
> +
> + return 0;
> +fail:
> + path_put();
> + return err;
> +}
> +#else
> +static inline int devpts_path_ptmx(struct file *filp)
> +{
> + return -ENOENT;
> +}
> +#endif
> +
> +struct inode *devpts_ptmx(struct inode *inode, struct file *filp)
> +{
> + int err;
> + if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
> + return inode;
> +
> + err = devpts_path_ptmx(filp);
> + if (err == 0)
> + return filp->f_inode;
> + if (err != -ENOENT)
> + return ERR_PTR(err);
> +
> + return inode;
> +}

Umm...  I'm not sure it makes for good calling conventions - the caller can
do inode = file_inode(filp) just as well, so why not simply return 0 or -E...?
"return inode;" cases become simply return 0...

> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1415,29 +1415,41 @@ static void follow_mount(struct path *path)
>   }
>  }
>  
> -static int follow_dotdot(struct nameidata *nd)

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

[Al Viro]

On Mon, Apr 04, 2016 at 08:29:17PM -0500, Eric W. Biederman wrote:

> +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> +static int devpts_path_ptmx(struct file *filp)
> +{
> + struct pts_fs_info *fsi;
> + struct path root, path;
> + struct dentry *old;
> + int err = -ENOENT;
> + int ret;
> +
> + /* Can the pts filesystem be found with a path walk? */
> + path = filp->f_path;
> + path_get();
> + get_fs_root(current->fs, );
> + ret = path_parent(, );
> + path_put();
> + if (ret != 1)
> + goto fail;

That, I take it, is a lookup for .. and buggering off if it fails *or* if
we had been in caller's root or something that overmount it?  Not that the
latter had been possible - root is a directory and can be overmounted only
by another such, and we are called from ->open() of a device node.

> + /* Remember the result of this permission check for later */
> + ret = inode_permission(path.dentry->d_inode, MAY_EXEC);
> + if (path_pts())
> + goto fail;

Egads, man - you've just introduced a special function for looking up
something named "pts" in a given directory!

The reason not to use kern_path() would be what, the fact that it doesn't
allow starting at given location?  So let's make a variant that would - and
rather than bothering with RCU, just go for something like (completely
untested)

/* on success overwrite *path with the result of walk; do _not_ drop the
   reference to old contents - let the caller arrange that */
int kern_path_relative(struct path *path, const char *s, int flags)
{
int err;
struct nameidata nd = {.path = *path};
struct filename *name;

if (!*s || *s == '/' || flags & (LOOKUP_ROOT | LOOKUP_RCU))
return -EINVAL;

name = getname_kernel(s);
if (IS_ERR(name))
return PTR_ERR(name);

set_nameidata(, AT_FDCWD, name);  

nd.last_type = LAST_ROOT;
nd.flags = flags | LOOKUP_REVAL | LOOKUP_JUMPED | LOOKUP_PARENT;
nd.m_seq = read_seqbegin(_lock);
path_get();
nd.inode = nd.path.dentry->d_inode;

while (!(err = link_path_walk(s, )) 
&& ((err = lookup_last()) > 0)) {
s = trailing_symlink();
if (IS_ERR(s)) {
err = PTR_ERR(s);
break;
}
}
if (!err)
err = complete_walk();

if (!err && flags & LOOKUP_DIRECTORY)
if (!d_can_lookup(nd.path.dentry))
err = -ENOTDIR;
if (!err) {
*path = nd.path;
nd.path.mnt = NULL;
nd.path.dentry = NULL;
}
terminate_walk();
restore_nameidata();
putname(name);
return err;
}

and use it as

path = filp->f_path;
err = kern_path_relative(, "../pts", LOOKUP_DIRECTORY);
if (err)
return err;
/* from here on we need to path_put() it */
if (path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC)
goto fail;
/* must be its root; no other directories on that puppy */

> + fsi = DEVPTS_SB(path.mnt->mnt_sb);
> +
> + /* Get out if the path walk resulted in the default devpts instance */
> + if (devpts_mnt->mnt_sb == path.mnt->mnt_sb)
> + goto fail;
> +
> + /* Don't allow bypassing the existing /dev/pts/ptmx permission check */

err = inode_permission(path.dentry->d_inode, MAY_EXEC);
if (err)
goto fail;
err = inode_permission(fsi->ptmx_dentry->d_inode,
   ACC_MODE(filp->f_flags));
if (err)
goto fail;

> + /* Advance path to the ptmx dentry */
> + old = path.dentry;
> + path.dentry = dget(fsi->ptmx_dentry);
> + dput(old);
> +
> + /* Make it look like /dev/pts/ptmx was opened */
> + err = update_file_path(filp, );
> + if (err)
> + goto fail;
> +
> + return 0;
> +fail:
> + path_put();
> + return err;
> +}
> +#else
> +static inline int devpts_path_ptmx(struct file *filp)
> +{
> + return -ENOENT;
> +}
> +#endif
> +
> +struct inode *devpts_ptmx(struct inode *inode, struct file *filp)
> +{
> + int err;
> + if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
> + return inode;
> +
> + err = devpts_path_ptmx(filp);
> + if (err == 0)
> + return filp->f_inode;
> + if (err != -ENOENT)
> + return ERR_PTR(err);
> +
> + return inode;
> +}

Umm...  I'm not sure it makes for good calling conventions - the caller can
do inode = file_inode(filp) just as well, so why not simply return 0 or -E...?
"return inode;" cases become simply return 0...

> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1415,29 +1415,41 @@ static void follow_mount(struct path *path)
>   }
>  }
>  
> -static int follow_dotdot(struct nameidata *nd)

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

[Eric W. Biederman]

This is in preparation for forcing each mount of devpts to be a
distinct filesystem.  The goal of this change is to have as few user
visible changes from the kernel today as possible.

On each open of /dev/ptmx look at the relative path pts and see if
devpts is mounted there.

If the filesystem found via the path lookup is the system devpts
do exactly what we do today.

If the filesystem found is not the system devpts make it appear
to the rest of the system that the /dev/pts/ptmx node was opened.
This includes respecting the permission checks of /dev/pts/ptmx
and updating the file's path to point to /dev/pts/ptmx.

In practice I expect the permission checks are a non-issue as the
permissions on /dev/ptmx and /dev/pts/ptmx are always 0666.  If
someone happens to change the permission on /dev/pts/ptmx is seems
important to honor them for the sake of backwards compatibility.  As
/dev/ptmx can be bind mounted to set next to any devpts filesystem not
honoring the permissions would provide a nice permission bypass under
the right circumstances if we did not check the permissions.

Similarly reflect the instance of devpts that was opened in f_path so
that we preserve the only way available today to test if someone is
attempting to confuse a pty creating program.

This winds up using 3 new vfs helpers path_parent, path_pts, and
update_file_path.

Signed-off-by: "Eric W. Biederman" 
---
 drivers/tty/pty.c |  4 +++
 fs/devpts/inode.c | 81 +++
 fs/namei.c| 64 ++---
 fs/open.c | 19 +++
 include/linux/devpts_fs.h |  2 ++
 include/linux/fs.h|  2 ++
 include/linux/namei.h |  3 ++
 7 files changed, 164 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index e16a49b507ef..557858ef00f5 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -725,6 +725,10 @@ static int ptmx_open(struct inode *inode, struct file 
*filp)
int retval;
int index;
 
+   inode = devpts_ptmx(inode, filp);
+   if (IS_ERR(inode))
+   return PTR_ERR(inode);
+
nonseekable_open(inode, filp);
 
/* We refuse fsnotify events on ptmx, since it's a shared resource */
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 655f21f99160..c14d51795577 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -17,6 +17,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -136,6 +137,86 @@ static inline struct pts_fs_info *DEVPTS_SB(struct 
super_block *sb)
return sb->s_fs_info;
 }
 
+#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
+static int devpts_path_ptmx(struct file *filp)
+{
+   struct pts_fs_info *fsi;
+   struct path root, path;
+   struct dentry *old;
+   int err = -ENOENT;
+   int ret;
+
+   /* Can the pts filesystem be found with a path walk? */
+   path = filp->f_path;
+   path_get();
+   get_fs_root(current->fs, );
+   ret = path_parent(, );
+   path_put();
+   if (ret != 1)
+   goto fail;
+
+   /* Remember the result of this permission check for later */
+   ret = inode_permission(path.dentry->d_inode, MAY_EXEC);
+   if (path_pts())
+   goto fail;
+
+   /* Is path the root of a devpts filesystem? */
+   if ((path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC) ||
+   (path.mnt->mnt_root != path.mnt->mnt_sb->s_root))
+   goto fail;
+   fsi = DEVPTS_SB(path.mnt->mnt_sb);
+
+   /* Get out if the path walk resulted in the default devpts instance */
+   if (devpts_mnt->mnt_sb == path.mnt->mnt_sb)
+   goto fail;
+
+   /* Don't allow bypassing the existing /dev/pts/ptmx permission check */
+   err = ret;
+   if (!err)
+   err = inode_permission(path.dentry->d_inode, MAY_EXEC);
+   if (!err)
+   err = inode_permission(fsi->ptmx_dentry->d_inode,
+  ACC_MODE(filp->f_flags));
+   if (err)
+   goto fail;
+
+   /* Advance path to the ptmx dentry */
+   old = path.dentry;
+   path.dentry = dget(fsi->ptmx_dentry);
+   dput(old);
+
+   /* Make it look like /dev/pts/ptmx was opened */
+   err = update_file_path(filp, );
+   if (err)
+   goto fail;
+
+   return 0;
+fail:
+   path_put();
+   return err;
+}
+#else
+static inline int devpts_path_ptmx(struct file *filp)
+{
+   return -ENOENT;
+}
+#endif
+
+struct inode *devpts_ptmx(struct inode *inode, struct file *filp)
+{
+   int err;
+   if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
+   return inode;
+
+   err = devpts_path_ptmx(filp);
+   if (err == 0)
+   return filp->f_inode;
+   if (err != -ENOENT)
+   return ERR_PTR(err);
+
+   return inode;
+}
+
 static inline struct super_block 

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

[Eric W. Biederman]

This is in preparation for forcing each mount of devpts to be a
distinct filesystem.  The goal of this change is to have as few user
visible changes from the kernel today as possible.

On each open of /dev/ptmx look at the relative path pts and see if
devpts is mounted there.

If the filesystem found via the path lookup is the system devpts
do exactly what we do today.

If the filesystem found is not the system devpts make it appear
to the rest of the system that the /dev/pts/ptmx node was opened.
This includes respecting the permission checks of /dev/pts/ptmx
and updating the file's path to point to /dev/pts/ptmx.

In practice I expect the permission checks are a non-issue as the
permissions on /dev/ptmx and /dev/pts/ptmx are always 0666.  If
someone happens to change the permission on /dev/pts/ptmx is seems
important to honor them for the sake of backwards compatibility.  As
/dev/ptmx can be bind mounted to set next to any devpts filesystem not
honoring the permissions would provide a nice permission bypass under
the right circumstances if we did not check the permissions.

Similarly reflect the instance of devpts that was opened in f_path so
that we preserve the only way available today to test if someone is
attempting to confuse a pty creating program.

This winds up using 3 new vfs helpers path_parent, path_pts, and
update_file_path.

Signed-off-by: "Eric W. Biederman" 
---
 drivers/tty/pty.c |  4 +++
 fs/devpts/inode.c | 81 +++
 fs/namei.c| 64 ++---
 fs/open.c | 19 +++
 include/linux/devpts_fs.h |  2 ++
 include/linux/fs.h|  2 ++
 include/linux/namei.h |  3 ++
 7 files changed, 164 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index e16a49b507ef..557858ef00f5 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -725,6 +725,10 @@ static int ptmx_open(struct inode *inode, struct file 
*filp)
int retval;
int index;
 
+   inode = devpts_ptmx(inode, filp);
+   if (IS_ERR(inode))
+   return PTR_ERR(inode);
+
nonseekable_open(inode, filp);
 
/* We refuse fsnotify events on ptmx, since it's a shared resource */
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 655f21f99160..c14d51795577 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -17,6 +17,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -136,6 +137,86 @@ static inline struct pts_fs_info *DEVPTS_SB(struct 
super_block *sb)
return sb->s_fs_info;
 }
 
+#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
+static int devpts_path_ptmx(struct file *filp)
+{
+   struct pts_fs_info *fsi;
+   struct path root, path;
+   struct dentry *old;
+   int err = -ENOENT;
+   int ret;
+
+   /* Can the pts filesystem be found with a path walk? */
+   path = filp->f_path;
+   path_get();
+   get_fs_root(current->fs, );
+   ret = path_parent(, );
+   path_put();
+   if (ret != 1)
+   goto fail;
+
+   /* Remember the result of this permission check for later */
+   ret = inode_permission(path.dentry->d_inode, MAY_EXEC);
+   if (path_pts())
+   goto fail;
+
+   /* Is path the root of a devpts filesystem? */
+   if ((path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC) ||
+   (path.mnt->mnt_root != path.mnt->mnt_sb->s_root))
+   goto fail;
+   fsi = DEVPTS_SB(path.mnt->mnt_sb);
+
+   /* Get out if the path walk resulted in the default devpts instance */
+   if (devpts_mnt->mnt_sb == path.mnt->mnt_sb)
+   goto fail;
+
+   /* Don't allow bypassing the existing /dev/pts/ptmx permission check */
+   err = ret;
+   if (!err)
+   err = inode_permission(path.dentry->d_inode, MAY_EXEC);
+   if (!err)
+   err = inode_permission(fsi->ptmx_dentry->d_inode,
+  ACC_MODE(filp->f_flags));
+   if (err)
+   goto fail;
+
+   /* Advance path to the ptmx dentry */
+   old = path.dentry;
+   path.dentry = dget(fsi->ptmx_dentry);
+   dput(old);
+
+   /* Make it look like /dev/pts/ptmx was opened */
+   err = update_file_path(filp, );
+   if (err)
+   goto fail;
+
+   return 0;
+fail:
+   path_put();
+   return err;
+}
+#else
+static inline int devpts_path_ptmx(struct file *filp)
+{
+   return -ENOENT;
+}
+#endif
+
+struct inode *devpts_ptmx(struct inode *inode, struct file *filp)
+{
+   int err;
+   if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
+   return inode;
+
+   err = devpts_path_ptmx(filp);
+   if (err == 0)
+   return filp->f_inode;
+   if (err != -ENOENT)
+   return ERR_PTR(err);
+
+   return inode;
+}
+
 static inline struct super_block *pts_sb_from_inode(struct inode