Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1306264 > unrolled thread

[PATCH 1/2 v2] pty: fix possible use after free of tty->driver_data

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2 v2] pty: fix possible use after free of tty->driver_data "Herton R. Krzesinski" <herton@redhat.com> - 2016-01-11 15:10 +0100
    Re: [PATCH 1/2 v2] pty: fix possible use after free of  tty->driver_data Peter Hurley <peter@hurleysoftware.com> - 2016-01-13 18:40 +0100
      Re: [PATCH 1/2 v2] pty: fix possible use after free of  tty->driver_data Josh Triplett <josh@joshtriplett.org> - 2016-01-13 19:30 +0100
        Re: [PATCH 1/2 v2] pty: fix possible use after free of  tty->driver_data "Herton R. Krzesinski" <herton@redhat.com> - 2016-01-14 21:20 +0100
          Re: [PATCH 1/2 v2] pty: fix possible use after free of  tty->driver_data Peter Hurley <peter@hurleysoftware.com> - 2016-01-14 22:30 +0100

#1306264 — [PATCH 1/2 v2] pty: fix possible use after free of tty->driver_data

This change fixes a bug for a corner case where we have the the last
release from a pty master/slave coming from a previously opened /dev/tty
file. When this happens, the tty->driver_data can be stale, due to all
ptmx or pts/N files having already been closed before (and thus the inode
related to these files, which tty->driver_data points to, being already
freed/destroyed).

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.

Signed-off-by: Herton R. Krzesinski <herton@redhat.com>
Cc: <stable@vger.kernel.org> # 2.6.29+
---
 drivers/tty/pty.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

