| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20151229175853.GA17652@dhcppc10.redhat.com>
Date: Tue, 29 Dec 2015 15:58:53 -0200
From: "Herton R. Krzesinski" <herton@...hat.com>
To: Peter Hurley <peter@...leysoftware.com>
Cc: linux-kernel@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jslaby@...e.com>
Subject: Re: [PATCH] pty: fix use after free of tty->driver_data
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 <fcntl.h>
#include <stdlib.h>
#include <sys/ioctl.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
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] [<ffffffff8139aed4>] dump_stack+0x48/0x64
[ 1326.234092] [<ffffffff81096f15>] warn_slowpath_common+0x95/0xe0
[ 1326.234094] [<ffffffff81097016>] warn_slowpath_fmt+0x46/0x50
[ 1326.234104] [<ffffffff811fc292>] ? kfree+0x112/0x150
[ 1326.234105] [<ffffffff8139c75b>] ida_remove+0x9b/0x130
[ 1326.234111] [<ffffffff8129f3c7>] devpts_kill_index+0x57/0x80
[ 1326.234120] [<ffffffff81480338>] pty_unix98_shutdown+0x18/0x20
[ 1326.234124] [<ffffffff81474a2e>] release_tty+0x3e/0xe0
[ 1326.234129] [<ffffffff8177d476>] ? mutex_lock+0x16/0x40
[ 1326.234131] [<ffffffff81475b0a>] tty_release+0x44a/0x580
[ 1326.234135] [<ffffffff8121e3d5>] __fput+0xb5/0x200
[ 1326.234137] [<ffffffff8121e5ce>] ____fput+0xe/0x10
[ 1326.234143] [<ffffffff810b2eb8>] task_work_run+0x68/0xa0
[ 1326.234145] [<ffffffff8109a3e0>] do_exit+0x320/0x670
[ 1326.234147] [<ffffffff810673d4>] ? __do_page_fault+0x1a4/0x450
[ 1326.234155] [<ffffffff811361d0>] ? __audit_syscall_entry+0xb0/0x110
[ 1326.234161] [<ffffffff81003376>] ? do_audit_syscall_entry+0x66/0x70
[ 1326.234164] [<ffffffff8109a781>] do_group_exit+0x51/0xc0
[ 1326.234166] [<ffffffff8109a807>] SyS_exit_group+0x17/0x20
[ 1326.234169] [<ffffffff8177f92e>] 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists