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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 24 Aug 2017 10:51:05 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Linus Torvalds <torvalds@...ux-foundation.org>
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

Linus Torvalds <torvalds@...ux-foundation.org> writes:

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

No.  That seems correct.  Change made.  If nothing else it is cheaper
and clearer so even if the other version wasn't wrong it is a good idea.

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

That change is a revert to the old v4.12 code.  So it is definitely
not regression inducing.

Further devpts_pty_new allocates a dentry keeps it in the devpts
filesystem.  The dentry is good until devpts_pty_kill where the
dentry is unlinked and killed.

I figure not differences from v4.12 if the logic hasn't changed
seems a good good way to cut down on the search for bugs/regressions.

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

Correct. That is buggy.  Will fix before I resend.

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

Eric