v2: fixed patch changelog

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index a45660f..96016e5 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 case where all references to ptmx inode are dropped and we
+	 * still have /dev/tty opened pointing to the master/slave pair (ptmx
+	 * is closed/released before /dev/tty), we must make sure that the inode
+	 * is still valid when we call the final pty_unix98_shutdown, thus we
+	 * hold an additional reference to the ptmx inode
+	 */
+	ihold(inode);
+
 	tty_add_file(tty, filp);
 
 	slave_inode = devpts_pty_new(inode,
-- 
2.4.3

[toc] | [next] | [standalone]

#1308665 — Re: [PATCH 1/2 v2] pty: fix possible use after free of tty->driver_data

On 01/11/2016 06:07 AM, Herton R. Krzesinski wrote:
> This change fixes a bug for a corner case where we have the the last
> release from a pty master/slave coming from a previously opened /dev/tty
> file. When this happens, the tty->driver_data can be stale, due to all
> ptmx or pts/N files having already been closed before (and thus the inode
> related to these files, which tty->driver_data points to, being already
> freed/destroyed).
> 
> 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.

Ideally, the tty core should be bumping the inode count for the underlying
controlling tty but I'm not sure how to make that work atm, and this
fixes the (overwhelmingly) most common use-case.

Thanks again,

Reviewed-by: Peter Hurley <peter@hurleysoftware.com>

[toc] | [prev] | [next] | [standalone]

#1308712 — Re: [PATCH 1/2 v2] pty: fix possible use after free of tty->driver_data

On Wed, Jan 13, 2016 at 09:39:29AM -0800, Peter Hurley wrote:
> On 01/11/2016 06:07 AM, Herton R. Krzesinski wrote:
> > This change fixes a bug for a corner case where we have the the last
> > release from a pty master/slave coming from a previously opened /dev/tty
> > file. When this happens, the tty->driver_data can be stale, due to all
> > ptmx or pts/N files having already been closed before (and thus the inode
> > related to these files, which tty->driver_data points to, being already
> > freed/destroyed).
> > 
> > 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.
> 
> Ideally, the tty core should be bumping the inode count for the underlying
> controlling tty

That does indeed sound like the right fix.  /dev/tty doesn't act exactly
like opening the underlying device (as it also supports the TIOCNOTTY
ioctl), but it should definitely hold a reference to that underlying
device.

[toc] | [prev] | [next] | [standalone]

#1309627 — Re: [PATCH 1/2 v2] pty: fix possible use after free of tty->driver_data

On Wed, Jan 13, 2016 at 10:28:44AM -0800, Josh Triplett wrote:
> On Wed, Jan 13, 2016 at 09:39:29AM -0800, Peter Hurley wrote:
> > On 01/11/2016 06:07 AM, Herton R. Krzesinski wrote:
> > > This change fixes a bug for a corner case where we have the the last
> > > release from a pty master/slave coming from a previously opened /dev/tty
> > > file. When this happens, the tty->driver_data can be stale, due to all
> > > ptmx or pts/N files having already been closed before (and thus the inode
> > > related to these files, which tty->driver_data points to, being already
> > > freed/destroyed).
> > > 
> > > 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.
> > 
> > Ideally, the tty core should be bumping the inode count for the underlying
> > controlling tty
> 
> That does indeed sound like the right fix.  /dev/tty doesn't act exactly
> like opening the underlying device (as it also supports the TIOCNOTTY
> ioctl), but it should definitely hold a reference to that underlying
> device.

Yeah, I thought previously as well that this should go to tty core (my thinking
though was to get extra references to the opened files instead of the inode).

However, what inode should I choose to increment the reference count in the
tty->tty_files list? I know most cases the device file will always be at the
/dev, but if it's opened from another place/file?

And handling this would overcomplicate a case which is pty specific, unless
I miss something here pty is the only inode user, and seem not worth or
useful to have at the moment this at tty core.

thanks,
-- 
Herton.

[toc] | [prev] | [next] | [standalone]

#1309657 — Re: [PATCH 1/2 v2] pty: fix possible use after free of tty->driver_data

On 01/14/2016 12:09 PM, Herton R. Krzesinski wrote:
> On Wed, Jan 13, 2016 at 10:28:44AM -0800, Josh Triplett wrote:
>> On Wed, Jan 13, 2016 at 09:39:29AM -0800, Peter Hurley wrote:
>>> On 01/11/2016 06:07 AM, Herton R. Krzesinski wrote:
>>>> This change fixes a bug for a corner case where we have the the last
>>>> release from a pty master/slave coming from a previously opened /dev/tty
>>>> file. When this happens, the tty->driver_data can be stale, due to all
>>>> ptmx or pts/N files having already been closed before (and thus the inode
>>>> related to these files, which tty->driver_data points to, being already
>>>> freed/destroyed).
>>>>
>>>> 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.
>>>
>>> Ideally, the tty core should be bumping the inode count for the underlying
>>> controlling tty
>>
>> That does indeed sound like the right fix.  /dev/tty doesn't act exactly
>> like opening the underlying device (as it also supports the TIOCNOTTY
>> ioctl), but it should definitely hold a reference to that underlying
>> device.

I'm ok with this fix for the moment because it's ideal for -stable;
simple, straightforward and addresses the one known use-case.


> Yeah, I thought previously as well that this should go to tty core (my thinking
> though was to get extra references to the opened files instead of the inode).
> 
> However, what inode should I choose to increment the reference count in the
> tty->tty_files list? I know most cases the device file will always be at the
> /dev, but if it's opened from another place/file?

I think the long-term solution would be to bump the inode ref at
controlling tty association because the appropriate inode is supplied for
either the ioctl(TIOCSCTTY) or the first qualifying open(). This inode
would be stored in the tty_struct, justifying the reference.

At dissociation, the inode reference would be dropped.

This approach also magically fixes the superblock pinning (because the inode
would be the slave inode so would be on the devpts fs, preventing umount).


> And handling this would overcomplicate a case which is pty specific, unless
> I miss something here pty is the only inode user, and seem not worth or
> useful to have at the moment this at tty core.

Any tty could be the process's controlling tty, and the tty core should be
pinning any fs objects related to that.

Regards,
Peter Hurley

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web