Question on release_one_tty

PATCH Superseded Cyrill Gorcunov 3 participants 7 messages Aug 7, 2014 TTY layer and serial drivers

lore

Share & tools

lore

Cyrill Gorcunov

author

2014-08-07 08:25 UTC #0 lore raw

Hi guys, could you please explain me the sequence

static void release_one_tty(struct work_struct *work)

{
	struct tty_struct *tty =
		container_of(work, struct tty_struct, hangup_work);
	struct tty_driver *driver = tty->driver;

	if (tty->ops->cleanup)
		tty->ops->cleanup(tty);

	tty->magic = 0;

--> tty_driver_kref_put(driver);
--> module_put(driver->owner);

why tty_driver_kref_put is called before module_put? As far as I understand tty_driver_kref_put may call the destruct_tty_driver which eventually does

static void destruct_tty_driver(struct kref *kref)

{
	struct tty_driver *driver = container_of(kref, struct tty_driver, kref);
	...
	kfree(driver->cdevs);
	kfree(driver->ports);
	kfree(driver->termios);
	kfree(driver->ttys);

--> kfree(driver);
}

so that the module_put(driver->owner) would access freed memory. Should not we call the reverse module_put and then tty_driver_kref_put, or I miss something obvious?

Cyrill

Pavel Emelyanov

2014-08-07 08:29 UTC #1 lore raw

Quoted 26 lines · Cyrill Gorcunov

Hi guys, could you please explain me the sequence
static void release_one_tty(struct work_struct *work)
{
	struct tty_struct *tty =
		container_of(work, struct tty_struct, hangup_work);
	struct tty_driver *driver = tty->driver;

	if (tty->ops->cleanup)
		tty->ops->cleanup(tty);

	tty->magic = 0;
--> tty_driver_kref_put(driver);
--> module_put(driver->owner);
why tty_driver_kref_put is called before module_put? As far as I understand tty_driver_kref_put may call the destruct_tty_driver which eventually does
static void destruct_tty_driver(struct kref *kref)
{
	struct tty_driver *driver = container_of(kref, struct tty_driver, kref);
	...
	kfree(driver->cdevs);
	kfree(driver->ports);
	kfree(driver->termios);
	kfree(driver->ttys);

Quoted 6 lines

--> kfree(driver);
}
so that the module_put(driver->owner) would access freed memory. Should not we call the reverse module_put and then tty_driver_kref_put, or I miss something obvious?

If you put the module it can be unloaded at any time killing the code that would be potentially required by kref_put.

Quoted 3 lines

Cyrill
.

Cyrill Gorcunov

author

2014-08-07 08:34 UTC #2 lore raw

Quoted 30 lines · Pavel Emelyanov

Quoted 29 lines · Cyrill Gorcunov
Hi guys, could you please explain me the sequence
static void release_one_tty(struct work_struct *work)
{
	struct tty_struct *tty =
		container_of(work, struct tty_struct, hangup_work);
	struct tty_driver *driver = tty->driver;

	if (tty->ops->cleanup)
		tty->ops->cleanup(tty);

	tty->magic = 0;
--> tty_driver_kref_put(driver);
--> module_put(driver->owner);
why tty_driver_kref_put is called before module_put? As far as I understand tty_driver_kref_put may call the destruct_tty_driver which eventually does
static void destruct_tty_driver(struct kref *kref)
{
	struct tty_driver *driver = container_of(kref, struct tty_driver, kref);
	...
	kfree(driver->cdevs);
	kfree(driver->ports);
	kfree(driver->termios);
	kfree(driver->ttys);
--> kfree(driver);
}

Quoted 6 lines

Quoted 3 lines
so that the module_put(driver->owner) would access freed memory. Should not we call the reverse module_put and then tty_driver_kref_put, or I miss something obvious?
If you put the module it can be unloaded at any time killing the code that would be potentially required by kref_put.

So how this code supposed to work then? I mean tty_driver_kref_put must never call for destruct_tty_driver, otherwise we're accessing freed memory.

Pavel Emelyanov

2014-08-07 09:05 UTC #3 lore raw

Quoted 35 lines · Cyrill Gorcunov

Quoted 34 lines · Pavel Emelyanov
Quoted 32 lines · Cyrill Gorcunov
Hi guys, could you please explain me the sequence
static void release_one_tty(struct work_struct *work)
{
	struct tty_struct *tty =
		container_of(work, struct tty_struct, hangup_work);
	struct tty_driver *driver = tty->driver;

	if (tty->ops->cleanup)
		tty->ops->cleanup(tty);

	tty->magic = 0;
--> tty_driver_kref_put(driver);
--> module_put(driver->owner);
why tty_driver_kref_put is called before module_put? As far as I understand tty_driver_kref_put may call the destruct_tty_driver which eventually does
static void destruct_tty_driver(struct kref *kref)
{
	struct tty_driver *driver = container_of(kref, struct tty_driver, kref);
	...
	kfree(driver->cdevs);
	kfree(driver->ports);
	kfree(driver->termios);
	kfree(driver->ttys);
--> kfree(driver);
}
so that the module_put(driver->owner) would access freed memory. Should not we call the reverse module_put and then tty_driver_kref_put, or I miss something obvious?

Quoted 5 lines

Quoted 2 lines
If you put the module it can be unloaded at any time killing the code that would be potentially required by kref_put.
So how this code supposed to work then? I mean tty_driver_kref_put must never call for destruct_tty_driver, otherwise we're accessing freed memory.

mod = driver->owner;
tty_driver_kref_put(driver);
module_put(mod);

Check the upstream whether the same issue exists there.

Cyrill Gorcunov

author

2014-08-07 09:18 UTC #4 lore raw

Quoted 9 lines · Pavel Emelyanov

Quoted 3 lines
So how this code supposed to work then? I mean tty_driver_kref_put must never call for destruct_tty_driver, otherwise we're accessing freed memory.
mod = driver->owner;
tty_driver_kref_put(driver);
module_put(mod);
Check the upstream whether the same issue exists there.

Same in tty.git

static void release_one_tty(struct work_struct *work)

{
	struct tty_struct *tty =
		container_of(work, struct tty_struct, hangup_work);
	struct tty_driver *driver = tty->driver;

	if (tty->ops->cleanup)
		tty->ops->cleanup(tty);

	tty->magic = 0;
	tty_driver_kref_put(driver);
	module_put(driver->owner);

	spin_lock(&tty_files_lock);
	list_del_init(&tty->tty_files);
	spin_unlock(&tty_files_lock);

	put_pid(tty->pgrp);
	put_pid(tty->session);
	free_tty_struct(tty);
}

so I think yes, the fix is needed. I'll prepare the patch and test/post for review.

Cyrill Gorcunov

author

2014-08-07 09:34 UTC #5 lore raw

PATCH 1/1 tty: Fix potential use after free in release_one_tty

Quoted 10 lines · Cyrill Gorcunov

Quoted 5 lines
mod = driver->owner;
tty_driver_kref_put(driver);
module_put(mod);
Check the upstream whether the same issue exists there.
Same in tty.git
static void release_one_tty(struct work_struct *work) {

---
From: Cyrill Gorcunov <[email protected]>
Subject: tty: Fix potential use after free in release_one_tty

In case if we're releasing the last tty reference the following call sequence is possible

tty_driver_kref_put
destruct_tty_driver

    kfree(driver);

where @driver is used in next module_put call, which leads to

 | [ 285.964007] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
 | [ 285.964007] Workqueue: events release_one_tty
 | [ 285.964007] task: ffff8800cc7ea5f0 ti: ffff8800cb800000 task.ti: ffff8800cb800000
 | [ 285.964007] RIP: 0010:[<ffffffff810aeaf5>] [<ffffffff810aeaf5>] module_put+0x24/0xf4
 | [ 285.964007] RSP: 0018:ffff8800cb801d48 EFLAGS: 00010213
 | [ 285.964007] RAX: ffff8800cb801fd8 RBX: ffff8800ca3429d0 RCX: ffff8800cb1db400
 | [ 285.964007] RDX: 0000000000000000 RSI: ffffffff817349c1 RDI: 0000000000000001
 | [ 285.964007] RBP: ffff8800cb801d60 R08: ffff8800cd632b40 R09: 0000000000000000
 | [ 285.964007] R10: 00000000ffffffff R11: ffff88011f40a000 R12: 6b6b6b6b6b6b6b6b
 | [ 285.964007] R13: ffff8800ca342520 R14: 0000000000000000 R15: ffff88011f5d8200
 | [ 285.964007] FS: 0000000000000000(0000) GS:ffff88011f400000(0000) knlGS:0000000000000000
 | [ 285.964007] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
 | [ 285.964007] CR2: 00007faf5229d090 CR3: 0000000001c0b000 CR4: 00000000000006f0
 | [ 285.964007] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 | [ 285.964007] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
 | [ 285.964007] Stack:
 | [ 285.964007] ffff8800ca3429d0 ffff8800ca342a30 ffff8800ca342520 ffff8800cb801d88
 | [ 285.964007] ffffffff8146554a ffff8800cc77cc78 ffff8800ca3429d0 ffff88011f5d3800
 | [ 285.964007] ffff8800cb801e08 ffffffff810683c1 ffffffff810682ff 0000000000000046
 | [ 285.964007] Call Trace:
 | [ 285.964007] [<ffffffff8146554a>] release_one_tty+0x54/0xa3
 | [ 285.964007] [<ffffffff810683c1>] process_one_work+0x223/0x404
 | [ 285.964007] [<ffffffff810682ff>] ? process_one_work+0x161/0x404
 | [ 285.964007] [<ffffffff81068971>] worker_thread+0x136/0x205
 | [ 285.964007] [<ffffffff8106883b>] ? rescuer_thread+0x26a/0x26a
 | [ 285.964007] [<ffffffff8106e5bf>] kthread+0xa2/0xaa
 | [ 285.964007] [<ffffffff810a4586>] ? trace_hardirqs_on_caller+0x16/0x1eb
 | [ 285.964007] [<ffffffff8106e51d>] ? __kthread_parkme+0x65/0x65
 | [ 285.964007] [<ffffffff8173f59c>] ret_from_fork+0x7c/0xb0
 | [ 285.964007] [<ffffffff8106e51d>] ? __kthread_parkme+0x65/0x65
 | [ 285.964007] Code: 09 00 5b 41 5c 5d c3 0f 1f 44 00 00 55 48 85 ff 48 89 e5 41 55 41 54 49 89 fc 53 0f 84 d3 00
 | 00 00 bf 01 00 00 00 e8 d0 a1 fc ff <49> 8b 84 24 50 02 00 00 65 48 ff 40 08 4c 8b 6d 08 0f 1f 44 00

so simply keep a local reference to the module owner and use it later.

(Note I hit the bug on custom kernel but I think vanilla one suffers
the same problem).

CC Pavel Emelyanov <[email protected]>

CC Jiri Slaby <[email protected]>

CC Greg Kroah-Hartman <[email protected]>

Signed-off-by Cyrill Gorcunov <[email protected]>

---
 drivers/tty/tty_io.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/tty/tty_io.c +2 -1

Index: linux-2.6.git/drivers/tty/tty_io.c
===================================================================
@@ -1559,13 +1559,14 @@ static void release_one_tty(struct work_
 	struct tty_struct *tty =
 		container_of(work, struct tty_struct, hangup_work);
 	struct tty_driver *driver = tty->driver;
+	struct module *owner = driver->owner;
 
 	if (tty->ops->cleanup)
 		tty->ops->cleanup(tty);
 
 	tty->magic = 0;
 	tty_driver_kref_put(driver);
-	module_put(driver->owner);
+	module_put(owner);
 
 	spin_lock(&tty_files_lock);
 	list_del_init(&tty->tty_files);

Greg Kroah-Hartman

maintainer

2014-08-07 20:22 UTC #6 lore raw

Re: [PATCH] tty: Fix potential use after free in release_one_tty

Quoted 81 lines · Cyrill Gorcunov

Quoted 10 lines · Cyrill Gorcunov

Quoted 5 lines
mod = driver->owner;
tty_driver_kref_put(driver);
module_put(mod);
Check the upstream whether the same issue exists there.
Same in tty.git
static void release_one_tty(struct work_struct *work) {

---
From: Cyrill Gorcunov <[email protected]>
Subject: tty: Fix potential use after free in release_one_tty

In case if we're releasing the last tty reference the following call sequence is possible

tty_driver_kref_put
destruct_tty_driver

    kfree(driver);

where @driver is used in next module_put call, which leads to

 | [ 285.964007] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
 | [ 285.964007] Workqueue: events release_one_tty
 | [ 285.964007] task: ffff8800cc7ea5f0 ti: ffff8800cb800000 task.ti: ffff8800cb800000
 | [ 285.964007] RIP: 0010:[<ffffffff810aeaf5>] [<ffffffff810aeaf5>] module_put+0x24/0xf4
 | [ 285.964007] RSP: 0018:ffff8800cb801d48 EFLAGS: 00010213
 | [ 285.964007] RAX: ffff8800cb801fd8 RBX: ffff8800ca3429d0 RCX: ffff8800cb1db400
 | [ 285.964007] RDX: 0000000000000000 RSI: ffffffff817349c1 RDI: 0000000000000001
 | [ 285.964007] RBP: ffff8800cb801d60 R08: ffff8800cd632b40 R09: 0000000000000000
 | [ 285.964007] R10: 00000000ffffffff R11: ffff88011f40a000 R12: 6b6b6b6b6b6b6b6b
 | [ 285.964007] R13: ffff8800ca342520 R14: 0000000000000000 R15: ffff88011f5d8200
 | [ 285.964007] FS: 0000000000000000(0000) GS:ffff88011f400000(0000) knlGS:0000000000000000
 | [ 285.964007] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
 | [ 285.964007] CR2: 00007faf5229d090 CR3: 0000000001c0b000 CR4: 00000000000006f0
 | [ 285.964007] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 | [ 285.964007] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
 | [ 285.964007] Stack:
 | [ 285.964007] ffff8800ca3429d0 ffff8800ca342a30 ffff8800ca342520 ffff8800cb801d88
 | [ 285.964007] ffffffff8146554a ffff8800cc77cc78 ffff8800ca3429d0 ffff88011f5d3800
 | [ 285.964007] ffff8800cb801e08 ffffffff810683c1 ffffffff810682ff 0000000000000046
 | [ 285.964007] Call Trace:
 | [ 285.964007] [<ffffffff8146554a>] release_one_tty+0x54/0xa3
 | [ 285.964007] [<ffffffff810683c1>] process_one_work+0x223/0x404
 | [ 285.964007] [<ffffffff810682ff>] ? process_one_work+0x161/0x404
 | [ 285.964007] [<ffffffff81068971>] worker_thread+0x136/0x205
 | [ 285.964007] [<ffffffff8106883b>] ? rescuer_thread+0x26a/0x26a
 | [ 285.964007] [<ffffffff8106e5bf>] kthread+0xa2/0xaa
 | [ 285.964007] [<ffffffff810a4586>] ? trace_hardirqs_on_caller+0x16/0x1eb
 | [ 285.964007] [<ffffffff8106e51d>] ? __kthread_parkme+0x65/0x65
 | [ 285.964007] [<ffffffff8173f59c>] ret_from_fork+0x7c/0xb0
 | [ 285.964007] [<ffffffff8106e51d>] ? __kthread_parkme+0x65/0x65
 | [ 285.964007] Code: 09 00 5b 41 5c 5d c3 0f 1f 44 00 00 55 48 85 ff 48 89 e5 41 55 41 54 49 89 fc 53 0f 84 d3 00
 | 00 00 bf 01 00 00 00 e8 d0 a1 fc ff <49> 8b 84 24 50 02 00 00 65 48 ff 40 08 4c 8b 6d 08 0f 1f 44 00

so simply keep a local reference to the module owner and use it later.

(Note I hit the bug on custom kernel but I think vanilla one suffers
the same problem).

CC Pavel Emelyanov <[email protected]>

CC Jiri Slaby <[email protected]>

CC Greg Kroah-Hartman <[email protected]>

Signed-off-by Cyrill Gorcunov <[email protected]>

---
 drivers/tty/tty_io.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6.git/drivers/tty/tty_io.c
===================================================================

drivers/tty/tty_io.c +1 -0

@@ -1559,13 +1559,14 @@ static void release_one_tty(struct work_
 	struct tty_struct *tty =
 		container_of(work, struct tty_struct, hangup_work);
 	struct tty_driver *driver = tty->driver;
+	struct module *owner = driver->owner;

drivers/tty/tty_io.c static void release_one_tty(struct work_ · line 1572

Quoted 10 lines

drivers/tty/tty_io.c +1 -1

 	if (tty->ops->cleanup)
 		tty->ops->cleanup(tty);
 
 	tty->magic = 0;
 	tty_driver_kref_put(driver);
-	module_put(driver->owner);
+	module_put(owner);
 
 	spin_lock(&tty_files_lock);
 	list_del_init(&tty->tty_files);

Looks good, thanks, can you resend it in a format I can apply it in (one that doesn't require me to edit it by hand)? Then I'll be glad to queue it up after 3.17-rc1 is out.

thanks,

greg k-h