pty: fix use after free/oops at pty_unix98_shutdown

pty: fix use after free/oops at pty_unix98_shutdown

[Herton R. Krzesinski]

Hi,

recently I got a report of a crash at pty_unix98_shutdown. after analyzing the
issue, I managed to create a small reproducer:

$ cat test.sh
#!/bin/sh

while true; do
        find /sys
        ./dopty
        echo 2 > /proc/sys/vm/drop_caches
        ps aux
        sleep 40
done

$ cat dopty.c
#define _XOPEN_SOURCE
#include <unistd.h>
#include <stdlib.h>
#include <fcntl.h>
#include <sys/stat.h>
#include <sys/ioctl.h>
#include <termios.h>
#include <errno.h>

int main(int argc, char **argv)
{
        pid_t pid;
        int ptm_fd, pty_fd, tty_fd, tout;
        char *pty_name;
        int c = 0;

        while (c < 100) {
                c++;
                pid = fork();
                if (pid != 0)
                        continue;
                daemon(1, 0);
                ptm_fd = posix_openpt(O_RDWR);
                if (ptm_fd < 0)
                        return -1;
                grantpt(ptm_fd);
                unlockpt(ptm_fd);
                pty_name = ptsname(ptm_fd);
                pty_fd = open(pty_name, O_RDWR);
                tty_fd = open("/dev/tty", O_RDWR);
                pid = fork();
                if (pid == 0)  {
                        ioctl(tty_fd, TIOCNOTTY, NULL);
                        setsid();
                        close(pty_fd);
                        close(ptm_fd);
                        sleep(60);
                        return 0;
                }
                sleep(10);
                return 0;
        }
        sleep(30);
        return 0;
}

Running test.sh above, after second iteration I'm able to get the following
oops at devpts_kill_index:

[ 1741.842926] test.sh (8786): drop_caches: 2
[ 1812.417114] test.sh (8786): drop_caches: 2
[ 1842.405991] BUG: unable to handle kernel NULL pointer dereference at 0000000000000060
[ 1842.406055] IP: [<ffffffff8129f36d>] devpts_kill_index+0x1d/0x80
[ 1842.406110] PGD 0 
[ 1842.406123] Oops: 0000 [#1] SMP 
[ 1842.406148] 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 ip6t
able_filter ip6_tables binfmt_misc ppdev parport_pc parport floppy joydev tpm_tis tpm virtio_balloon serio_raw virtio_console virtio_net iosf_mbi crct10dif_pclmul crc32_pclmul 
pcspkr qxl ttm drm_kms_helper drm snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device snd_pcm snd_timer snd soundcore i2c_piix4 virt
io_blk crc32c_intel virtio_pci virtio_ring virtio pata_acpi ata_generic [last unloaded: speedstep_lib]
[ 1842.406383] CPU: 2 PID: 9204 Comm: dopty Not tainted 4.4.0-rc5 #1
[ 1842.406404] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.1-20150318_183358- 04/01/2014
[ 1842.406427] task: ffff880039da3a00 ti: ffff880037a7c000 task.ti: ffff880037a7c000
[ 1842.406445] RIP: 0010:[<ffffffff8129f36d>]  [<ffffffff8129f36d>] devpts_kill_index+0x1d/0x80
[ 1842.406473] RSP: 0018:ffff880037a7fc98  EFLAGS: 00010282
[ 1842.406487] RAX: 0000000000000000 RBX: ffff880039e75c00 RCX: 00000001810000fd
[ 1842.406504] RDX: 00000000ffffffff RSI: 0000000000000004 RDI: ffff88003d0cc6a8
[ 1842.406521] RBP: ffff880037a7fca8 R08: ffffea0000ebcc50 R09: 0000000000000001
[ 1842.406539] R10: ffffea0000ebcc58 R11: 0000000000000000 R12: 0000000000000004
[ 1842.406556] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[ 1842.406573] FS:  0000000000000000(0000) GS:ffff88003fd00000(0000) knlGS:0000000000000000
[ 1842.406611] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1842.406634] CR2: 0000000000000060 CR3: 0000000001c0a000 CR4: 00000000001406e0
[ 1842.406665] Stack:
[ 1842.406677]  ffff880039e75c00 0000000000000000 ffff880037a7fcb8 ffffffff814802f8
[ 1842.406715]  ffff880037a7fce8 ffffffff814749fe ffff880000000000 ffffffff8177ce56
[ 1842.406785]  0000000000000000 ffff880039e75c00 ffff880037a7fd88 ffffffff81475ada
[ 1842.406821] Call Trace:
[ 1842.406821]  [<ffffffff814802f8>] pty_unix98_shutdown+0x18/0x20
[ 1842.406821]  [<ffffffff814749fe>] release_tty+0x3e/0xe0
[ 1842.406821]  [<ffffffff8177ce56>] ? mutex_lock+0x16/0x40
[ 1842.406821]  [<ffffffff81475ada>] tty_release+0x44a/0x580
[ 1842.406821]  [<ffffffff8121e3e5>] __fput+0xb5/0x200
[ 1842.406821]  [<ffffffff8121e5de>] ____fput+0xe/0x10
[ 1842.406821]  [<ffffffff810b2eb8>] task_work_run+0x68/0xa0
[ 1842.406821]  [<ffffffff8109a3e0>] do_exit+0x320/0x670
[ 1842.406821]  [<ffffffff810673d4>] ? __do_page_fault+0x1a4/0x450
[ 1842.406821]  [<ffffffff811361d0>] ? __audit_syscall_entry+0xb0/0x110
[ 1842.406821]  [<ffffffff81003376>] ? do_audit_syscall_entry+0x66/0x70
[ 1842.406821]  [<ffffffff8109a781>] do_group_exit+0x51/0xc0
[ 1842.406821]  [<ffffffff8109a807>] SyS_exit_group+0x17/0x20
[ 1842.406821]  [<ffffffff8177f2ee>] entry_SYSCALL_64_fastpath+0x12/0x71
[ 1842.406821] Code: 48 c7 c3 f4 ff ff ff eb 84 e8 10 7b df ff 55 48 89 e5 48 83 ec 10 48 89 1c 24 4c 89 64 24 08 66 66 66 66 90 48 8b 47 28 41 89 f4 <48> 81 78 60 d1 1c 00 00 74 12 48 8b 15 4a bd d6 00 31 c0 48 85 
[ 1842.406821] RIP  [<ffffffff8129f36d>] devpts_kill_index+0x1d/0x80
[ 1842.406821]  RSP <ffff880037a7fc98>
[ 1842.406821] CR2: 0000000000000060

