Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753623AbbL2R66 (ORCPT ); Tue, 29 Dec 2015 12:58:58 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52974 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752628AbbL2R6z (ORCPT ); Tue, 29 Dec 2015 12:58:55 -0500 Date: Tue, 29 Dec 2015 15:58:53 -0200 From: "Herton R. Krzesinski" To: Peter Hurley Cc: linux-kernel@vger.kernel.org, Greg Kroah-Hartman , Jiri Slaby Subject: Re: [PATCH] pty: fix use after free of tty->driver_data Message-ID: <20151229175853.GA17652@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> <20151215180509.GC20334@dhcppc10.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151215180509.GC20334@dhcppc10.redhat.com> User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6912 Lines: 166 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? 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: 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, }; -- 2.4.3 That is, move devpts_kill_index up into pty_close(). It also resolves the problem, while at the same time handles another problem which my previous patch didn't catch, for example look at this other test case: #define _XOPEN_SOURCE #include #include #include #include #include #include int main(int argc, char **argv) { pid_t pid; int ptm_fd, pty_fd, tty_fd; system("mkdir -p /mnt/newpts"); system("mount -t devpts -o newinstance none /mnt/newpts"); pid = fork(); if (pid != 0) exit(0); daemon(1, 0); ptm_fd = open("/mnt/newpts/ptmx", O_RDWR); unlockpt(ptm_fd); pty_fd = open("/mnt/newpts/0", O_RDWR); tty_fd = open("/dev/tty", O_RDWR); pid = fork(); if (pid == 0) { ioctl(tty_fd, TIOCNOTTY, NULL); setsid(); sleep(20); close(pty_fd); close(ptm_fd); system("umount /mnt/newpts"); sleep(10); exit(0); } sleep(10); return 0; } The idea here is to umount a pts mount while still we have /dev/tty pointing to a pty opened... And of course it doesn't go well with the late devpts_kill_index: [ 1326.233991] ------------[ cut here ]------------ [ 1326.234014] WARNING: CPU: 1 PID: 2668 at lib/idr.c:1051 ida_remove+0x9b/0x130() [ 1326.234015] ida_remove called for id=0 which is not allocated. [ 1326.234016] Modules linked in: 8021q mrp garp stp llc nf_conntrack_ipv4 nf_defrag_ipv4 ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6 nf_defrag_ipv6 xt_state nf_conntrack ip6table_filter ip6_tables binfmt_misc ppdev joydev floppy parport_pc parport serio_raw tpm_tis tpm virtio_balloon virtio_console virtio_net iosf_mbi crct10dif_pclmul crc32_pclmul pcspkr snd_hda_codec_generic i2c_piix4 snd_hda_intel snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device snd_pcm snd_timer snd soundcore qxl ttm drm_kms_helper drm virtio_blk crc32c_intel virtio_pci virtio_ring virtio pata_acpi ata_generic [last unloaded: speedstep_lib] [ 1326.234061] CPU: 1 PID: 2668 Comm: newpty Not tainted 4.4.0-rc7 #1 [ 1326.234062] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.1-20150318_183358- 04/01/2014 [ 1326.234065] 000000000000041b ffff8800375ffbc8 ffffffff8139aed4 0000000000000009 [ 1326.234068] ffff8800375ffc18 000000000000041b ffff8800375ffc18 ffff8800375ffc08 [ 1326.234069] ffffffff81096f15 ffff8800375ffbf8 ffff8801399c47e0 0000000000000000 [ 1326.234071] Call Trace: [ 1326.234083] [] dump_stack+0x48/0x64 [ 1326.234092] [] warn_slowpath_common+0x95/0xe0 [ 1326.234094] [] warn_slowpath_fmt+0x46/0x50 [ 1326.234104] [] ? kfree+0x112/0x150 [ 1326.234105] [] ida_remove+0x9b/0x130 [ 1326.234111] [] devpts_kill_index+0x57/0x80 [ 1326.234120] [] pty_unix98_shutdown+0x18/0x20 [ 1326.234124] [] release_tty+0x3e/0xe0 [ 1326.234129] [] ? mutex_lock+0x16/0x40 [ 1326.234131] [] tty_release+0x44a/0x580 [ 1326.234135] [] __fput+0xb5/0x200 [ 1326.234137] [] ____fput+0xe/0x10 [ 1326.234143] [] task_work_run+0x68/0xa0 [ 1326.234145] [] do_exit+0x320/0x670 [ 1326.234147] [] ? __do_page_fault+0x1a4/0x450 [ 1326.234155] [] ? __audit_syscall_entry+0xb0/0x110 [ 1326.234161] [] ? do_audit_syscall_entry+0x66/0x70 [ 1326.234164] [] do_group_exit+0x51/0xc0 [ 1326.234166] [] SyS_exit_group+0x17/0x20 [ 1326.234169] [] entry_SYSCALL_64_fastpath+0x12/0x71 [ 1326.234171] ---[ end trace 23cebcbb1a28e0e8 ]--- So to avoid all this problem/headache with the late devpts_kill_index, we should just move devpts_kill_index up into pty_close I think. I don't see any problem with this yet. If you are ok I would like to submit the new diff/patch above with a proper changelog/signoff etc. -- []'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/