Date: Tue, 15 Dec 2015 16:05:09 -0200
From: "Herton R. Krzesinski" <herton@redhat.com>
To: Peter Hurley <peter@hurleysoftware.com>
Cc: linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.com>
Subject: Re: [PATCH] pty: fix use after free of tty->driver_data
Message-ID: <20151215180509.GC20334@dhcppc10.redhat.com>
References: <1450150179-20925-1-git-send-email-herton@redhat.com>
 <1450150179-20925-2-git-send-email-herton@redhat.com>
 <56704F9A.6050006@hurleysoftware.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <56704F9A.6050006@hurleysoftware.com>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5044
Lines: 134

On Tue, Dec 15, 2015 at 09:36:26AM -0800, Peter Hurley wrote:
> 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.]

Doh, you are right. The changelog is wrong here, and I must remove as what
I wrote isn't possible. The problem is only related to /dev/tty.

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

yes :(

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

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

Hmm no. I did cc stable because the first report I got about this issue
was on RHEL 7 with 3.10 based kernel, so this issue goes far back
some releases that are still supported and similar code is there.

On a quick check on a 2.6.32 kernel, things were very different,
tty_release_dev() called directly devpts_kill_index with inode
from the same file being closed. I'll check more and adjust the tag.

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

-- 
[]'s
Herton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/