The problem above is that on final close, pty_unix98_shutdown may give an stale/
already evicted inode from tty->driver_data. Easily to happen as the test case
shows, in case all opened /dev/pts/N references are closed before "/dev/tty". I
also expect in a rare case where all ptmx references are gone/closed, this also
could happen on final close when the master tty is given to pty_unix98_shutdown.

Following this is a proposed fix to the problem, I tested here and it fixed the
issue.

Please take a look and review, thanks!

Regards,
Herton.

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

[Herton R. Krzesinski]

pty_unix98_shutdown allows a potential use after free of inode from
slave tty->driver_data: if final pty close is called with slave
tty_struct, and inode was released already by devpts_pty_kill at
pty_close, pty_unix98_shutdown will access stale data. If the evicted
inode is quickly reused again as another inode instance, this can
potentially break in the case the inode is on a different devpts
instance than the default devpts mount, not to mention a possible ops
if the inode_cache slab is destroyed and reused as something else.

Also there is an evident problem in case the last close is from a
opened "/dev/tty" which points to the master/slave pty: 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.

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.

Signed-off-by: Herton R. Krzesinski <herton@redhat.com>
Cc: <stable@vger.kernel.org>
---
 drivers/tty/pty.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index a45660f..90743b0 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -681,7 +681,14 @@ 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);
