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

[Herton R. Krzesinski]

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.