From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753632AbcAGWgL (ORCPT ); Thu, 7 Jan 2016 17:36:11 -0500 Received: from mail-pa0-f50.google.com ([209.85.220.50]:36119 "EHLO mail-pa0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752593AbcAGWgJ (ORCPT ); Thu, 7 Jan 2016 17:36:09 -0500 Subject: Re: [PATCH] pty: fix use after free of tty->driver_data To: "Herton R. Krzesinski" References: <1450150179-20925-1-git-send-email-herton@redhat.com> <1450150179-20925-2-git-send-email-herton@redhat.com> <56704F9A.6050006@hurleysoftware.com> <20151215180509.GC20334@dhcppc10.redhat.com> <20151229175853.GA17652@dhcppc10.redhat.com> Cc: linux-kernel@vger.kernel.org, Greg Kroah-Hartman , Jiri Slaby From: Peter Hurley Message-ID: <568EE854.20608@hurleysoftware.com> Date: Thu, 7 Jan 2016 14:36:04 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <20151229175853.GA17652@dhcppc10.redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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, > }; > >