+	struct inode *ptmx_inode;
+
+	if (tty->driver->subtype == PTY_TYPE_MASTER)
+		ptmx_inode = tty->driver_data;
+	else
+		ptmx_inode = tty->link->driver_data;
+	devpts_kill_index(ptmx_inode, tty->index);
+	iput(ptmx_inode); /* drop reference we acquired at ptmx_open */
 }
 
 static const struct tty_operations ptm_unix98_ops = {
@@ -773,6 +780,15 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 	set_bit(TTY_PTY_LOCK, &tty->flags); /* LOCK THE SLAVE */
 	tty->driver_data = inode;
 
+	/*
+	 * In the rare case all references to ptmx inode are dropped (files
+	 * closed), and we still have a device opened pointing to the
+	 * master/slave pair (eg., "/dev/tty" opened), we must make sure that
+	 * the inode is still valid when we call the final pty_unix98_shutdown:
+	 * thus we must hold an additional reference to the ptmx inode here
+	 */
+	ihold(inode);
+
 	tty_add_file(tty, filp);
 
 	slave_inode = devpts_pty_new(inode,
-- 
2.4.3

Re: pty: fix use after free/oops at pty_unix98_shutdown

[Peter Hurley]

Hi Herton,

On 12/14/2015 07:29 PM, Herton R. Krzesinski wrote:
> Hi,
> 
> recently I got a report of a crash at pty_unix98_shutdown. after analyzing the
> issue, I managed to create a small reproducer:
> 
> $ cat test.sh
> #!/bin/sh
> 
> while true; do
>         find /sys
>         ./dopty
>         echo 2 > /proc/sys/vm/drop_caches
>         ps aux
>         sleep 40
> done
> 
> $ cat dopty.c
> #define _XOPEN_SOURCE
> #include <unistd.h>
> #include <stdlib.h>
> #include <fcntl.h>
> #include <sys/stat.h>
> #include <sys/ioctl.h>
> #include <termios.h>
> #include <errno.h>
> 
> int main(int argc, char **argv)
> {
>         pid_t pid;
>         int ptm_fd, pty_fd, tty_fd, tout;
>         char *pty_name;
>         int c = 0;
> 
>         while (c < 100) {
>                 c++;
>                 pid = fork();
>                 if (pid != 0)
>                         continue;
>                 daemon(1, 0);
>                 ptm_fd = posix_openpt(O_RDWR);
>                 if (ptm_fd < 0)
>                         return -1;
>                 grantpt(ptm_fd);
>                 unlockpt(ptm_fd);
>                 pty_name = ptsname(ptm_fd);
>                 pty_fd = open(pty_name, O_RDWR);
>                 tty_fd = open("/dev/tty", O_RDWR);
>                 pid = fork();
>                 if (pid == 0)  {
>                         ioctl(tty_fd, TIOCNOTTY, NULL);
>                         setsid();
>                         close(pty_fd);
>                         close(ptm_fd);
>                         sleep(60);
>                         return 0;
>                 }
>                 sleep(10);
>                 return 0;
>         }
>         sleep(30);
>         return 0;
> }

Thanks for the reproducer!


> Running test.sh above, after second iteration I'm able to get the following
> oops at devpts_kill_index:
> 
> [ 1741.842926] test.sh (8786): drop_caches: 2
> [ 1812.417114] test.sh (8786): drop_caches: 2
> [ 1842.405991] BUG: unable to handle kernel NULL pointer dereference at 0000000000000060
> [ 1842.406055] IP: [<ffffffff8129f36d>] devpts_kill_index+0x1d/0x80
> [ 1842.406110] PGD 0 
> [ 1842.406123] Oops: 0000 [#1] SMP 
> [ 1842.406148] 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 ip6t
> able_filter ip6_tables binfmt_misc ppdev parport_pc parport floppy joydev tpm_tis tpm virtio_balloon serio_raw virtio_console virtio_net iosf_mbi crct10dif_pclmul crc32_pclmul 
> pcspkr qxl ttm drm_kms_helper drm snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device snd_pcm snd_timer snd soundcore i2c_piix4 virt
> io_blk crc32c_intel virtio_pci virtio_ring virtio pata_acpi ata_generic [last unloaded: speedstep_lib]
> [ 1842.406383] CPU: 2 PID: 9204 Comm: dopty Not tainted 4.4.0-rc5 #1
> [ 1842.406404] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.8.1-20150318_183358- 04/01/2014
> [ 1842.406427] task: ffff880039da3a00 ti: ffff880037a7c000 task.ti: ffff880037a7c000
> [ 1842.406445] RIP: 0010:[<ffffffff8129f36d>]  [<ffffffff8129f36d>] devpts_kill_index+0x1d/0x80
> [ 1842.406473] RSP: 0018:ffff880037a7fc98  EFLAGS: 00010282
> [ 1842.406487] RAX: 0000000000000000 RBX: ffff880039e75c00 RCX: 00000001810000fd
> [ 1842.406504] RDX: 00000000ffffffff RSI: 0000000000000004 RDI: ffff88003d0cc6a8
> [ 1842.406521] RBP: ffff880037a7fca8 R08: ffffea0000ebcc50 R09: 0000000000000001
> [ 1842.406539] R10: ffffea0000ebcc58 R11: 0000000000000000 R12: 0000000000000004
> [ 1842.406556] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> [ 1842.406573] FS:  0000000000000000(0000) GS:ffff88003fd00000(0000) knlGS:0000000000000000
> [ 1842.406611] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1842.406634] CR2: 0000000000000060 CR3: 0000000001c0a000 CR4: 00000000001406e0
> [ 1842.406665] Stack:
> [ 1842.406677]  ffff880039e75c00 0000000000000000 ffff880037a7fcb8 ffffffff814802f8
> [ 1842.406715]  ffff880037a7fce8 ffffffff814749fe ffff880000000000 ffffffff8177ce56
> [ 1842.406785]  0000000000000000 ffff880039e75c00 ffff880037a7fd88 ffffffff81475ada
> [ 1842.406821] Call Trace:
> [ 1842.406821]  [<ffffffff814802f8>] pty_unix98_shutdown+0x18/0x20
> [ 1842.406821]  [<ffffffff814749fe>] release_tty+0x3e/0xe0
> [ 1842.406821]  [<ffffffff8177ce56>] ? mutex_lock+0x16/0x40
> [ 1842.406821]  [<ffffffff81475ada>] tty_release+0x44a/0x580
> [ 1842.406821]  [<ffffffff8121e3e5>] __fput+0xb5/0x200
> [ 1842.406821]  [<ffffffff8121e5de>] ____fput+0xe/0x10
> [ 1842.406821]  [<ffffffff810b2eb8>] task_work_run+0x68/0xa0
> [ 1842.406821]  [<ffffffff8109a3e0>] do_exit+0x320/0x670
> [ 1842.406821]  [<ffffffff810673d4>] ? __do_page_fault+0x1a4/0x450
> [ 1842.406821]  [<ffffffff811361d0>] ? __audit_syscall_entry+0xb0/0x110
> [ 1842.406821]  [<ffffffff81003376>] ? do_audit_syscall_entry+0x66/0x70
> [ 1842.406821]  [<ffffffff8109a781>] do_group_exit+0x51/0xc0
> [ 1842.406821]  [<ffffffff8109a807>] SyS_exit_group+0x17/0x20
> [ 1842.406821]  [<ffffffff8177f2ee>] entry_SYSCALL_64_fastpath+0x12/0x71
> [ 1842.406821] Code: 48 c7 c3 f4 ff ff ff eb 84 e8 10 7b df ff 55 48 89 e5 48 83 ec 10 48 89 1c 24 4c 89 64 24 08 66 66 66 66 90 48 8b 47 28 41 89 f4 <48> 81 78 60 d1 1c 00 00 74 12 48 8b 15 4a bd d6 00 31 c0 48 85 
> [ 1842.406821] RIP  [<ffffffff8129f36d>] devpts_kill_index+0x1d/0x80
> [ 1842.406821]  RSP <ffff880037a7fc98>
> [ 1842.406821] CR2: 0000000000000060
> 
> The problem above is that on final close, pty_unix98_shutdown may give an stale/
> already evicted inode from tty->driver_data. Easily to happen as the test case
> shows, in case all opened /dev/pts/N references are closed before "/dev/tty".


Yeah, I agree with your analysis here. Nothing bumps the slave inode reference for
which the current tty is a proxy.

So if the current tty is the last released, that means all the master and slave
inodes have been released. So the inode still in tty->driver_data for the slave is
stale.


> I also expect in a rare case where all ptmx references are gone/closed, this also
> could happen on final close when the master tty is given to pty_unix98_shutdown.

This logic I'm not following. If the pty master is being released, then the inode
is valid for the release() operation in-progress.

Regards,
Peter Hurley


> Following this is a proposed fix to the problem, I tested here and it fixed the
> issue.
> 
> Please take a look and review, thanks!

Looking at it now.


Regards,
Peter Hurley

Re: pty: fix use after free/oops at pty_unix98_shutdown

[Herton R. Krzesinski]

On Tue, Dec 15, 2015 at 08:17:56AM -0800, Peter Hurley wrote:
> > I also expect in a rare case where all ptmx references are gone/closed, this also
> > could happen on final close when the master tty is given to pty_unix98_shutdown.
> 
> This logic I'm not following. If the pty master is being released, then the inode
> is valid for the release() operation in-progress.

Hi Peter,

yes, you're right if you are eg. closing the /dev/ptmx or /dev/pts/ptmx file
previously opened. But I thought and refer above to the case where for example
you are closing /dev/tty and that's the final close and there is no other
process in the system with the /dev/{,*/}ptmx opened, the inode which referenced
the previously opened ptmx could be gone. It would be rare though since in a
running system any logged in user eg. through ssh or with a terminal open in X
will have at least a ptmx device opened.

> 
> Regards,
> Peter Hurley
>

-- 
[]'s
Herton 

Re: pty: fix use after free/oops at pty_unix98_shutdown

[Peter Hurley]

On 12/15/2015 08:36 AM, Herton R. Krzesinski wrote:
> On Tue, Dec 15, 2015 at 08:17:56AM -0800, Peter Hurley wrote:
>>> I also expect in a rare case where all ptmx references are gone/closed, this also
>>> could happen on final close when the master tty is given to pty_unix98_shutdown.
>>
>> This logic I'm not following. If the pty master is being released, then the inode
>> is valid for the release() operation in-progress.
> 
> Hi Peter,
> 
> yes, you're right if you are eg. closing the /dev/ptmx or /dev/pts/ptmx file
> previously opened. But I thought and refer above to the case where for example
> you are closing /dev/tty and that's the final close and there is no other
> process in the system with the /dev/{,*/}ptmx opened, the inode which referenced
> the previously opened ptmx could be gone. It would be rare though since in a
> running system any logged in user eg. through ssh or with a terminal open in X
> will have at least a ptmx device opened.

/dev/tty can never be an alias for /dev/ptmx in Linux: a master pty cannot be
a controlling terminal. So if the master pty is being released it will always be
with the /dev/ptmx inode.

Regards,
Peter Hurley

PS - for the purpose of this discussion, /dev/pts/ptmx is equivalent.

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

[Peter Hurley]

Hi Herton,

On 12/14/2015 07:29 PM, Herton R. Krzesinski wrote:
> pty_unix98_shutdown allows a potential use after free of inode from
> slave tty->driver_data: if final pty close is called with slave
> tty_struct, and inode was released already by devpts_pty_kill at
> pty_close, pty_unix98_shutdown will access stale data.

I'm not following this logic.

Suppose there is no open current tty alias for the slave pty; then
there is one inode reference for the master and N + 1 inode references
for however many times the slave has been opened.

If the master is closed first, the slave dentry is dropped so there
are now N slave inode references. When the last slave closes, the
slave inode is still valid when devpts_kill_index() is called.

So afaict, this problem only applies when /dev/tty is the final
close, and at no other time. [Not to minimize the scale of the problem:
quite often, /dev/tty would be the last file closed.]


> If the evicted inode is quickly reused again as another inode
> instance, this can potentially break in the case the inode is on a
> different devpts instance than the default devpts mount, not to
> mention a possible ops if the inode_cache slab is destroyed and
> reused as something else.
> 
> Also there is an evident problem in case the last close is from a
> opened "/dev/tty" which points to the master/slave pty:

/dev/tty can only refer to the slave pty.


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


> Signed-off-by: Herton R. Krzesinski <herton@redhat.com>
> Cc: <stable@vger.kernel.org>

Afaict, the stable tag goes back to the original implementation.
Did you research how far back the /dev/tty alias problem goes?

Regards,
Peter Hurley

> ---
>  drivers/tty/pty.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> index a45660f..90743b0 100644
> --- a/drivers/tty/pty.c
> +++ b/drivers/tty/pty.c
> @@ -681,7 +681,14 @@ 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);
> +	struct inode *ptmx_inode;
> +
> +	if (tty->driver->subtype == PTY_TYPE_MASTER)
> +		ptmx_inode = tty->driver_data;
> +	else
> +		ptmx_inode = tty->link->driver_data;
> +	devpts_kill_index(ptmx_inode, tty->index);
> +	iput(ptmx_inode); /* drop reference we acquired at ptmx_open */
>  }
>  
>  static const struct tty_operations ptm_unix98_ops = {
> @@ -773,6 +780,15 @@ static int ptmx_open(struct inode *inode, struct file *filp)
>  	set_bit(TTY_PTY_LOCK, &tty->flags); /* LOCK THE SLAVE */
>  	tty->driver_data = inode;
>  
> +	/*
> +	 * In the rare case all references to ptmx inode are dropped (files
> +	 * closed), and we still have a device opened pointing to the
> +	 * master/slave pair (eg., "/dev/tty" opened), we must make sure that
> +	 * the inode is still valid when we call the final pty_unix98_shutdown:
> +	 * thus we must hold an additional reference to the ptmx inode here
> +	 */
> +	ihold(inode);
> +
>  	tty_add_file(tty, filp);
>  
>  	slave_inode = devpts_pty_new(inode,
> 

Re: pty: fix use after free/oops at pty_unix98_shutdown

[Herton R. Krzesinski]

On Tue, Dec 15, 2015 at 09:28:28AM -0800, Peter Hurley wrote:
> On 12/15/2015 08:36 AM, Herton R. Krzesinski wrote:
> > On Tue, Dec 15, 2015 at 08:17:56AM -0800, Peter Hurley wrote:
> >>> I also expect in a rare case where all ptmx references are gone/closed, this also
> >>> could happen on final close when the master tty is given to pty_unix98_shutdown.
> >>
> >> This logic I'm not following. If the pty master is being released, then the inode
> >> is valid for the release() operation in-progress.
> > 
> > Hi Peter,
> > 
> > yes, you're right if you are eg. closing the /dev/ptmx or /dev/pts/ptmx file
> > previously opened. But I thought and refer above to the case where for example
> > you are closing /dev/tty and that's the final close and there is no other
> > process in the system with the /dev/{,*/}ptmx opened, the inode which referenced
> > the previously opened ptmx could be gone. It would be rare though since in a
> > running system any logged in user eg. through ssh or with a terminal open in X
> > will have at least a ptmx device opened.
> 
> /dev/tty can never be an alias for /dev/ptmx in Linux: a master pty cannot be
> a controlling terminal. So if the master pty is being released it will always be
> with the /dev/ptmx inode.

Indeed, pty_unix98_shutdown in case of /dev/tty close will be called with slave
pty tty_struct. Sorry about my previous confusing/wrong statement. My concern is
only valid in case final /dev/tty close used the ptmx inode instead of slave_inode
created at ptmx_open, which was not the case before/with current code, but is
the case when applying my patch, thus I grab the reference to the inode.

> 
> Regards,
> Peter Hurley
> 
> PS - for the purpose of this discussion, /dev/pts/ptmx is equivalent.

-- 
[]'s
Herton

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

[Herton R. Krzesinski]

On Tue, Dec 15, 2015 at 09:36:26AM -0800, Peter Hurley wrote:
> Hi Herton,
> 
> On 12/14/2015 07:29 PM, Herton R. Krzesinski wrote:
> > pty_unix98_shutdown allows a potential use after free of inode from
> > slave tty->driver_data: if final pty close is called with slave
> > tty_struct, and inode was released already by devpts_pty_kill at
> > pty_close, pty_unix98_shutdown will access stale data.
> 
> I'm not following this logic.
> 
> Suppose there is no open current tty alias for the slave pty; then
> there is one inode reference for the master and N + 1 inode references
> for however many times the slave has been opened.
> 
> If the master is closed first, the slave dentry is dropped so there
> are now N slave inode references. When the last slave closes, the
> slave inode is still valid when devpts_kill_index() is called.
> 
> So afaict, this problem only applies when /dev/tty is the final
> close, and at no other time. [Not to minimize the scale of the problem:
> quite often, /dev/tty would be the last file closed.]

Doh, you are right. The changelog is wrong here, and I must remove as what
I wrote isn't possible. The problem is only related to /dev/tty.

> 
> 
> > If the evicted inode is quickly reused again as another inode
> > instance, this can potentially break in the case the inode is on a
> > different devpts instance than the default devpts mount, not to
> > mention a possible ops if the inode_cache slab is destroyed and
> > reused as something else.
> > 
> > Also there is an evident problem in case the last close is from a
> > opened "/dev/tty" which points to the master/slave pty:
> 
> /dev/tty can only refer to the slave pty.

yes :(

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

> 
> 
> > Signed-off-by: Herton R. Krzesinski <herton@redhat.com>
> > Cc: <stable@vger.kernel.org>
> 
> Afaict, the stable tag goes back to the original implementation.
> Did you research how far back the /dev/tty alias problem goes?

Hmm no. I did cc stable because the first report I got about this issue
was on RHEL 7 with 3.10 based kernel, so this issue goes far back
some releases that are still supported and similar code is there.

On a quick check on a 2.6.32 kernel, things were very different,
tty_release_dev() called directly devpts_kill_index with inode
from the same file being closed. I'll check more and adjust the tag.

> 
> Regards,
> Peter Hurley
> 
> > ---
> >  drivers/tty/pty.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> > index a45660f..90743b0 100644
> > --- a/drivers/tty/pty.c
> > +++ b/drivers/tty/pty.c
> > @@ -681,7 +681,14 @@ 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);
> > +	struct inode *ptmx_inode;
> > +
> > +	if (tty->driver->subtype == PTY_TYPE_MASTER)
> > +		ptmx_inode = tty->driver_data;
> > +	else
> > +		ptmx_inode = tty->link->driver_data;
> > +	devpts_kill_index(ptmx_inode, tty->index);
> > +	iput(ptmx_inode); /* drop reference we acquired at ptmx_open */
> >  }
> >  
> >  static const struct tty_operations ptm_unix98_ops = {
> > @@ -773,6 +780,15 @@ static int ptmx_open(struct inode *inode, struct file *filp)
> >  	set_bit(TTY_PTY_LOCK, &tty->flags); /* LOCK THE SLAVE */
> >  	tty->driver_data = inode;
> >  
> > +	/*
> > +	 * In the rare case all references to ptmx inode are dropped (files
> > +	 * closed), and we still have a device opened pointing to the
> > +	 * master/slave pair (eg., "/dev/tty" opened), we must make sure that
> > +	 * the inode is still valid when we call the final pty_unix98_shutdown:
> > +	 * thus we must hold an additional reference to the ptmx inode here
> > +	 */
> > +	ihold(inode);
> > +
> >  	tty_add_file(tty, filp);
> >  
> >  	slave_inode = devpts_pty_new(inode,
> > 
> 

-- 
[]'s
Herton

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

[Herton R. Krzesinski]

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:
> > 
> > 
> > > Signed-off-by: Herton R. Krzesinski <herton@redhat.com>
> > > Cc: <stable@vger.kernel.org>
> > 
> > Afaict, the stable tag goes back to the original implementation.
> > Did you research how far back the /dev/tty alias problem goes?
> 
> Hmm no. I did cc stable because the first report I got about this issue
> was on RHEL 7 with 3.10 based kernel, so this issue goes far back
> some releases that are still supported and similar code is there.
> 
> On a quick check on a 2.6.32 kernel, things were very different,
> tty_release_dev() called directly devpts_kill_index with inode
> from the same file being closed. I'll check more and adjust the tag.

FYI, checked here and the problem should start with 3.8, after commit
fa2ecfc5a68d85624bbd84f7d010860776b7e602 devpts_kill_index was moved
to pty.c/pty_unix98_shutdown

-- 
[]'s
Herton

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

[Peter Hurley]

On 12/15/2015 11:23 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:
>>>
>>>
>>>> Signed-off-by: Herton R. Krzesinski <herton@redhat.com>
>>>> Cc: <stable@vger.kernel.org>
>>>
>>> Afaict, the stable tag goes back to the original implementation.
>>> Did you research how far back the /dev/tty alias problem goes?
>>
>> Hmm no. I did cc stable because the first report I got about this issue
>> was on RHEL 7 with 3.10 based kernel, so this issue goes far back
>> some releases that are still supported and similar code is there.
>>
>> On a quick check on a 2.6.32 kernel, things were very different,
>> tty_release_dev() called directly devpts_kill_index with inode
>> from the same file being closed. I'll check more and adjust the tag.
> 
> FYI, checked here and the problem should start with 3.8, after commit
> fa2ecfc5a68d85624bbd84f7d010860776b7e602 devpts_kill_index was moved
> to pty.c/pty_unix98_shutdown
> 

istm this goes back to multi-instance devpts support added in 2.6.28.

Before then, there was no inode parameter because there was only
one devpts instance and the idas were global.

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

[Herton R. Krzesinski]

On Tue, Dec 15, 2015 at 11:52:14AM -0800, Peter Hurley wrote:
> On 12/15/2015 11:23 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:
> >>>
> >>>
> >>>> Signed-off-by: Herton R. Krzesinski <herton@redhat.com>
> >>>> Cc: <stable@vger.kernel.org>
> >>>
> >>> Afaict, the stable tag goes back to the original implementation.
> >>> Did you research how far back the /dev/tty alias problem goes?
> >>
> >> Hmm no. I did cc stable because the first report I got about this issue
> >> was on RHEL 7 with 3.10 based kernel, so this issue goes far back
> >> some releases that are still supported and similar code is there.
> >>
> >> On a quick check on a 2.6.32 kernel, things were very different,
> >> tty_release_dev() called directly devpts_kill_index with inode
> >> from the same file being closed. I'll check more and adjust the tag.
> > 
> > FYI, checked here and the problem should start with 3.8, after commit
> > fa2ecfc5a68d85624bbd84f7d010860776b7e602 devpts_kill_index was moved
> > to pty.c/pty_unix98_shutdown
> > 
> 
> istm this goes back to multi-instance devpts support added in 2.6.28.
> 
> Before then, there was no inode parameter because there was only
> one devpts instance and the idas were global.

Yeah, I'm not ruling out problems with devpts instances prior to 3.8, where to
me the wrong inode will be given in the final close with /dev/tty case, when the
ptmx is on a different instance other than the main ptmx instance (
pts_sb_from_inode will choose the "root"/main devpts instance, as the /dev/tty
inode usually is inode tied to devtmpfs mount at /dev). Both fa2ecfc5a68d85624b
and the new fix could be backported to 3.7 and as far as 2.6.28 perhaps, not
sure if anything else will be needed, however may not be worth the trouble.

-- 
[]'s
Herton

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

[Peter Hurley]

On 12/15/2015 12:34 PM, Herton R. Krzesinski wrote:
> On Tue, Dec 15, 2015 at 11:52:14AM -0800, Peter Hurley wrote:
>> On 12/15/2015 11:23 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:
>>>>>
>>>>>
>>>>>> Signed-off-by: Herton R. Krzesinski <herton@redhat.com>
>>>>>> Cc: <stable@vger.kernel.org>
>>>>>
>>>>> Afaict, the stable tag goes back to the original implementation.
>>>>> Did you research how far back the /dev/tty alias problem goes?
>>>>
>>>> Hmm no. I did cc stable because the first report I got about this issue
>>>> was on RHEL 7 with 3.10 based kernel, so this issue goes far back
>>>> some releases that are still supported and similar code is there.
>>>>
>>>> On a quick check on a 2.6.32 kernel, things were very different,
>>>> tty_release_dev() called directly devpts_kill_index with inode
>>>> from the same file being closed. I'll check more and adjust the tag.
>>>
>>> FYI, checked here and the problem should start with 3.8, after commit
>>> fa2ecfc5a68d85624bbd84f7d010860776b7e602 devpts_kill_index was moved
>>> to pty.c/pty_unix98_shutdown
>>>
>>
>> istm this goes back to multi-instance devpts support added in 2.6.28.
>>
>> Before then, there was no inode parameter because there was only
>> one devpts instance and the idas were global.
> 
> Yeah, I'm not ruling out problems with devpts instances prior to 3.8, where to
> me the wrong inode will be given in the final close with /dev/tty case, when the
> ptmx is on a different instance other than the main ptmx instance (
> pts_sb_from_inode will choose the "root"/main devpts instance, as the /dev/tty
> inode usually is inode tied to devtmpfs mount at /dev). Both fa2ecfc5a68d85624b
> and the new fix could be backported to 3.7 and as far as 2.6.28 perhaps, not
> sure if anything else will be needed, however may not be worth the trouble.

I think a 2.6.28 tag is sufficient.

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

[Herton R. Krzesinski]

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

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

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

[Herton R. Krzesinski]

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.