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

[Peter Hurley]

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

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