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

Groups > linux.kernel > #1303955 > unrolled thread

Re: [PATCH] pty: fix 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

  Re: [PATCH] pty: fix use after free of tty->driver_data Peter Hurley <peter@hurleysoftware.com> - 2016-01-07 23:40 +0100
    Re: [PATCH] pty: fix use after free of tty->driver_data "Herton R. Krzesinski" <herton@redhat.com> - 2016-01-11 15:20 +0100

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

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

[toc] | [next] | [standalone]

#1306293

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web