linux-kernel - Re: fs, tty: WARNING in devpts_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFyc3H+rGLUgaNxsS7BWUfKbwdgAu0YBkT83GzMF=AC70g@mail.gmail.com>
Date:   Sat, 3 Sep 2016 10:58:13 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Dmitry Vyukov <dvyukov@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Peter Hurley <peter@...leysoftware.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        LKML <linux-kernel@...r.kernel.org>,
        Jiri Slaby <jslaby@...e.com>,
        syzkaller <syzkaller@...glegroups.com>
Subject: Re: fs, tty: WARNING in devpts_get_priv

On Sat, Sep 3, 2016 at 9:40 AM, Eric W. Biederman <ebiederm@...ssion.com> wrote:
>
> Yes it looks like we need to stop supporting ptys that are not
> on /dev/pts.

We never have.

The warning was added to see if that case actually ever triggers, but
it didn't use to work before that either: we *used* to just return
NULL without warning at all (and I actually expected d_fsdata to be
NULL normally, but that's not necessarily true for all filesystems).

So replacing the warning with a "return NULL" will get the old behavior.

And I don't think your patch actually works anyway (did you test it? I
haven't, but I *think* it is broken), because the way the devpts pts
nodes work is that they do

        init_special_inode(inode, S_IFCHR|opts->mode,
MKDEV(UNIX98_PTY_SLAVE_MAJOR, index));

and then they rely on the vfs layer filling things in and then the tty
layer to do the lookup. So even the real devpts pty entries do rely on
that "tty_register_driver(pts_driver)", I think.

Which is all silly, of course: we now have the inode and the dentry,
so we *could* just have made the inode open op go directly to the pts
open, rather than indirectly through both the vfs character devices
*and* the tty layer open code.

But that would be a much bigger change. Probably not worth it.

So I think the correct thing to do for now is to just replace the
warning with a return NULL. Like we used to. So something like

  diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
  index d116453b0276..79a5941c2474 100644
  --- a/fs/devpts/inode.c
  +++ b/fs/devpts/inode.c
  @@ -585,7 +585,8 @@ struct dentry *devpts_pty_new(struct pts_fs_info
*fsi, int index, void *priv)
    */
   void *devpts_get_priv(struct dentry *dentry)
   {
  -       WARN_ON_ONCE(dentry->d_sb->s_magic != DEVPTS_SUPER_MAGIC);
  +       if (dentry->d_sb->s_magic != DEVPTS_SUPER_MAGIC)
  +               return NULL;
          return dentry->d_fsdata;
   }

and mark it for stable.

Comments?

                 Linus