Re: [PATCH] pty: fix use after free of tty->driver_data

From: Herton R. Krzesinski
Date: Mon Jan 11 2016 - 09:12:06 EST


On Thu, Jan 07, 2016 at 02:36:04PM -0800, Peter Hurley wrote:
> On 12/29/2015 09:58 AM, Herton R. Krzesinski wrote:
> > On Tue, Dec 15, 2015 at 04:05:09PM -0200, Herton R. Krzesinski wrote:
> >> On Tue, Dec 15, 2015 at 09:36:26AM -0800, Peter Hurley wrote:
> >>>> since in this
> >>>> case any of the tty->driver_data can be stale, due to all references/
> >>>> files being closed before (files related to ptmx/pts inodes set at
> >>>> tty->driver_data), we have the possibility of referencing an already
> >>>> freed inode.
> >>>
> >>> As I wrote above, I believe this is the only possible circumstance
> >>> for which the file that is releasing could have stale pts inodes.
> >>>
> >>>
> >>>> The fix here is to keep a reference on the opened master ptmx inode.
> >>>> We maintain the inode referenced until the final pty_unix98_shutdown,
> >>>> and only pass this inode to devpts_kill_index.
> >>>
> >>> Let me think some on your proposed solution.
> >>
> >> Ok, let me know what you think, at least I will have to repost the patch
> >> with the changelog fixed, unless you think there is another/better solution
> >> for the issue.
> >
> > Hi Peter, any news on this issue?
>
> Sorry, I haven't forgotten this issue; just busy with the holidays, etc.
>
> > I gave some more thought and testing into this, and I think we simply should do
> > a change like below instead of my previous patch proposal:
>
> Regarding the patch below, the slave side hasn't been hung up yet
> (so could be in the middle of i/o at the time the index is released).

Ok, I see. I prepared a new fix and submitted now, along with resubmit
of previous patch with the fixed changelog.

>
> afaict, there is nothing wrong with your original solution, strictly
> speaking. What I was wondering at the time is if we would be better off
> in the long run teaching the pty driver about multi-instance devpts.
>
> But after giving it some thought, I think those changes can wait until
> a solution exists (or is part of the solution) for broken userspace
> devpts setups.
>
> IOW, please re-submit your earlier patch with the changelog edits.
>
> Regards,
> Peter Hurley
>
>
>
> > diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> > index a45660f..73e36bd 100644
> > --- a/drivers/tty/pty.c
> > +++ b/drivers/tty/pty.c
> > @@ -68,6 +68,7 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
> > mutex_lock(&devpts_mutex);
> > if (tty->link->driver_data)
> > devpts_pty_kill(tty->link->driver_data);
> > + devpts_kill_index(tty->driver_data, tty->index);
> > mutex_unlock(&devpts_mutex);
> > }
> > #endif
> > @@ -678,12 +679,6 @@ static void pty_unix98_remove(struct tty_driver *driver, struct tty_struct *tty)
> > {
> > }
> >
> > -/* this is called once with whichever end is closed last */
> > -static void pty_unix98_shutdown(struct tty_struct *tty)
> > -{
> > - devpts_kill_index(tty->driver_data, tty->index);
> > -}
> > -
> > static const struct tty_operations ptm_unix98_ops = {
> > .lookup = ptm_unix98_lookup,
> > .install = pty_unix98_install,
> > @@ -697,7 +692,6 @@ static const struct tty_operations ptm_unix98_ops = {
> > .unthrottle = pty_unthrottle,
> > .ioctl = pty_unix98_ioctl,
> > .resize = pty_resize,
> > - .shutdown = pty_unix98_shutdown,
> > .cleanup = pty_cleanup
> > };
> >
> > @@ -715,7 +709,6 @@ static const struct tty_operations pty_unix98_ops = {
> > .set_termios = pty_set_termios,
> > .start = pty_start,
> > .stop = pty_stop,
> > - .shutdown = pty_unix98_shutdown,
> > .cleanup = pty_cleanup,
> > };
> >
> >
>

thanks,
Herton.