use-after-free in sixpack_close

use-after-free in sixpack_close

[Dmitry Vyukov]

Hello,

The following program triggers use-after-free in sixpack_close:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include <syscall.h>
#include <string.h>
#include <stdint.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/ioctl.h>
#include <fcntl.h>

int main()
{
        int fd = open("/dev/ptmx", O_RDWR);
        int opt = 0x7;
        ioctl(fd, TIOCSETD, &opt);
        return 0;
}

==================================================================
BUG: KASAN: use-after-free in del_timer+0x116/0x130 at addr ffff88005b15f788
Write of size 8 by task a.out/6775
=============================================================================
BUG kmalloc-4096 (Not tainted): kasan: bad access detected
-----------------------------------------------------------------------------

INFO: Allocated in seq_buf_alloc+0x61/0x80 age=0 cpu=3 pid=6779
[<      none      >] ___slab_alloc+0x489/0x4e0 mm/slub.c:2468
[<      none      >] __slab_alloc+0x4c/0x90 mm/slub.c:2497
[<     inline     >] slab_alloc_node mm/slub.c:2560
[<     inline     >] slab_alloc mm/slub.c:2602
[<      none      >] __kmalloc+0x299/0x330 mm/slub.c:3562
[<     inline     >] kmalloc include/linux/slab.h:463
[<      none      >] seq_buf_alloc+0x61/0x80 fs/seq_file.c:39
[<      none      >] seq_read+0x9b1/0x11d0 fs/seq_file.c:210
[<      none      >] __vfs_read+0x113/0x460 fs/read_write.c:432
[<      none      >] vfs_read+0x106/0x310 fs/read_write.c:454
[<     inline     >] SYSC_read fs/read_write.c:569
[<      none      >] SyS_read+0x111/0x220 fs/read_write.c:562
[<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

INFO: Freed in kvfree+0x36/0x60 age=0 cpu=3 pid=6779
[<      none      >] __slab_free+0x1fc/0x320 mm/slub.c:2678
[<     inline     >] slab_free mm/slub.c:2833
[<      none      >] kfree+0x26a/0x290 mm/slub.c:3662
[<      none      >] kvfree+0x36/0x60 mm/util.c:324
[<     inline     >] seq_release fs/seq_file.c:368
[<      none      >] single_release+0x78/0xb0 fs/seq_file.c:605
[<      none      >] __fput+0x233/0x780 fs/file_table.c:208
[<      none      >] ____fput+0x15/0x20 fs/file_table.c:244
[<      none      >] task_work_run+0x16b/0x200 kernel/task_work.c:115
[<     inline     >] tracehook_notify_resume include/linux/tracehook.h:191
[<      none      >] exit_to_usermode_loop+0x180/0x1a0
arch/x86/entry/common.c:251
[<     inline     >] prepare_exit_to_usermode arch/x86/entry/common.c:282
[<      none      >] syscall_return_slowpath+0x19f/0x210
arch/x86/entry/common.c:344
[<      none      >] int_ret_from_sys_call+0x25/0x9f
arch/x86/entry/entry_64.S:281

INFO: Slab 0xffffea00016c5600 objects=7 used=5 fp=0xffff88005b15b588
flags=0x5fffc0000004080
INFO: Object 0xffff88005b15eb10 @offset=27408 fp=0xffff88005b15d938
CPU: 1 PID: 6775 Comm: a.out Tainted: G    B           4.4.0-rc5+ #165
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 00000000ffffffff ffff88005de679d8 ffffffff82899fbd ffff88003e806a00
 ffff88005b15eb10 ffff88005b158000 ffff88005de67a08 ffffffff816c56f4
 ffff88003e806a00 ffffea00016c5600 ffff88005b15eb10 ffff88005de67b58

Call Trace:
 [<     inline     >] __dump_stack lib/dump_stack.c:15
 [<ffffffff82899fbd>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
 [<ffffffff816c56f4>] print_trailer+0xf4/0x150 mm/slub.c:659
 [<ffffffff816cbeaf>] object_err+0x2f/0x40 mm/slub.c:689
 [<     inline     >] print_address_description mm/kasan/report.c:138
 [<ffffffff816ce86d>] kasan_report_error+0x25d/0x560 mm/kasan/report.c:251
 [<     inline     >] kasan_report mm/kasan/report.c:274
 [<ffffffff816cedae>] __asan_report_store8_noabort+0x3e/0x40
mm/kasan/report.c:300
 [<     inline     >] timer_stats_timer_clear_start_info
include/linux/timer.h:208
 [<ffffffff8144f126>] del_timer+0x116/0x130 kernel/time/timer.c:1028
 [<ffffffff839f0d9f>] sixpack_close+0xbf/0x140 drivers/net/hamradio/6pack.c:688
 [<ffffffff82be4239>] tty_ldisc_close.isra.1+0x99/0xe0
drivers/tty/tty_ldisc.c:471
 [<ffffffff82be44d2>] tty_ldisc_kill+0x42/0x190 drivers/tty/tty_ldisc.c:753
 [<ffffffff82be5a77>] tty_ldisc_release+0x117/0x260 drivers/tty/tty_ldisc.c:781
 [<ffffffff82bd03da>] tty_release+0xa7a/0xff0 drivers/tty/tty_io.c:1905
 [<ffffffff81716103>] __fput+0x233/0x780 fs/file_table.c:208
 [<ffffffff817166d5>] ____fput+0x15/0x20 fs/file_table.c:244
 [<ffffffff8134679b>] task_work_run+0x16b/0x200 kernel/task_work.c:115
 [<     inline     >] exit_task_work include/linux/task_work.h:21
 [<ffffffff812f4d3b>] do_exit+0x8bb/0x2b20 kernel/exit.c:750
 [<ffffffff812f7118>] do_group_exit+0x108/0x320 kernel/exit.c:880
 [<     inline     >] SYSC_exit_group kernel/exit.c:891
 [<ffffffff812f734d>] SyS_exit_group+0x1d/0x20 kernel/exit.c:889
 [<ffffffff85ccc3f6>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185
==================================================================

This report is then followed by a dozen of other use-after-free reports.

On commit edb42dc7bc0da0125ceacab810a553ce1f0cac8d (Dec 15).

Thank you

Re: use-after-free in sixpack_close

[One Thousand Gnomes]

> This report is then followed by a dozen of other use-after-free reports.
> 
> On commit edb42dc7bc0da0125ceacab810a553ce1f0cac8d (Dec 15).
> 
> Thank you

sixpack_close does unregister_netdev(sp->dev), which frees sp as sp is
actually allocated via alloc_netdev()

Then deletes two timers within sp

Then frees two buffers indexed off sp

The mkiss driver also appears to share the same bug and references
ax->rbuff/xbuff after they have been freed, and then writes to ax->tty.


Alan

Re: use-after-free in sixpack_close

[David Miller]

From: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
Date: Thu, 17 Dec 2015 11:41:04 +0000

>> This report is then followed by a dozen of other use-after-free reports.
>> 
>> On commit edb42dc7bc0da0125ceacab810a553ce1f0cac8d (Dec 15).
>> 
>> Thank you
> 
> sixpack_close does unregister_netdev(sp->dev), which frees sp as sp is
> actually allocated via alloc_netdev()
> 
> Then deletes two timers within sp
> 
> Then frees two buffers indexed off sp

This should fix it, the only thing I'm unsure of is if we should perhaps
also use del_timer_sync() here.  Anyone?

====================
[PATCH 1/2] 6pack: Fix use after free in sixpack_close().

Need to do the unregister_device() after all references to the driver
private have been done.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 drivers/net/hamradio/6pack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
index 7c4a415..218f3ab 100644
--- a/drivers/net/hamradio/6pack.c
+++ b/drivers/net/hamradio/6pack.c
@@ -683,14 +683,14 @@ static void sixpack_close(struct tty_struct *tty)
 	if (!atomic_dec_and_test(&sp->refcnt))
 		down(&sp->dead_sem);
 
-	unregister_netdev(sp->dev);
-
 	del_timer(&sp->tx_t);
 	del_timer(&sp->resync_t);
 
 	/* Free all 6pack frame buffers. */
 	kfree(sp->rbuff);
 	kfree(sp->xbuff);
+
+	unregister_netdev(sp->dev);
 }
 
 /* Perform I/O control on an active 6pack channel. */
-- 
2.4.1

Re: use-after-free in sixpack_close

[Ralf Baechle DL5RB]

On Thu, Dec 17, 2015 at 04:05:32PM -0500, David Miller wrote:

> This should fix it, the only thing I'm unsure of is if we should perhaps
> also use del_timer_sync() here.  Anyone?

I think so.

  Ralf

Re: use-after-free in sixpack_close

[One Thousand Gnomes]

On Thu, 17 Dec 2015 16:05:32 -0500 (EST)
David Miller <davem@davemloft.net> wrote:

> From: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
> Date: Thu, 17 Dec 2015 11:41:04 +0000
> 
> >> This report is then followed by a dozen of other use-after-free reports.
> >> 
> >> On commit edb42dc7bc0da0125ceacab810a553ce1f0cac8d (Dec 15).
> >> 
> >> Thank you
> > 
> > sixpack_close does unregister_netdev(sp->dev), which frees sp as sp is
> > actually allocated via alloc_netdev()
> > 
> > Then deletes two timers within sp
> > 
> > Then frees two buffers indexed off sp
> 
> This should fix it, the only thing I'm unsure of is if we should perhaps
> also use del_timer_sync() here.  Anyone?

I think you need to yes as the timers use the data you wil be freeing in
the unregister.

Also you are at the point the tty is closing so the net device may be
active. Don't you need to netif_stop_queue() or defer the buffer
kfrees until after the network device is unregistered so you don't pee
into free memory if you have a transmit occurring ?

Alan

Re: use-after-free in sixpack_close

[David Miller]

From: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
Date: Thu, 17 Dec 2015 23:47:39 +0000

> On Thu, 17 Dec 2015 16:05:32 -0500 (EST)
> David Miller <davem@davemloft.net> wrote:
> 
>> From: One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
>> Date: Thu, 17 Dec 2015 11:41:04 +0000
>> 
>> >> This report is then followed by a dozen of other use-after-free reports.
>> >> 
>> >> On commit edb42dc7bc0da0125ceacab810a553ce1f0cac8d (Dec 15).
>> >> 
>> >> Thank you
>> > 
>> > sixpack_close does unregister_netdev(sp->dev), which frees sp as sp is
>> > actually allocated via alloc_netdev()
>> > 
>> > Then deletes two timers within sp
>> > 
>> > Then frees two buffers indexed off sp
>> 
>> This should fix it, the only thing I'm unsure of is if we should perhaps
>> also use del_timer_sync() here.  Anyone?
> 
> I think you need to yes as the timers use the data you wil be freeing in
> the unregister.

Ok.

> Also you are at the point the tty is closing so the net device may be
> active. Don't you need to netif_stop_queue() or defer the buffer
> kfrees until after the network device is unregistered so you don't pee
> into free memory if you have a transmit occurring ?

I'm pretty sure that's what the semaphore down above this sequence is
accomplishing.  But if we do need the netif_stop_queue() let's do that
as a separate patch.

Here is the patch I am checking in:

====================
[PATCH] 6pack: Fix use after free in sixpack_close().

Need to do the unregister_device() after all references to the driver
private have been done.

Also we need to use del_timer_sync() for the timers so that we don't
have any asynchronous references after the unregister.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 drivers/net/hamradio/6pack.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
index 7c4a415..9f0b1c3 100644
--- a/drivers/net/hamradio/6pack.c
+++ b/drivers/net/hamradio/6pack.c
@@ -683,14 +683,14 @@ static void sixpack_close(struct tty_struct *tty)
 	if (!atomic_dec_and_test(&sp->refcnt))
 		down(&sp->dead_sem);
 
-	unregister_netdev(sp->dev);
-
-	del_timer(&sp->tx_t);
-	del_timer(&sp->resync_t);
+	del_timer_sync(&sp->tx_t);
+	del_timer_sync(&sp->resync_t);
 
 	/* Free all 6pack frame buffers. */
 	kfree(sp->rbuff);
 	kfree(sp->xbuff);
+
+	unregister_netdev(sp->dev);
 }
 
 /* Perform I/O control on an active 6pack channel. */
-- 
2.4.1

Re: use-after-free in sixpack_close

[One Thousand Gnomes]

> > Also you are at the point the tty is closing so the net device may be
> > active. Don't you need to netif_stop_queue() or defer the buffer
> > kfrees until after the network device is unregistered so you don't pee
> > into free memory if you have a transmit occurring ?
> 
> I'm pretty sure that's what the semaphore down above this sequence is
> accomplishing.  But if we do need the netif_stop_queue() let's do that
> as a separate patch.

Follow the code path for sp_xmit(). If sp_xmit is called it digs out sp
from the ndetdev, locks sp->lock and stops the queue then calls sp_encaps
which touches sp->xbuff.

So if one thread of execution hits sp_xmit and another closes the ldisc
at just the wrong moment then we have no protection.

Alan