Messages in this thread | | | Subject | Re: [PATCH] pty: fix use after free of tty->driver_data | From | Peter Hurley <> | Date | Tue, 15 Dec 2015 09:36:26 -0800 |
| |
Hi Herton,
On 12/14/2015 07:29 PM, Herton R. Krzesinski wrote: > pty_unix98_shutdown allows a potential use after free of inode from > slave tty->driver_data: if final pty close is called with slave > tty_struct, and inode was released already by devpts_pty_kill at > pty_close, pty_unix98_shutdown will access stale data.
I'm not following this logic.
Suppose there is no open current tty alias for the slave pty; then there is one inode reference for the master and N + 1 inode references for however many times the slave has been opened.
If the master is closed first, the slave dentry is dropped so there are now N slave inode references. When the last slave closes, the slave inode is still valid when devpts_kill_index() is called.
So afaict, this problem only applies when /dev/tty is the final close, and at no other time. [Not to minimize the scale of the problem: quite often, /dev/tty would be the last file closed.]
> If the evicted inode is quickly reused again as another inode > instance, this can potentially break in the case the inode is on a > different devpts instance than the default devpts mount, not to > mention a possible ops if the inode_cache slab is destroyed and > reused as something else. > > Also there is an evident problem in case the last close is from a > opened "/dev/tty" which points to the master/slave pty:
/dev/tty can only refer to the slave pty.
> 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.
> Signed-off-by: Herton R. Krzesinski <herton@redhat.com> > Cc: <stable@vger.kernel.org>
Afaict, the stable tag goes back to the original implementation. Did you research how far back the /dev/tty alias problem goes?
Regards, Peter Hurley
> --- > drivers/tty/pty.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c > index a45660f..90743b0 100644 > --- a/drivers/tty/pty.c > +++ b/drivers/tty/pty.c > @@ -681,7 +681,14 @@ 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); > + struct inode *ptmx_inode; > + > + if (tty->driver->subtype == PTY_TYPE_MASTER) > + ptmx_inode = tty->driver_data; > + else > + ptmx_inode = tty->link->driver_data; > + devpts_kill_index(ptmx_inode, tty->index); > + iput(ptmx_inode); /* drop reference we acquired at ptmx_open */ > } > > static const struct tty_operations ptm_unix98_ops = { > @@ -773,6 +780,15 @@ static int ptmx_open(struct inode *inode, struct file *filp) > set_bit(TTY_PTY_LOCK, &tty->flags); /* LOCK THE SLAVE */ > tty->driver_data = inode; > > + /* > + * In the rare case all references to ptmx inode are dropped (files > + * closed), and we still have a device opened pointing to the > + * master/slave pair (eg., "/dev/tty" opened), we must make sure that > + * the inode is still valid when we call the final pty_unix98_shutdown: > + * thus we must hold an additional reference to the ptmx inode here > + */ > + ihold(inode); > + > tty_add_file(tty, filp); > > slave_inode = devpts_pty_new(inode, >
| |