* Question on release_one_tty
@ 2014-08-07 8:25 Cyrill Gorcunov
2014-08-07 8:28 ` Pavel Emelyanov
0 siblings, 1 reply; 7+ messages in thread
From: Cyrill Gorcunov @ 2014-08-07 8:25 UTC (permalink / raw)
To: LKML, Jiri Slaby, Greg Kroah-Hartman; +Cc: Pavel Emelyanov
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: Question on release_one_tty
2014-08-07 8:25 Question on release_one_tty Cyrill Gorcunov
@ 2014-08-07 8:28 ` Pavel Emelyanov
2014-08-07 8:34 ` Cyrill Gorcunov
0 siblings, 1 reply; 7+ messages in thread
From: Pavel Emelyanov @ 2014-08-07 8:28 UTC (permalink / raw)
To: Cyrill Gorcunov, LKML, Jiri Slaby, Greg Kroah-Hartman
On 08/07/2014 12:25 PM, Cyrill Gorcunov wrote:
> 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?
If you put the module it can be unloaded at any time killing the code that would
be potentially required by kref_put.
> Cyrill
> .
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: Question on release_one_tty
2014-08-07 8:28 ` Pavel Emelyanov
@ 2014-08-07 8:34 ` Cyrill Gorcunov
2014-08-07 9:05 ` Pavel Emelyanov
0 siblings, 1 reply; 7+ messages in thread
From: Cyrill Gorcunov @ 2014-08-07 8:34 UTC (permalink / raw)
To: Pavel Emelyanov; +Cc: LKML, Jiri Slaby, Greg Kroah-Hartman
On Thu, Aug 07, 2014 at 12:28:58PM +0400, Pavel Emelyanov wrote:
> On 08/07/2014 12:25 PM, Cyrill Gorcunov wrote:
> > 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?
>
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: Question on release_one_tty
2014-08-07 8:34 ` Cyrill Gorcunov
@ 2014-08-07 9:05 ` Pavel Emelyanov
2014-08-07 9:18 ` Cyrill Gorcunov
0 siblings, 1 reply; 7+ messages in thread
From: Pavel Emelyanov @ 2014-08-07 9:05 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: LKML, Jiri Slaby, Greg Kroah-Hartman
On 08/07/2014 12:34 PM, Cyrill Gorcunov wrote:
> On Thu, Aug 07, 2014 at 12:28:58PM +0400, Pavel Emelyanov wrote:
>> On 08/07/2014 12:25 PM, Cyrill Gorcunov wrote:
>>> 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?
>>
>> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: Question on release_one_tty
2014-08-07 9:05 ` Pavel Emelyanov
@ 2014-08-07 9:18 ` Cyrill Gorcunov
2014-08-07 9:34 ` [PATCH] tty: Fix potential use after free in release_one_tty Cyrill Gorcunov
0 siblings, 1 reply; 7+ messages in thread
From: Cyrill Gorcunov @ 2014-08-07 9:18 UTC (permalink / raw)
To: Pavel Emelyanov; +Cc: LKML, Jiri Slaby, Greg Kroah-Hartman
On Thu, Aug 07, 2014 at 01:05:28PM +0400, Pavel Emelyanov wrote:
> >
> > 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.
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH] tty: Fix potential use after free in release_one_tty
2014-08-07 9:18 ` Cyrill Gorcunov
@ 2014-08-07 9:34 ` Cyrill Gorcunov
2014-08-07 20:22 ` Greg Kroah-Hartman
0 siblings, 1 reply; 7+ messages in thread
From: Cyrill Gorcunov @ 2014-08-07 9:34 UTC (permalink / raw)
To: Pavel Emelyanov, LKML, Jiri Slaby, Greg Kroah-Hartman
On Thu, Aug 07, 2014 at 01:18:12PM +0400, Cyrill Gorcunov wrote:
> > 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 <gorcunov@gmail.com>
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 <xemul@parallels.com>
CC: Jiri Slaby <jslaby@suse.cz>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
drivers/tty/tty_io.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-2.6.git/drivers/tty/tty_io.c
===================================================================
--- linux-2.6.git.orig/drivers/tty/tty_io.c
+++ 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);
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] tty: Fix potential use after free in release_one_tty
2014-08-07 9:34 ` [PATCH] tty: Fix potential use after free in release_one_tty Cyrill Gorcunov
@ 2014-08-07 20:22 ` Greg Kroah-Hartman
0 siblings, 0 replies; 7+ messages in thread
From: Greg Kroah-Hartman @ 2014-08-07 20:22 UTC (permalink / raw)
To: Cyrill Gorcunov; +Cc: Pavel Emelyanov, LKML, Jiri Slaby
On Thu, Aug 07, 2014 at 01:34:28PM +0400, Cyrill Gorcunov wrote:
> On Thu, Aug 07, 2014 at 01:18:12PM +0400, Cyrill Gorcunov wrote:
> > > 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 <gorcunov@gmail.com>
> 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 <xemul@parallels.com>
> CC: Jiri Slaby <jslaby@suse.cz>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> ---
> drivers/tty/tty_io.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> Index: linux-2.6.git/drivers/tty/tty_io.c
> ===================================================================
> --- linux-2.6.git.orig/drivers/tty/tty_io.c
> +++ 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);
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-08-07 20:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-07 8:25 Question on release_one_tty Cyrill Gorcunov
2014-08-07 8:28 ` Pavel Emelyanov
2014-08-07 8:34 ` Cyrill Gorcunov
2014-08-07 9:05 ` Pavel Emelyanov
2014-08-07 9:18 ` Cyrill Gorcunov
2014-08-07 9:34 ` [PATCH] tty: Fix potential use after free in release_one_tty Cyrill Gorcunov
2014-08-07 20:22 ` Greg Kroah-Hartman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox