linux-kernel - Re: [PATCH 0/1] devpts: use dynamic

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 23 Aug 2017 20:24:35 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Stefan Lippers-Hollmann <s.l-h@....de>,
        Christian Brauner <christian.brauner@...onical.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Thorsten Leemhuis <regressions@...mhuis.info>
Subject: Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name

On Wed, Aug 23, 2017 at 8:11 PM, Eric W. Biederman
<ebiederm@...ssion.com> wrote:
> -static int pty_get_peer(struct tty_struct *tty, int flags)
> +int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags)
>  {
>         int fd = -1;
>         struct file *filp = NULL;
>         int retval = -EINVAL;
> +       struct path path;
> +
> +       if ((tty->driver->type != TTY_DRIVER_TYPE_PTY) ||
> +           (tty->driver->subtype != PTY_TYPE_MASTER))
> +               return -EIO;

No. Afaik, that could be a legact PTY, which wouldn't be ok.

I think you need to do

        if (tty->driver != ptm_driver)
                return -EIO;

which should check both that it's the unix98 pty, and that it's the master.

Maybe I'm missing something.

That check used to be implicit, in that only the unix98 pty's could
reach that pty_unix98_ioctl() function, so then testing just that it
was a master was sufficient.

> -       /* We need to cache a fake path for TIOCGPTPEER. */
> -       pts_path = kmalloc(sizeof(struct path), GFP_KERNEL);
> -       if (!pts_path)
> -               goto err_release;
> -       pts_path->mnt = filp->f_path.mnt;
> -       pts_path->dentry = dentry;
> -       path_get(pts_path);
> -       tty->link->driver_data = pts_path;
> +       tty->link->driver_data = dentry;

We used to do "path_get()". Shouldn't we now use "dget()"?

But maybe the slave dentry is guaranteed to be around and we don't
need to do that. So your approach may be fine. You did remove all the
path_put() calls too, so I guess it all matches up.

So this looks like it could be fine, but I'd like to make sure.

> +struct vfsmount *devpts_mnt(struct file *filp)
> +{
> +       struct path path;
> +       int err;
> +
> +       path = filp->f_path;
> +       path_get(&path);
> +
> +       err = devpts_ptmx_path(&path);
> +       if (err) {
> +               path_put(&path);
> +               path.mnt = ERR_PTR(err);
> +       }
> +       return path.mnt;
> +}

That can't be right. You're leaking the dentry that you're not returning, no?

But yes, apart from those comments, this looks like what I envisioned.

Needs testing, and needs more looking at those reference counts, but
otherwise looks good.

And while the patch is a bit bigger, I do like getting rid of that
'struct path' thing, and keeping just the dentry.

                      Linus