Messages in this thread | | | Date | Mon, 11 Jan 2016 12:11:57 -0200 | From | "Herton R. Krzesinski" <> | Subject | Re: [PATCH] pty: fix use after free of tty->driver_data |
| |
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.
| |