Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1308488 > unrolled thread

sound: use-after-free in snd_timer_interrupt

Started byDmitry Vyukov <dvyukov@google.com>
First post2016-01-13 16:10 +0100
Last post2016-01-13 21:50 +0100
Articles 12 — 2 participants

Back to article view | Back to linux.kernel

Contents

  sound: use-after-free in snd_timer_interrupt Dmitry Vyukov <dvyukov@google.com> - 2016-01-13 16:10 +0100
    Re: sound: use-after-free in snd_timer_interrupt Takashi Iwai <tiwai@suse.de> - 2016-01-13 18:00 +0100
      Re: sound: use-after-free in snd_timer_interrupt Dmitry Vyukov <dvyukov@google.com> - 2016-01-13 19:40 +0100
        Re: sound: use-after-free in snd_timer_interrupt Takashi Iwai <tiwai@suse.de> - 2016-01-13 20:10 +0100
          Re: sound: use-after-free in snd_timer_interrupt Dmitry Vyukov <dvyukov@google.com> - 2016-01-13 20:40 +0100
            Re: sound: use-after-free in snd_timer_interrupt Dmitry Vyukov <dvyukov@google.com> - 2016-01-13 20:50 +0100
              Re: sound: use-after-free in snd_timer_interrupt Takashi Iwai <tiwai@suse.de> - 2016-01-13 21:40 +0100
                Re: sound: use-after-free in snd_timer_interrupt Dmitry Vyukov <dvyukov@google.com> - 2016-01-13 21:50 +0100
                  Re: sound: use-after-free in snd_timer_interrupt Takashi Iwai <tiwai@suse.de> - 2016-01-13 22:00 +0100
                    Re: sound: use-after-free in snd_timer_interrupt Takashi Iwai <tiwai@suse.de> - 2016-01-14 17:20 +0100
                      Re: sound: use-after-free in snd_timer_interrupt Dmitry Vyukov <dvyukov@google.com> - 2016-01-15 09:10 +0100
            Re: sound: use-after-free in snd_timer_interrupt Takashi Iwai <tiwai@suse.de> - 2016-01-13 21:50 +0100

#1308488 — sound: use-after-free in snd_timer_interrupt

FromDmitry Vyukov <dvyukov@google.com>
Date2016-01-13 16:10 +0100
Subjectsound: use-after-free in snd_timer_interrupt
Message-ID<qQvdF-1tl-43@gated-at.bofh.it>
Hello,

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

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include <unistd.h>
#include <sys/syscall.h>
#include <string.h>
#include <stdint.h>
#include <pthread.h>

long r[84];

void *thr(void *arg)
{
        switch ((long)arg) {
        case 0:
                r[0] = syscall(SYS_mmap, 0x20000000ul, 0xe000ul,
0x3ul, 0x32ul, 0xfffffffffffffffful, 0x0ul);
                break;
        case 1:
                memcpy((void*)0x20000990,
"\x2f\x64\x65\x76\x2f\x73\x6e\x64\x2f\x74\x69\x6d\x65\x72", 14);
                r[2] = syscall(SYS_open, 0x20000990ul, 0x1ul, 0x0ul, 0, 0, 0);
                break;
        case 2:
                *(uint32_t*)0x20000000 = (uint32_t)0x1;
                *(uint32_t*)0x20000004 = (uint32_t)0x7;
                *(uint32_t*)0x20000008 = (uint32_t)0x3;
                *(uint32_t*)0x2000000c = (uint32_t)0x0;
                *(uint32_t*)0x20000010 = (uint32_t)0x0;
                *(uint8_t*)0x20000014 = (uint8_t)0x0;
                *(uint8_t*)0x20000015 = (uint8_t)0x0;
                *(uint8_t*)0x20000016 = (uint8_t)0x0;
                *(uint8_t*)0x20000017 = (uint8_t)0x0;
                *(uint8_t*)0x20000018 = (uint8_t)0x0;
                *(uint8_t*)0x20000019 = (uint8_t)0x0;
                *(uint8_t*)0x2000001a = (uint8_t)0x0;
                *(uint8_t*)0x2000001b = (uint8_t)0x0;
                *(uint8_t*)0x2000001c = (uint8_t)0x0;
                *(uint8_t*)0x2000001d = (uint8_t)0x0;
                *(uint8_t*)0x2000001e = (uint8_t)0x0;
                *(uint8_t*)0x2000001f = (uint8_t)0x0;
                *(uint8_t*)0x20000020 = (uint8_t)0x0;
                *(uint8_t*)0x20000021 = (uint8_t)0x0;
                *(uint8_t*)0x20000022 = (uint8_t)0x0;
                *(uint8_t*)0x20000023 = (uint8_t)0x0;
                *(uint8_t*)0x20000024 = (uint8_t)0x0;
                *(uint8_t*)0x20000025 = (uint8_t)0x0;
                *(uint8_t*)0x20000026 = (uint8_t)0x0;
                *(uint8_t*)0x20000027 = (uint8_t)0x0;
                *(uint8_t*)0x20000028 = (uint8_t)0x0;
                *(uint8_t*)0x20000029 = (uint8_t)0x0;
                *(uint8_t*)0x2000002a = (uint8_t)0x0;
                *(uint8_t*)0x2000002b = (uint8_t)0x0;
                *(uint8_t*)0x2000002c = (uint8_t)0x0;
                *(uint8_t*)0x2000002d = (uint8_t)0x0;
                *(uint8_t*)0x2000002e = (uint8_t)0x0;
                *(uint8_t*)0x2000002f = (uint8_t)0x0;
                *(uint8_t*)0x20000030 = (uint8_t)0x0;
                *(uint8_t*)0x20000031 = (uint8_t)0x0;
                *(uint8_t*)0x20000032 = (uint8_t)0x0;
                *(uint8_t*)0x20000033 = (uint8_t)0x0;
                r[40] = syscall(SYS_ioctl, r[2], 0x40345410ul,
0x20000000ul, 0, 0, 0);
                break;
        case 3:
                r[41] = syscall(SYS_ioctl, r[2], 0x54a2ul, 0, 0, 0, 0);
                break;
        case 4:
                r[42] = syscall(SYS_mmap, 0x2000e000ul, 0x1000ul,
0x3ul, 0x32ul, 0xfffffffffffffffful, 0x0ul);
                break;
        case 5:
                *(uint32_t*)0x2000efcc = (uint32_t)0x7;
                *(uint32_t*)0x2000efd0 = (uint32_t)0x9;
                *(uint32_t*)0x2000efd4 = (uint32_t)0x4513;
                *(uint32_t*)0x2000efd8 = (uint32_t)0x9;
                *(uint32_t*)0x2000efdc = (uint32_t)0x5;
                *(uint8_t*)0x2000efe0 = (uint8_t)0x0;
                *(uint8_t*)0x2000efe1 = (uint8_t)0x0;
                *(uint8_t*)0x2000efe2 = (uint8_t)0x0;
                *(uint8_t*)0x2000efe3 = (uint8_t)0x0;
                *(uint8_t*)0x2000efe4 = (uint8_t)0x0;
                *(uint8_t*)0x2000efe5 = (uint8_t)0x0;
                *(uint8_t*)0x2000efe6 = (uint8_t)0x0;
                *(uint8_t*)0x2000efe7 = (uint8_t)0x0;
                *(uint8_t*)0x2000efe8 = (uint8_t)0x0;
                *(uint8_t*)0x2000efe9 = (uint8_t)0x0;
                *(uint8_t*)0x2000efea = (uint8_t)0x0;
                *(uint8_t*)0x2000efeb = (uint8_t)0x0;
                *(uint8_t*)0x2000efec = (uint8_t)0x0;
                *(uint8_t*)0x2000efed = (uint8_t)0x0;
                *(uint8_t*)0x2000efee = (uint8_t)0x0;
                *(uint8_t*)0x2000efef = (uint8_t)0x0;
                *(uint8_t*)0x2000eff0 = (uint8_t)0x0;
                *(uint8_t*)0x2000eff1 = (uint8_t)0x0;
                *(uint8_t*)0x2000eff2 = (uint8_t)0x0;
                *(uint8_t*)0x2000eff3 = (uint8_t)0x0;
                *(uint8_t*)0x2000eff4 = (uint8_t)0x0;
                *(uint8_t*)0x2000eff5 = (uint8_t)0x0;
                *(uint8_t*)0x2000eff6 = (uint8_t)0x0;
                *(uint8_t*)0x2000eff7 = (uint8_t)0x0;
                *(uint8_t*)0x2000eff8 = (uint8_t)0x0;
                *(uint8_t*)0x2000eff9 = (uint8_t)0x0;
                *(uint8_t*)0x2000effa = (uint8_t)0x0;
                *(uint8_t*)0x2000effb = (uint8_t)0x0;
                *(uint8_t*)0x2000effc = (uint8_t)0x0;
                *(uint8_t*)0x2000effd = (uint8_t)0x0;
                *(uint8_t*)0x2000effe = (uint8_t)0x0;
                *(uint8_t*)0x2000efff = (uint8_t)0x0;
                r[80] = syscall(SYS_ioctl, r[2], 0x40345410ul,
0x2000efccul, 0, 0, 0);
                break;
        case 6:
                r[81] = syscall(SYS_ioctl, r[2], 0x54a0ul, 0, 0, 0, 0);
                break;
        case 7:
                r[82] = syscall(SYS_mmap, 0x2000f000ul, 0x1000ul,
0x3ul, 0x32ul, 0xfffffffffffffffful, 0x0ul);
                break;
        case 8:
                r[83] = syscall(SYS_ioctl, r[2], 0x80e85411ul,
0x2000ffd4ul, 0, 0, 0);
                break;
        }
        return 0;
}

int main()
{
        long i;
        pthread_t th[9];

        memset(r, -1, sizeof(r));
        for (i = 0; i < 9; i++) {
                pthread_create(&th[i], 0, thr, (void*)i);
                usleep(10000);
        }
        for (i = 0; i < 9; i++) {
                pthread_create(&th[i], 0, thr, (void*)i);
                if (i%2==0)
                        usleep(10000);
        }
        usleep(100000);
        return 0;
}


==================================================================
BUG: KASAN: use-after-free in snd_timer_interrupt+0xb11/0xbf0 at addr
ffff88002fd230d8
Read of size 8 by task syz-executor/8301
=============================================================================
BUG kmalloc-256 (Not tainted): kasan: bad access detected
-----------------------------------------------------------------------------

INFO: Allocated in snd_timer_instance_new+0x52/0x3a0 age=2 cpu=1 pid=8283
[<      none      >] ___slab_alloc+0x486/0x4e0 mm/slub.c:2468
[<      none      >] __slab_alloc+0x66/0xc0 mm/slub.c:2497
[<     inline     >] slab_alloc_node mm/slub.c:2560
[<     inline     >] slab_alloc mm/slub.c:2602
[<      none      >] kmem_cache_alloc_trace+0x284/0x310 mm/slub.c:2619
[<     inline     >] kmalloc include/linux/slab.h:458
[<     inline     >] kzalloc include/linux/slab.h:602
[<      none      >] snd_timer_instance_new+0x52/0x3a0 sound/core/timer.c:105
[<      none      >] snd_timer_open+0x522/0xc90 sound/core/timer.c:286
[<     inline     >] snd_timer_user_tselect sound/core/timer.c:1527
[<      none      >] snd_timer_user_ioctl+0x89f/0x2540 sound/core/timer.c:1809
[<     inline     >] vfs_ioctl fs/ioctl.c:43
[<      none      >] do_vfs_ioctl+0x18c/0xfa0 fs/ioctl.c:674
[<     inline     >] SYSC_ioctl fs/ioctl.c:689
[<      none      >] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:680
[<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

INFO: Freed in snd_timer_close+0x354/0x5f0 age=1 cpu=1 pid=8283
[<      none      >] __slab_free+0x1fc/0x320 mm/slub.c:2678
[<     inline     >] slab_free mm/slub.c:2833
[<      none      >] kfree+0x2a8/0x2d0 mm/slub.c:3662
[<      none      >] snd_timer_close+0x354/0x5f0 sound/core/timer.c:364
[<     inline     >] snd_timer_user_tselect sound/core/timer.c:1517
[<      none      >] snd_timer_user_ioctl+0x784/0x2540 sound/core/timer.c:1809
[<     inline     >] vfs_ioctl fs/ioctl.c:43
[<      none      >] do_vfs_ioctl+0x18c/0xfa0 fs/ioctl.c:674
[<     inline     >] SYSC_ioctl fs/ioctl.c:689
[<      none      >] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:680
[<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

INFO: Slab 0xffffea0000bf4800 objects=22 used=2 fp=0xffff88002fd23058
flags=0x1fffc0000004080
INFO: Object 0xffff88002fd23058 @offset=12376 fp=0xffff88002fd227d0
CPU: 2 PID: 8301 Comm: syz-executor Tainted: G    B           4.4.0+ #240
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 00000000ffffffff ffff88006d607b08 ffffffff82926eed ffff88003e807000
 ffff88002fd23058 ffff88002fd20000 ffff88006d607b38 ffffffff81740ca4
 ffff88003e807000 ffffea0000bf4800 ffff88002fd23058 ffff88002fd230d8

Call Trace:
 [<ffffffff8174a1fe>] __asan_report_load8_noabort+0x3e/0x40
mm/kasan/report.c:295
 [<ffffffff84ebe841>] snd_timer_interrupt+0xb11/0xbf0 sound/core/timer.c:680
 [<ffffffff84ebe9dd>] snd_timer_s_function+0xbd/0x130 sound/core/timer.c:963
 [<ffffffff814b8c56>] call_timer_fn+0x176/0x550 kernel/time/timer.c:1178
 [<     inline     >] __run_timers kernel/time/timer.c:1254
 [<ffffffff814ba175>] run_timer_softirq+0x5c5/0x9f0 kernel/time/timer.c:1437
 [<ffffffff813606a8>] __do_softirq+0x268/0x920 kernel/softirq.c:273
 [<     inline     >] invoke_softirq kernel/softirq.c:350
 [<ffffffff813610ef>] irq_exit+0x18f/0x1d0 kernel/softirq.c:391
 [<     inline     >] exiting_irq ./arch/x86/include/asm/apic.h:659
 [<ffffffff8125157b>] smp_apic_timer_interrupt+0x7b/0xa0
arch/x86/kernel/apic/apic.c:932
 [<ffffffff86273dec>] apic_timer_interrupt+0x8c/0xa0
arch/x86/entry/entry_64.S:520
 <EOI>  [<ffffffff813ac59d>] ? alloc_pid+0x5d/0xc90 kernel/pid.c:306
 [<     inline     >] slab_alloc_node mm/slub.c:2560
 [<     inline     >] slab_alloc mm/slub.c:2602
 [<ffffffff817446e1>] kmem_cache_alloc+0x261/0x2e0 mm/slub.c:2607
 [<ffffffff813ac59d>] alloc_pid+0x5d/0xc90 kernel/pid.c:306
 [<ffffffff8134ca2e>] copy_process.part.35+0x374e/0x5770 kernel/fork.c:1463
 [<     inline     >] copy_process kernel/fork.c:1275
 [<ffffffff8134ed7c>] _do_fork+0x1bc/0xcb0 kernel/fork.c:1724
 [<     inline     >] SYSC_clone kernel/fork.c:1833
 [<ffffffff8134f947>] SyS_clone+0x37/0x50 kernel/fork.c:1827
 [<ffffffff86272ff6>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185
==================================================================

On commit 67990608c8b95d2b8ccc29932376ae73d5818727 (Jan 12).

[toc] | [next] | [standalone]

#1308623

FromTakashi Iwai <tiwai@suse.de>
Date2016-01-13 18:00 +0100
Message-ID<qQwW7-2vW-23@gated-at.bofh.it>
In reply to#1308488
On Wed, 13 Jan 2016 16:00:09 +0100,
Dmitry Vyukov wrote:
> 
> Hello,
> 
> The following program triggers use-after-free in snd_timer_interrupt:
> 
> // autogenerated by syzkaller (http://github.com/google/syzkaller)
> #include <unistd.h>
> #include <sys/syscall.h>
> #include <string.h>
> #include <stdint.h>
> #include <pthread.h>
> 
> long r[84];
> 
> void *thr(void *arg)
> {
>         switch ((long)arg) {
>         case 0:
>                 r[0] = syscall(SYS_mmap, 0x20000000ul, 0xe000ul,
> 0x3ul, 0x32ul, 0xfffffffffffffffful, 0x0ul);
>                 break;
>         case 1:
>                 memcpy((void*)0x20000990,
> "\x2f\x64\x65\x76\x2f\x73\x6e\x64\x2f\x74\x69\x6d\x65\x72", 14);
>                 r[2] = syscall(SYS_open, 0x20000990ul, 0x1ul, 0x0ul, 0, 0, 0);
>                 break;
>         case 2:
>                 *(uint32_t*)0x20000000 = (uint32_t)0x1;
>                 *(uint32_t*)0x20000004 = (uint32_t)0x7;
>                 *(uint32_t*)0x20000008 = (uint32_t)0x3;
>                 *(uint32_t*)0x2000000c = (uint32_t)0x0;
>                 *(uint32_t*)0x20000010 = (uint32_t)0x0;
>                 *(uint8_t*)0x20000014 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000015 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000016 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000017 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000018 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000019 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000001a = (uint8_t)0x0;
>                 *(uint8_t*)0x2000001b = (uint8_t)0x0;
>                 *(uint8_t*)0x2000001c = (uint8_t)0x0;
>                 *(uint8_t*)0x2000001d = (uint8_t)0x0;
>                 *(uint8_t*)0x2000001e = (uint8_t)0x0;
>                 *(uint8_t*)0x2000001f = (uint8_t)0x0;
>                 *(uint8_t*)0x20000020 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000021 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000022 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000023 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000024 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000025 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000026 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000027 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000028 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000029 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000002a = (uint8_t)0x0;
>                 *(uint8_t*)0x2000002b = (uint8_t)0x0;
>                 *(uint8_t*)0x2000002c = (uint8_t)0x0;
>                 *(uint8_t*)0x2000002d = (uint8_t)0x0;
>                 *(uint8_t*)0x2000002e = (uint8_t)0x0;
>                 *(uint8_t*)0x2000002f = (uint8_t)0x0;
>                 *(uint8_t*)0x20000030 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000031 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000032 = (uint8_t)0x0;
>                 *(uint8_t*)0x20000033 = (uint8_t)0x0;
>                 r[40] = syscall(SYS_ioctl, r[2], 0x40345410ul,
> 0x20000000ul, 0, 0, 0);
>                 break;
>         case 3:
>                 r[41] = syscall(SYS_ioctl, r[2], 0x54a2ul, 0, 0, 0, 0);
>                 break;
>         case 4:
>                 r[42] = syscall(SYS_mmap, 0x2000e000ul, 0x1000ul,
> 0x3ul, 0x32ul, 0xfffffffffffffffful, 0x0ul);
>                 break;
>         case 5:
>                 *(uint32_t*)0x2000efcc = (uint32_t)0x7;
>                 *(uint32_t*)0x2000efd0 = (uint32_t)0x9;
>                 *(uint32_t*)0x2000efd4 = (uint32_t)0x4513;
>                 *(uint32_t*)0x2000efd8 = (uint32_t)0x9;
>                 *(uint32_t*)0x2000efdc = (uint32_t)0x5;
>                 *(uint8_t*)0x2000efe0 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000efe1 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000efe2 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000efe3 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000efe4 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000efe5 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000efe6 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000efe7 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000efe8 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000efe9 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000efea = (uint8_t)0x0;
>                 *(uint8_t*)0x2000efeb = (uint8_t)0x0;
>                 *(uint8_t*)0x2000efec = (uint8_t)0x0;
>                 *(uint8_t*)0x2000efed = (uint8_t)0x0;
>                 *(uint8_t*)0x2000efee = (uint8_t)0x0;
>                 *(uint8_t*)0x2000efef = (uint8_t)0x0;
>                 *(uint8_t*)0x2000eff0 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000eff1 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000eff2 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000eff3 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000eff4 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000eff5 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000eff6 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000eff7 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000eff8 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000eff9 = (uint8_t)0x0;
>                 *(uint8_t*)0x2000effa = (uint8_t)0x0;
>                 *(uint8_t*)0x2000effb = (uint8_t)0x0;
>                 *(uint8_t*)0x2000effc = (uint8_t)0x0;
>                 *(uint8_t*)0x2000effd = (uint8_t)0x0;
>                 *(uint8_t*)0x2000effe = (uint8_t)0x0;
>                 *(uint8_t*)0x2000efff = (uint8_t)0x0;
>                 r[80] = syscall(SYS_ioctl, r[2], 0x40345410ul,
> 0x2000efccul, 0, 0, 0);
>                 break;
>         case 6:
>                 r[81] = syscall(SYS_ioctl, r[2], 0x54a0ul, 0, 0, 0, 0);
>                 break;
>         case 7:
>                 r[82] = syscall(SYS_mmap, 0x2000f000ul, 0x1000ul,
> 0x3ul, 0x32ul, 0xfffffffffffffffful, 0x0ul);
>                 break;
>         case 8:
>                 r[83] = syscall(SYS_ioctl, r[2], 0x80e85411ul,
> 0x2000ffd4ul, 0, 0, 0);
>                 break;
>         }
>         return 0;
> }
> 
> int main()
> {
>         long i;
>         pthread_t th[9];
> 
>         memset(r, -1, sizeof(r));
>         for (i = 0; i < 9; i++) {
>                 pthread_create(&th[i], 0, thr, (void*)i);
>                 usleep(10000);
>         }
>         for (i = 0; i < 9; i++) {
>                 pthread_create(&th[i], 0, thr, (void*)i);
>                 if (i%2==0)
>                         usleep(10000);
>         }
>         usleep(100000);
>         return 0;
> }
> 
> 
> ==================================================================
> BUG: KASAN: use-after-free in snd_timer_interrupt+0xb11/0xbf0 at addr
> ffff88002fd230d8
> Read of size 8 by task syz-executor/8301
> =============================================================================
> BUG kmalloc-256 (Not tainted): kasan: bad access detected
> -----------------------------------------------------------------------------
> 
> INFO: Allocated in snd_timer_instance_new+0x52/0x3a0 age=2 cpu=1 pid=8283
> [<      none      >] ___slab_alloc+0x486/0x4e0 mm/slub.c:2468
> [<      none      >] __slab_alloc+0x66/0xc0 mm/slub.c:2497
> [<     inline     >] slab_alloc_node mm/slub.c:2560
> [<     inline     >] slab_alloc mm/slub.c:2602
> [<      none      >] kmem_cache_alloc_trace+0x284/0x310 mm/slub.c:2619
> [<     inline     >] kmalloc include/linux/slab.h:458
> [<     inline     >] kzalloc include/linux/slab.h:602
> [<      none      >] snd_timer_instance_new+0x52/0x3a0 sound/core/timer.c:105
> [<      none      >] snd_timer_open+0x522/0xc90 sound/core/timer.c:286
> [<     inline     >] snd_timer_user_tselect sound/core/timer.c:1527
> [<      none      >] snd_timer_user_ioctl+0x89f/0x2540 sound/core/timer.c:1809
> [<     inline     >] vfs_ioctl fs/ioctl.c:43
> [<      none      >] do_vfs_ioctl+0x18c/0xfa0 fs/ioctl.c:674
> [<     inline     >] SYSC_ioctl fs/ioctl.c:689
> [<      none      >] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:680
> [<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
> arch/x86/entry/entry_64.S:185
> 
> INFO: Freed in snd_timer_close+0x354/0x5f0 age=1 cpu=1 pid=8283
> [<      none      >] __slab_free+0x1fc/0x320 mm/slub.c:2678
> [<     inline     >] slab_free mm/slub.c:2833
> [<      none      >] kfree+0x2a8/0x2d0 mm/slub.c:3662
> [<      none      >] snd_timer_close+0x354/0x5f0 sound/core/timer.c:364
> [<     inline     >] snd_timer_user_tselect sound/core/timer.c:1517
> [<      none      >] snd_timer_user_ioctl+0x784/0x2540 sound/core/timer.c:1809
> [<     inline     >] vfs_ioctl fs/ioctl.c:43
> [<      none      >] do_vfs_ioctl+0x18c/0xfa0 fs/ioctl.c:674
> [<     inline     >] SYSC_ioctl fs/ioctl.c:689
> [<      none      >] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:680
> [<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
> arch/x86/entry/entry_64.S:185
> 
> INFO: Slab 0xffffea0000bf4800 objects=22 used=2 fp=0xffff88002fd23058
> flags=0x1fffc0000004080
> INFO: Object 0xffff88002fd23058 @offset=12376 fp=0xffff88002fd227d0
> CPU: 2 PID: 8301 Comm: syz-executor Tainted: G    B           4.4.0+ #240
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>  00000000ffffffff ffff88006d607b08 ffffffff82926eed ffff88003e807000
>  ffff88002fd23058 ffff88002fd20000 ffff88006d607b38 ffffffff81740ca4
>  ffff88003e807000 ffffea0000bf4800 ffff88002fd23058 ffff88002fd230d8
> 
> Call Trace:
>  [<ffffffff8174a1fe>] __asan_report_load8_noabort+0x3e/0x40
> mm/kasan/report.c:295
>  [<ffffffff84ebe841>] snd_timer_interrupt+0xb11/0xbf0 sound/core/timer.c:680
>  [<ffffffff84ebe9dd>] snd_timer_s_function+0xbd/0x130 sound/core/timer.c:963
>  [<ffffffff814b8c56>] call_timer_fn+0x176/0x550 kernel/time/timer.c:1178
>  [<     inline     >] __run_timers kernel/time/timer.c:1254
>  [<ffffffff814ba175>] run_timer_softirq+0x5c5/0x9f0 kernel/time/timer.c:1437
>  [<ffffffff813606a8>] __do_softirq+0x268/0x920 kernel/softirq.c:273
>  [<     inline     >] invoke_softirq kernel/softirq.c:350
>  [<ffffffff813610ef>] irq_exit+0x18f/0x1d0 kernel/softirq.c:391
>  [<     inline     >] exiting_irq ./arch/x86/include/asm/apic.h:659
>  [<ffffffff8125157b>] smp_apic_timer_interrupt+0x7b/0xa0
> arch/x86/kernel/apic/apic.c:932
>  [<ffffffff86273dec>] apic_timer_interrupt+0x8c/0xa0
> arch/x86/entry/entry_64.S:520
>  <EOI>  [<ffffffff813ac59d>] ? alloc_pid+0x5d/0xc90 kernel/pid.c:306
>  [<     inline     >] slab_alloc_node mm/slub.c:2560
>  [<     inline     >] slab_alloc mm/slub.c:2602
>  [<ffffffff817446e1>] kmem_cache_alloc+0x261/0x2e0 mm/slub.c:2607
>  [<ffffffff813ac59d>] alloc_pid+0x5d/0xc90 kernel/pid.c:306
>  [<ffffffff8134ca2e>] copy_process.part.35+0x374e/0x5770 kernel/fork.c:1463
>  [<     inline     >] copy_process kernel/fork.c:1275
>  [<ffffffff8134ed7c>] _do_fork+0x1bc/0xcb0 kernel/fork.c:1724
>  [<     inline     >] SYSC_clone kernel/fork.c:1833
>  [<ffffffff8134f947>] SyS_clone+0x37/0x50 kernel/fork.c:1827
>  [<ffffffff86272ff6>] entry_SYSCALL_64_fastpath+0x16/0x7a
> arch/x86/entry/entry_64.S:185
> ==================================================================
> 
> On commit 67990608c8b95d2b8ccc29932376ae73d5818727 (Jan 12).

This and your other relevant reports seem pointing the race of timer
ioctls.  Although snd_timer_close() itself calls snd_timer_stop(),
there is no other protection against the concurrent execution.

If my guess is correct, a simplistic fix like below should work.  It
basically serializes the timer ioctl by using a new mutex (and
replacing the old tread_sem mutex).  They are no longtime blocking
calls, so this shouldn't be a big problem.  But certainly there can be
a less intrusive way to paper over this if this really matters.

In this case for timer.c, I'd leave the final decision rather to
Jaroslav.  Jaroslav, what do you think?


thanks,

Takashi

---
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: timer: Fix race among timer ioctls

ALSA timer ioctls have an open race and this may lead to a
use-after-free of timer instance object.  A simplistic fix is to make
each ioctl exclusive.  We have already tread_sem for controlling the
tread, and extend this as a global mutex to be applied to each ioctl.

The downside is, of course, the worse concurrency.  But these ioctls
aren't to be parallel accessible, in anyway, so it should be fine to
serialize there.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/timer.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/sound/core/timer.c b/sound/core/timer.c
index 31f40f03e5b7..b03a9e489286 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -73,7 +73,7 @@ struct snd_timer_user {
 	struct timespec tstamp;		/* trigger tstamp */
 	wait_queue_head_t qchange_sleep;
 	struct fasync_struct *fasync;
-	struct mutex tread_sem;
+	struct mutex ioctl_lock;
 };
 
 /* list of timers */
@@ -1253,7 +1253,7 @@ static int snd_timer_user_open(struct inode *inode, struct file *file)
 		return -ENOMEM;
 	spin_lock_init(&tu->qlock);
 	init_waitqueue_head(&tu->qchange_sleep);
-	mutex_init(&tu->tread_sem);
+	mutex_init(&tu->ioctl_lock);
 	tu->ticks = 1;
 	tu->queue_size = 128;
 	tu->queue = kmalloc(tu->queue_size * sizeof(struct snd_timer_read),
@@ -1273,8 +1273,10 @@ static int snd_timer_user_release(struct inode *inode, struct file *file)
 	if (file->private_data) {
 		tu = file->private_data;
 		file->private_data = NULL;
+		mutex_lock(&tu->ioctl_lock);
 		if (tu->timeri)
 			snd_timer_close(tu->timeri);
+		mutex_unlock(&tu->ioctl_lock);
 		kfree(tu->queue);
 		kfree(tu->tqueue);
 		kfree(tu);
@@ -1512,7 +1514,6 @@ static int snd_timer_user_tselect(struct file *file,
 	int err = 0;
 
 	tu = file->private_data;
-	mutex_lock(&tu->tread_sem);
 	if (tu->timeri) {
 		snd_timer_close(tu->timeri);
 		tu->timeri = NULL;
@@ -1556,7 +1557,6 @@ static int snd_timer_user_tselect(struct file *file,
 	}
 
       __err:
-      	mutex_unlock(&tu->tread_sem);
 	return err;
 }
 
@@ -1769,7 +1769,7 @@ enum {
 	SNDRV_TIMER_IOCTL_PAUSE_OLD = _IO('T', 0x23),
 };
 
-static long snd_timer_user_ioctl(struct file *file, unsigned int cmd,
+static long __snd_timer_user_ioctl(struct file *file, unsigned int cmd,
 				 unsigned long arg)
 {
 	struct snd_timer_user *tu;
@@ -1786,17 +1786,11 @@ static long snd_timer_user_ioctl(struct file *file, unsigned int cmd,
 	{
 		int xarg;
 
-		mutex_lock(&tu->tread_sem);
-		if (tu->timeri)	{	/* too late */
-			mutex_unlock(&tu->tread_sem);
+		if (tu->timeri)	/* too late */
 			return -EBUSY;
-		}
-		if (get_user(xarg, p)) {
-			mutex_unlock(&tu->tread_sem);
+		if (get_user(xarg, p))
 			return -EFAULT;
-		}
 		tu->tread = xarg ? 1 : 0;
-		mutex_unlock(&tu->tread_sem);
 		return 0;
 	}
 	case SNDRV_TIMER_IOCTL_GINFO:
@@ -1829,6 +1823,18 @@ static long snd_timer_user_ioctl(struct file *file, unsigned int cmd,
 	return -ENOTTY;
 }
 
+static long snd_timer_user_ioctl(struct file *file, unsigned int cmd,
+				 unsigned long arg)
+{
+	struct snd_timer_user *tu = file->private_data;
+	long ret;
+
+	mutex_lock(&tu->ioctl_lock);
+	ret = __snd_timer_user_ioctl(file, cmd, arg);
+	mutex_unlock(&tu->ioctl_lock);
+	return ret;
+}
+
 static int snd_timer_user_fasync(int fd, struct file * file, int on)
 {
 	struct snd_timer_user *tu;
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1308720

FromDmitry Vyukov <dvyukov@google.com>
Date2016-01-13 19:40 +0100
Message-ID<qQyuR-3Ce-17@gated-at.bofh.it>
In reply to#1308623
On Wed, Jan 13, 2016 at 5:53 PM, Takashi Iwai <tiwai@suse.de> wrote:
> This and your other relevant reports seem pointing the race of timer
> ioctls.  Although snd_timer_close() itself calls snd_timer_stop(),
> there is no other protection against the concurrent execution.
>
> If my guess is correct, a simplistic fix like below should work.  It
> basically serializes the timer ioctl by using a new mutex (and
> replacing the old tread_sem mutex).  They are no longtime blocking
> calls, so this shouldn't be a big problem.  But certainly there can be
> a less intrusive way to paper over this if this really matters.
>
> In this case for timer.c, I'd leave the final decision rather to
> Jaroslav.  Jaroslav, what do you think?


After applying this patch I still see the following WARNINGS:

------------[ cut here ]------------
WARNING: CPU: 2 PID: 30398 at lib/list_debug.c:53 __list_del_entry+0x10b/0x1e0()
list_del corruption, ffff880032d933b0->next is LIST_POISON1 (dead000000000100)
Modules linked in:
CPU: 2 PID: 30398 Comm: syz-executor Not tainted 4.4.0+ #241
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 00000000ffffffff ffff8800627778d8 ffffffff82926eed ffff880062777948
 ffff880061c2af80 ffffffff8660b640 ffff880062777918 ffffffff81350c89
 ffffffff8298e77b ffffed000c4eef25 ffffffff8660b640 0000000000000035
Call Trace:
 [<     inline     >] __dump_stack lib/dump_stack.c:15
 [<ffffffff82926eed>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
 [<ffffffff81350c89>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:483
 [<ffffffff81350d99>] warn_slowpath_fmt+0xa9/0xd0 kernel/panic.c:495
 [<ffffffff8298e77b>] __list_del_entry+0x10b/0x1e0 lib/list_debug.c:51
 [<     inline     >] list_del_init include/linux/list.h:145
 [<ffffffff84ebd199>] _snd_timer_stop+0x119/0x450 sound/core/timer.c:501
 [<ffffffff84ebd4f4>] snd_timer_stop+0x24/0x140 sound/core/timer.c:535
 [<ffffffff84ebd648>] snd_timer_close+0x38/0x5f0 sound/core/timer.c:317
 [<     inline     >] snd_timer_user_tselect sound/core/timer.c:1518
 [<     inline     >] __snd_timer_user_ioctl sound/core/timer.c:1803
 [<ffffffff84ec4362>] snd_timer_user_ioctl+0x7b2/0x25c0 sound/core/timer.c:1833
 [<     inline     >] vfs_ioctl fs/ioctl.c:43
 [<ffffffff817cbd3c>] do_vfs_ioctl+0x18c/0xfa0 fs/ioctl.c:674
 [<     inline     >] SYSC_ioctl fs/ioctl.c:689
 [<ffffffff817ccbdf>] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:680
 [<ffffffff86273076>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185
---[ end trace bfebf27b922184a1 ]---

[toc] | [prev] | [next] | [standalone]

#1308733

FromTakashi Iwai <tiwai@suse.de>
Date2016-01-13 20:10 +0100
Message-ID<qQyXT-41l-1@gated-at.bofh.it>
In reply to#1308720
On Wed, 13 Jan 2016 19:34:36 +0100,
Dmitry Vyukov wrote:
> 
> On Wed, Jan 13, 2016 at 5:53 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > This and your other relevant reports seem pointing the race of timer
> > ioctls.  Although snd_timer_close() itself calls snd_timer_stop(),
> > there is no other protection against the concurrent execution.
> >
> > If my guess is correct, a simplistic fix like below should work.  It
> > basically serializes the timer ioctl by using a new mutex (and
> > replacing the old tread_sem mutex).  They are no longtime blocking
> > calls, so this shouldn't be a big problem.  But certainly there can be
> > a less intrusive way to paper over this if this really matters.
> >
> > In this case for timer.c, I'd leave the final decision rather to
> > Jaroslav.  Jaroslav, what do you think?
> 
> 
> After applying this patch I still see the following WARNINGS:
> 
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 30398 at lib/list_debug.c:53 __list_del_entry+0x10b/0x1e0()
> list_del corruption, ffff880032d933b0->next is LIST_POISON1 (dead000000000100)
> Modules linked in:
> CPU: 2 PID: 30398 Comm: syz-executor Not tainted 4.4.0+ #241
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>  00000000ffffffff ffff8800627778d8 ffffffff82926eed ffff880062777948
>  ffff880061c2af80 ffffffff8660b640 ffff880062777918 ffffffff81350c89
>  ffffffff8298e77b ffffed000c4eef25 ffffffff8660b640 0000000000000035
> Call Trace:
>  [<     inline     >] __dump_stack lib/dump_stack.c:15
>  [<ffffffff82926eed>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
>  [<ffffffff81350c89>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:483
>  [<ffffffff81350d99>] warn_slowpath_fmt+0xa9/0xd0 kernel/panic.c:495
>  [<ffffffff8298e77b>] __list_del_entry+0x10b/0x1e0 lib/list_debug.c:51
>  [<     inline     >] list_del_init include/linux/list.h:145
>  [<ffffffff84ebd199>] _snd_timer_stop+0x119/0x450 sound/core/timer.c:501

This is

	list_del_init(&timeri->active_list);

right?  Possibly the following oneliner covers it?


thanks,

Takashi

---
diff --git a/sound/core/timer.c b/sound/core/timer.c
index b03a9e489286..3810ee8f1205 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -694,7 +694,7 @@ void snd_timer_interrupt(struct snd_timer * timer, unsigned long ticks_left)
 		} else {
 			ti->flags &= ~SNDRV_TIMER_IFLG_RUNNING;
 			if (--timer->running)
-				list_del(&ti->active_list);
+				list_del_init(&ti->active_list);
 		}
 		if ((timer->hw.flags & SNDRV_TIMER_HW_TASKLET) ||
 		    (ti->flags & SNDRV_TIMER_IFLG_FAST))

[toc] | [prev] | [next] | [standalone]

#1308767

FromDmitry Vyukov <dvyukov@google.com>
Date2016-01-13 20:40 +0100
Message-ID<qQzqV-4ct-7@gated-at.bofh.it>
In reply to#1308733
On Wed, Jan 13, 2016 at 8:05 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 13 Jan 2016 19:34:36 +0100,
> Dmitry Vyukov wrote:
>>
>> On Wed, Jan 13, 2016 at 5:53 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> > This and your other relevant reports seem pointing the race of timer
>> > ioctls.  Although snd_timer_close() itself calls snd_timer_stop(),
>> > there is no other protection against the concurrent execution.
>> >
>> > If my guess is correct, a simplistic fix like below should work.  It
>> > basically serializes the timer ioctl by using a new mutex (and
>> > replacing the old tread_sem mutex).  They are no longtime blocking
>> > calls, so this shouldn't be a big problem.  But certainly there can be
>> > a less intrusive way to paper over this if this really matters.
>> >
>> > In this case for timer.c, I'd leave the final decision rather to
>> > Jaroslav.  Jaroslav, what do you think?
>>
>>
>> After applying this patch I still see the following WARNINGS:
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 2 PID: 30398 at lib/list_debug.c:53 __list_del_entry+0x10b/0x1e0()
>> list_del corruption, ffff880032d933b0->next is LIST_POISON1 (dead000000000100)
>> Modules linked in:
>> CPU: 2 PID: 30398 Comm: syz-executor Not tainted 4.4.0+ #241
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>>  00000000ffffffff ffff8800627778d8 ffffffff82926eed ffff880062777948
>>  ffff880061c2af80 ffffffff8660b640 ffff880062777918 ffffffff81350c89
>>  ffffffff8298e77b ffffed000c4eef25 ffffffff8660b640 0000000000000035
>> Call Trace:
>>  [<     inline     >] __dump_stack lib/dump_stack.c:15
>>  [<ffffffff82926eed>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
>>  [<ffffffff81350c89>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:483
>>  [<ffffffff81350d99>] warn_slowpath_fmt+0xa9/0xd0 kernel/panic.c:495
>>  [<ffffffff8298e77b>] __list_del_entry+0x10b/0x1e0 lib/list_debug.c:51
>>  [<     inline     >] list_del_init include/linux/list.h:145
>>  [<ffffffff84ebd199>] _snd_timer_stop+0x119/0x450 sound/core/timer.c:501
>
> This is
>
>         list_del_init(&timeri->active_list);
>
> right?  Possibly the following oneliner covers it?

Yes, that is this line.
Yes, these two patches fix use-after-frees and GPFs.

Tested-by: Dmitry Vyukov <dvyukov@google.com>

The only one that still happens is "sound: spinlock lockup in
sound/core/timer.c".


> thanks,
>
> Takashi
>
> ---
> diff --git a/sound/core/timer.c b/sound/core/timer.c
> index b03a9e489286..3810ee8f1205 100644
> --- a/sound/core/timer.c
> +++ b/sound/core/timer.c
> @@ -694,7 +694,7 @@ void snd_timer_interrupt(struct snd_timer * timer, unsigned long ticks_left)
>                 } else {
>                         ti->flags &= ~SNDRV_TIMER_IFLG_RUNNING;
>                         if (--timer->running)
> -                               list_del(&ti->active_list);
> +                               list_del_init(&ti->active_list);
>                 }
>                 if ((timer->hw.flags & SNDRV_TIMER_HW_TASKLET) ||
>                     (ti->flags & SNDRV_TIMER_IFLG_FAST))

[toc] | [prev] | [next] | [standalone]

#1308771

FromDmitry Vyukov <dvyukov@google.com>
Date2016-01-13 20:50 +0100
Message-ID<qQzAC-4i0-3@gated-at.bofh.it>
In reply to#1308767
On Wed, Jan 13, 2016 at 8:30 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Wed, Jan 13, 2016 at 8:05 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> On Wed, 13 Jan 2016 19:34:36 +0100,
>> Dmitry Vyukov wrote:
>>>
>>> On Wed, Jan 13, 2016 at 5:53 PM, Takashi Iwai <tiwai@suse.de> wrote:
>>> > This and your other relevant reports seem pointing the race of timer
>>> > ioctls.  Although snd_timer_close() itself calls snd_timer_stop(),
>>> > there is no other protection against the concurrent execution.
>>> >
>>> > If my guess is correct, a simplistic fix like below should work.  It
>>> > basically serializes the timer ioctl by using a new mutex (and
>>> > replacing the old tread_sem mutex).  They are no longtime blocking
>>> > calls, so this shouldn't be a big problem.  But certainly there can be
>>> > a less intrusive way to paper over this if this really matters.
>>> >
>>> > In this case for timer.c, I'd leave the final decision rather to
>>> > Jaroslav.  Jaroslav, what do you think?
>>>
>>>
>>> After applying this patch I still see the following WARNINGS:
>>>
>>> ------------[ cut here ]------------
>>> WARNING: CPU: 2 PID: 30398 at lib/list_debug.c:53 __list_del_entry+0x10b/0x1e0()
>>> list_del corruption, ffff880032d933b0->next is LIST_POISON1 (dead000000000100)
>>> Modules linked in:
>>> CPU: 2 PID: 30398 Comm: syz-executor Not tainted 4.4.0+ #241
>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>>>  00000000ffffffff ffff8800627778d8 ffffffff82926eed ffff880062777948
>>>  ffff880061c2af80 ffffffff8660b640 ffff880062777918 ffffffff81350c89
>>>  ffffffff8298e77b ffffed000c4eef25 ffffffff8660b640 0000000000000035
>>> Call Trace:
>>>  [<     inline     >] __dump_stack lib/dump_stack.c:15
>>>  [<ffffffff82926eed>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
>>>  [<ffffffff81350c89>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:483
>>>  [<ffffffff81350d99>] warn_slowpath_fmt+0xa9/0xd0 kernel/panic.c:495
>>>  [<ffffffff8298e77b>] __list_del_entry+0x10b/0x1e0 lib/list_debug.c:51
>>>  [<     inline     >] list_del_init include/linux/list.h:145
>>>  [<ffffffff84ebd199>] _snd_timer_stop+0x119/0x450 sound/core/timer.c:501
>>
>> This is
>>
>>         list_del_init(&timeri->active_list);
>>
>> right?  Possibly the following oneliner covers it?
>
> Yes, that is this line.
> Yes, these two patches fix use-after-frees and GPFs.
>
> Tested-by: Dmitry Vyukov <dvyukov@google.com>


I've re-tested the programs that I reported. But when I started the
fuzzer again I hit a similar use-after-free in snd_timer_interrupt:

BUG: KASAN: use-after-free in snd_timer_interrupt+0xaea/0xc40 at addr
ffff8800644df960
Read of size 8 by task kworker/u10:3/561
=============================================================================
BUG kmalloc-256 (Not tainted): kasan: bad access detected
-----------------------------------------------------------------------------

INFO: Allocated in snd_timer_instance_new+0x52/0x3a0 age=13 cpu=2 pid=18656
[<      none      >] ___slab_alloc+0x486/0x4e0 mm/slub.c:2468
[<      none      >] __slab_alloc+0x66/0xc0 mm/slub.c:2497
[<     inline     >] slab_alloc_node mm/slub.c:2560
[<     inline     >] slab_alloc mm/slub.c:2602
[<      none      >] kmem_cache_alloc_trace+0x284/0x310 mm/slub.c:2619
[<     inline     >] kmalloc include/linux/slab.h:458
[<     inline     >] kzalloc include/linux/slab.h:602
[<      none      >] snd_timer_instance_new+0x52/0x3a0 sound/core/timer.c:105
[<      none      >] snd_timer_open+0x522/0xc90 sound/core/timer.c:286
[<      none      >] snd_seq_timer_open+0x223/0x540
sound/core/seq/seq_timer.c:279
[<      none      >] snd_seq_queue_use+0x147/0x230
sound/core/seq/seq_queue.c:528
[<      none      >] snd_seq_queue_alloc+0x36a/0x4d0
sound/core/seq/seq_queue.c:199
[<      none      >] snd_seq_ioctl_create_queue+0xdb/0x2b0
sound/core/seq/seq_clientmgr.c:1536
[<      none      >] snd_seq_do_ioctl+0x19a/0x1c0
sound/core/seq/seq_clientmgr.c:2209
[<      none      >] snd_seq_ioctl+0x5d/0x80 sound/core/seq/seq_clientmgr.c:2224
[<     inline     >] vfs_ioctl fs/ioctl.c:43
[<      none      >] do_vfs_ioctl+0x18c/0xfa0 fs/ioctl.c:674
[<     inline     >] SYSC_ioctl fs/ioctl.c:689
[<      none      >] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:680
[<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

INFO: Freed in snd_timer_close+0x354/0x5f0 age=13 cpu=3 pid=18658
[<      none      >] __slab_free+0x1fc/0x320 mm/slub.c:2678
[<     inline     >] slab_free mm/slub.c:2833
[<      none      >] kfree+0x2a8/0x2d0 mm/slub.c:3662
[<      none      >] snd_timer_close+0x354/0x5f0 sound/core/timer.c:364
[<      none      >] snd_seq_timer_close+0x9e/0x100
sound/core/seq/seq_timer.c:312
snd_seq_queue_timer
[<      none      >] snd_seq_ioctl_set_queue_timer+0x159/0x300
sound/core/seq/seq_clientmgr.c:1809
[<      none      >] snd_seq_do_ioctl+0x19a/0x1c0
sound/core/seq/seq_clientmgr.c:2209
[<      none      >] snd_seq_ioctl+0x5d/0x80 sound/core/seq/seq_clientmgr.c:2224
[<     inline     >] vfs_ioctl fs/ioctl.c:43
[<      none      >] do_vfs_ioctl+0x18c/0xfa0 fs/ioctl.c:674
[<     inline     >] SYSC_ioctl fs/ioctl.c:689
[<      none      >] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:680
[<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185

INFO: Slab 0xffffea0001913700 objects=22 used=10 fp=0xffff8800644df8e0
flags=0x5fffc0000004080
INFO: Object 0xffff8800644df8e0 @offset=14560 fp=0xffff8800644ddf48
CPU: 2 PID: 561 Comm: kworker/u10:3 Tainted: G    B           4.4.0+ #243
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Workqueue: events_unbound call_usermodehelper_exec_work
 00000000ffffffff ffff88006d607be0 ffffffff82926eed ffff88003e807000
 ffff8800644df8e0 ffff8800644dc000 ffff88006d607c10 ffffffff81740ca4
 ffff88003e807000 ffffea0001913700 ffff8800644df8e0 ffff8800644df960

Call Trace:
 [<     inline     >] kasan_report mm/kasan/report.c:274
 [<ffffffff8174a1fe>] __asan_report_load8_noabort+0x3e/0x40
mm/kasan/report.c:295
 [<ffffffff84ebe84a>] snd_timer_interrupt+0xaea/0xc40 sound/core/timer.c:680
 [<ffffffff84ec6c16>] snd_hrtimer_callback+0x166/0x230 sound/core/hrtimer.c:54
 [<     inline     >] __run_hrtimer kernel/time/hrtimer.c:1229
 [<ffffffff814c3723>] __hrtimer_run_queues+0x363/0xc10
kernel/time/hrtimer.c:1293
 [<ffffffff814c5732>] hrtimer_interrupt+0x182/0x430 kernel/time/hrtimer.c:1327
 [<ffffffff8124e10f>] local_apic_timer_interrupt+0x6f/0xe0
arch/x86/kernel/apic/apic.c:907
 [<ffffffff81251576>] smp_apic_timer_interrupt+0x76/0xa0
arch/x86/kernel/apic/apic.c:931
 [<ffffffff86273eec>] apic_timer_interrupt+0x8c/0xa0
arch/x86/entry/entry_64.S:520
 <EOI>  [<     inline     >] alloc_task_struct_node kernel/fork.c:142
 <EOI>  [<     inline     >] dup_task_struct kernel/fork.c:342
 <EOI>  [<ffffffff8134950e>] copy_process.part.35+0x22e/0x5770
kernel/fork.c:1304
 [<     inline     >] slab_alloc_node mm/slub.c:2560
 [<ffffffff817447f3>] kmem_cache_alloc_node+0x93/0x300 mm/slub.c:2630
 [<     inline     >] alloc_task_struct_node kernel/fork.c:142
 [<     inline     >] dup_task_struct kernel/fork.c:342
 [<ffffffff8134950e>] copy_process.part.35+0x22e/0x5770 kernel/fork.c:1304
 [<     inline     >] copy_process kernel/fork.c:1275
 [<ffffffff8134ed7c>] _do_fork+0x1bc/0xcb0 kernel/fork.c:1724
 [<ffffffff8134f8a4>] kernel_thread+0x34/0x40 kernel/fork.c:1785
 [<     inline     >] call_usermodehelper_exec_sync kernel/kmod.c:275
 [<ffffffff81391874>] call_usermodehelper_exec_work+0xf4/0x230 kernel/kmod.c:327
 [<ffffffff8139e824>] process_one_work+0x794/0x1440 kernel/workqueue.c:2036
 [<ffffffff8139f5ab>] worker_thread+0xdb/0xfc0 kernel/workqueue.c:2170
 [<ffffffff813b2cef>] kthread+0x23f/0x2d0 drivers/block/aoe/aoecmd.c:1303
 [<ffffffff862734af>] ret_from_fork+0x3f/0x70 arch/x86/entry/entry_64.S:468
==================================================================

[toc] | [prev] | [next] | [standalone]

#1308792

FromTakashi Iwai <tiwai@suse.de>
Date2016-01-13 21:40 +0100
Message-ID<qQAn0-4VL-21@gated-at.bofh.it>
In reply to#1308771
On Wed, 13 Jan 2016 20:41:30 +0100,
Dmitry Vyukov wrote:
> 
> On Wed, Jan 13, 2016 at 8:30 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> > On Wed, Jan 13, 2016 at 8:05 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> On Wed, 13 Jan 2016 19:34:36 +0100,
> >> Dmitry Vyukov wrote:
> >>>
> >>> On Wed, Jan 13, 2016 at 5:53 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >>> > This and your other relevant reports seem pointing the race of timer
> >>> > ioctls.  Although snd_timer_close() itself calls snd_timer_stop(),
> >>> > there is no other protection against the concurrent execution.
> >>> >
> >>> > If my guess is correct, a simplistic fix like below should work.  It
> >>> > basically serializes the timer ioctl by using a new mutex (and
> >>> > replacing the old tread_sem mutex).  They are no longtime blocking
> >>> > calls, so this shouldn't be a big problem.  But certainly there can be
> >>> > a less intrusive way to paper over this if this really matters.
> >>> >
> >>> > In this case for timer.c, I'd leave the final decision rather to
> >>> > Jaroslav.  Jaroslav, what do you think?
> >>>
> >>>
> >>> After applying this patch I still see the following WARNINGS:
> >>>
> >>> ------------[ cut here ]------------
> >>> WARNING: CPU: 2 PID: 30398 at lib/list_debug.c:53 __list_del_entry+0x10b/0x1e0()
> >>> list_del corruption, ffff880032d933b0->next is LIST_POISON1 (dead000000000100)
> >>> Modules linked in:
> >>> CPU: 2 PID: 30398 Comm: syz-executor Not tainted 4.4.0+ #241
> >>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> >>>  00000000ffffffff ffff8800627778d8 ffffffff82926eed ffff880062777948
> >>>  ffff880061c2af80 ffffffff8660b640 ffff880062777918 ffffffff81350c89
> >>>  ffffffff8298e77b ffffed000c4eef25 ffffffff8660b640 0000000000000035
> >>> Call Trace:
> >>>  [<     inline     >] __dump_stack lib/dump_stack.c:15
> >>>  [<ffffffff82926eed>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
> >>>  [<ffffffff81350c89>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:483
> >>>  [<ffffffff81350d99>] warn_slowpath_fmt+0xa9/0xd0 kernel/panic.c:495
> >>>  [<ffffffff8298e77b>] __list_del_entry+0x10b/0x1e0 lib/list_debug.c:51
> >>>  [<     inline     >] list_del_init include/linux/list.h:145
> >>>  [<ffffffff84ebd199>] _snd_timer_stop+0x119/0x450 sound/core/timer.c:501
> >>
> >> This is
> >>
> >>         list_del_init(&timeri->active_list);
> >>
> >> right?  Possibly the following oneliner covers it?
> >
> > Yes, that is this line.
> > Yes, these two patches fix use-after-frees and GPFs.
> >
> > Tested-by: Dmitry Vyukov <dvyukov@google.com>
> 
> 
> I've re-tested the programs that I reported. But when I started the
> fuzzer again I hit a similar use-after-free in snd_timer_interrupt:

Is it the result with all patches, i.e. four patches (two for
sequencer and two for timer)?


Takashi

> 
> BUG: KASAN: use-after-free in snd_timer_interrupt+0xaea/0xc40 at addr
> ffff8800644df960
> Read of size 8 by task kworker/u10:3/561
> =============================================================================
> BUG kmalloc-256 (Not tainted): kasan: bad access detected
> -----------------------------------------------------------------------------
> 
> INFO: Allocated in snd_timer_instance_new+0x52/0x3a0 age=13 cpu=2 pid=18656
> [<      none      >] ___slab_alloc+0x486/0x4e0 mm/slub.c:2468
> [<      none      >] __slab_alloc+0x66/0xc0 mm/slub.c:2497
> [<     inline     >] slab_alloc_node mm/slub.c:2560
> [<     inline     >] slab_alloc mm/slub.c:2602
> [<      none      >] kmem_cache_alloc_trace+0x284/0x310 mm/slub.c:2619
> [<     inline     >] kmalloc include/linux/slab.h:458
> [<     inline     >] kzalloc include/linux/slab.h:602
> [<      none      >] snd_timer_instance_new+0x52/0x3a0 sound/core/timer.c:105
> [<      none      >] snd_timer_open+0x522/0xc90 sound/core/timer.c:286
> [<      none      >] snd_seq_timer_open+0x223/0x540
> sound/core/seq/seq_timer.c:279
> [<      none      >] snd_seq_queue_use+0x147/0x230
> sound/core/seq/seq_queue.c:528
> [<      none      >] snd_seq_queue_alloc+0x36a/0x4d0
> sound/core/seq/seq_queue.c:199
> [<      none      >] snd_seq_ioctl_create_queue+0xdb/0x2b0
> sound/core/seq/seq_clientmgr.c:1536
> [<      none      >] snd_seq_do_ioctl+0x19a/0x1c0
> sound/core/seq/seq_clientmgr.c:2209
> [<      none      >] snd_seq_ioctl+0x5d/0x80 sound/core/seq/seq_clientmgr.c:2224
> [<     inline     >] vfs_ioctl fs/ioctl.c:43
> [<      none      >] do_vfs_ioctl+0x18c/0xfa0 fs/ioctl.c:674
> [<     inline     >] SYSC_ioctl fs/ioctl.c:689
> [<      none      >] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:680
> [<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
> arch/x86/entry/entry_64.S:185
> 
> INFO: Freed in snd_timer_close+0x354/0x5f0 age=13 cpu=3 pid=18658
> [<      none      >] __slab_free+0x1fc/0x320 mm/slub.c:2678
> [<     inline     >] slab_free mm/slub.c:2833
> [<      none      >] kfree+0x2a8/0x2d0 mm/slub.c:3662
> [<      none      >] snd_timer_close+0x354/0x5f0 sound/core/timer.c:364
> [<      none      >] snd_seq_timer_close+0x9e/0x100
> sound/core/seq/seq_timer.c:312
> snd_seq_queue_timer
> [<      none      >] snd_seq_ioctl_set_queue_timer+0x159/0x300
> sound/core/seq/seq_clientmgr.c:1809
> [<      none      >] snd_seq_do_ioctl+0x19a/0x1c0
> sound/core/seq/seq_clientmgr.c:2209
> [<      none      >] snd_seq_ioctl+0x5d/0x80 sound/core/seq/seq_clientmgr.c:2224
> [<     inline     >] vfs_ioctl fs/ioctl.c:43
> [<      none      >] do_vfs_ioctl+0x18c/0xfa0 fs/ioctl.c:674
> [<     inline     >] SYSC_ioctl fs/ioctl.c:689
> [<      none      >] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:680
> [<      none      >] entry_SYSCALL_64_fastpath+0x16/0x7a
> arch/x86/entry/entry_64.S:185
> 
> INFO: Slab 0xffffea0001913700 objects=22 used=10 fp=0xffff8800644df8e0
> flags=0x5fffc0000004080
> INFO: Object 0xffff8800644df8e0 @offset=14560 fp=0xffff8800644ddf48
> CPU: 2 PID: 561 Comm: kworker/u10:3 Tainted: G    B           4.4.0+ #243
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> Workqueue: events_unbound call_usermodehelper_exec_work
>  00000000ffffffff ffff88006d607be0 ffffffff82926eed ffff88003e807000
>  ffff8800644df8e0 ffff8800644dc000 ffff88006d607c10 ffffffff81740ca4
>  ffff88003e807000 ffffea0001913700 ffff8800644df8e0 ffff8800644df960
> 
> Call Trace:
>  [<     inline     >] kasan_report mm/kasan/report.c:274
>  [<ffffffff8174a1fe>] __asan_report_load8_noabort+0x3e/0x40
> mm/kasan/report.c:295
>  [<ffffffff84ebe84a>] snd_timer_interrupt+0xaea/0xc40 sound/core/timer.c:680
>  [<ffffffff84ec6c16>] snd_hrtimer_callback+0x166/0x230 sound/core/hrtimer.c:54
>  [<     inline     >] __run_hrtimer kernel/time/hrtimer.c:1229
>  [<ffffffff814c3723>] __hrtimer_run_queues+0x363/0xc10
> kernel/time/hrtimer.c:1293
>  [<ffffffff814c5732>] hrtimer_interrupt+0x182/0x430 kernel/time/hrtimer.c:1327
>  [<ffffffff8124e10f>] local_apic_timer_interrupt+0x6f/0xe0
> arch/x86/kernel/apic/apic.c:907
>  [<ffffffff81251576>] smp_apic_timer_interrupt+0x76/0xa0
> arch/x86/kernel/apic/apic.c:931
>  [<ffffffff86273eec>] apic_timer_interrupt+0x8c/0xa0
> arch/x86/entry/entry_64.S:520
>  <EOI>  [<     inline     >] alloc_task_struct_node kernel/fork.c:142
>  <EOI>  [<     inline     >] dup_task_struct kernel/fork.c:342
>  <EOI>  [<ffffffff8134950e>] copy_process.part.35+0x22e/0x5770
> kernel/fork.c:1304
>  [<     inline     >] slab_alloc_node mm/slub.c:2560
>  [<ffffffff817447f3>] kmem_cache_alloc_node+0x93/0x300 mm/slub.c:2630
>  [<     inline     >] alloc_task_struct_node kernel/fork.c:142
>  [<     inline     >] dup_task_struct kernel/fork.c:342
>  [<ffffffff8134950e>] copy_process.part.35+0x22e/0x5770 kernel/fork.c:1304
>  [<     inline     >] copy_process kernel/fork.c:1275
>  [<ffffffff8134ed7c>] _do_fork+0x1bc/0xcb0 kernel/fork.c:1724
>  [<ffffffff8134f8a4>] kernel_thread+0x34/0x40 kernel/fork.c:1785
>  [<     inline     >] call_usermodehelper_exec_sync kernel/kmod.c:275
>  [<ffffffff81391874>] call_usermodehelper_exec_work+0xf4/0x230 kernel/kmod.c:327
>  [<ffffffff8139e824>] process_one_work+0x794/0x1440 kernel/workqueue.c:2036
>  [<ffffffff8139f5ab>] worker_thread+0xdb/0xfc0 kernel/workqueue.c:2170
>  [<ffffffff813b2cef>] kthread+0x23f/0x2d0 drivers/block/aoe/aoecmd.c:1303
>  [<ffffffff862734af>] ret_from_fork+0x3f/0x70 arch/x86/entry/entry_64.S:468
> ==================================================================
>

[toc] | [prev] | [next] | [standalone]

#1308798

FromDmitry Vyukov <dvyukov@google.com>
Date2016-01-13 21:50 +0100
Message-ID<qQAwF-50b-13@gated-at.bofh.it>
In reply to#1308792
On Wed, Jan 13, 2016 at 9:30 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 13 Jan 2016 20:41:30 +0100,
> Dmitry Vyukov wrote:
>>
>> On Wed, Jan 13, 2016 at 8:30 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> > On Wed, Jan 13, 2016 at 8:05 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> >> On Wed, 13 Jan 2016 19:34:36 +0100,
>> >> Dmitry Vyukov wrote:
>> >>>
>> >>> On Wed, Jan 13, 2016 at 5:53 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> >>> > This and your other relevant reports seem pointing the race of timer
>> >>> > ioctls.  Although snd_timer_close() itself calls snd_timer_stop(),
>> >>> > there is no other protection against the concurrent execution.
>> >>> >
>> >>> > If my guess is correct, a simplistic fix like below should work.  It
>> >>> > basically serializes the timer ioctl by using a new mutex (and
>> >>> > replacing the old tread_sem mutex).  They are no longtime blocking
>> >>> > calls, so this shouldn't be a big problem.  But certainly there can be
>> >>> > a less intrusive way to paper over this if this really matters.
>> >>> >
>> >>> > In this case for timer.c, I'd leave the final decision rather to
>> >>> > Jaroslav.  Jaroslav, what do you think?
>> >>>
>> >>>
>> >>> After applying this patch I still see the following WARNINGS:
>> >>>
>> >>> ------------[ cut here ]------------
>> >>> WARNING: CPU: 2 PID: 30398 at lib/list_debug.c:53 __list_del_entry+0x10b/0x1e0()
>> >>> list_del corruption, ffff880032d933b0->next is LIST_POISON1 (dead000000000100)
>> >>> Modules linked in:
>> >>> CPU: 2 PID: 30398 Comm: syz-executor Not tainted 4.4.0+ #241
>> >>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> >>>  00000000ffffffff ffff8800627778d8 ffffffff82926eed ffff880062777948
>> >>>  ffff880061c2af80 ffffffff8660b640 ffff880062777918 ffffffff81350c89
>> >>>  ffffffff8298e77b ffffed000c4eef25 ffffffff8660b640 0000000000000035
>> >>> Call Trace:
>> >>>  [<     inline     >] __dump_stack lib/dump_stack.c:15
>> >>>  [<ffffffff82926eed>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
>> >>>  [<ffffffff81350c89>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:483
>> >>>  [<ffffffff81350d99>] warn_slowpath_fmt+0xa9/0xd0 kernel/panic.c:495
>> >>>  [<ffffffff8298e77b>] __list_del_entry+0x10b/0x1e0 lib/list_debug.c:51
>> >>>  [<     inline     >] list_del_init include/linux/list.h:145
>> >>>  [<ffffffff84ebd199>] _snd_timer_stop+0x119/0x450 sound/core/timer.c:501
>> >>
>> >> This is
>> >>
>> >>         list_del_init(&timeri->active_list);
>> >>
>> >> right?  Possibly the following oneliner covers it?
>> >
>> > Yes, that is this line.
>> > Yes, these two patches fix use-after-frees and GPFs.
>> >
>> > Tested-by: Dmitry Vyukov <dvyukov@google.com>
>>
>>
>> I've re-tested the programs that I reported. But when I started the
>> fuzzer again I hit a similar use-after-free in snd_timer_interrupt:
>
> Is it the result with all patches, i.e. four patches (two for
> sequencer and two for timer)?

Yes, with 4 recent patches.

[toc] | [prev] | [next] | [standalone]

#1308810

FromTakashi Iwai <tiwai@suse.de>
Date2016-01-13 22:00 +0100
Message-ID<qQAGl-54j-7@gated-at.bofh.it>
In reply to#1308798
On Wed, 13 Jan 2016 21:48:47 +0100,
Dmitry Vyukov wrote:
> 
> On Wed, Jan 13, 2016 at 9:30 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Wed, 13 Jan 2016 20:41:30 +0100,
> > Dmitry Vyukov wrote:
> >>
> >> On Wed, Jan 13, 2016 at 8:30 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> >> > On Wed, Jan 13, 2016 at 8:05 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> >> On Wed, 13 Jan 2016 19:34:36 +0100,
> >> >> Dmitry Vyukov wrote:
> >> >>>
> >> >>> On Wed, Jan 13, 2016 at 5:53 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> >>> > This and your other relevant reports seem pointing the race of timer
> >> >>> > ioctls.  Although snd_timer_close() itself calls snd_timer_stop(),
> >> >>> > there is no other protection against the concurrent execution.
> >> >>> >
> >> >>> > If my guess is correct, a simplistic fix like below should work.  It
> >> >>> > basically serializes the timer ioctl by using a new mutex (and
> >> >>> > replacing the old tread_sem mutex).  They are no longtime blocking
> >> >>> > calls, so this shouldn't be a big problem.  But certainly there can be
> >> >>> > a less intrusive way to paper over this if this really matters.
> >> >>> >
> >> >>> > In this case for timer.c, I'd leave the final decision rather to
> >> >>> > Jaroslav.  Jaroslav, what do you think?
> >> >>>
> >> >>>
> >> >>> After applying this patch I still see the following WARNINGS:
> >> >>>
> >> >>> ------------[ cut here ]------------
> >> >>> WARNING: CPU: 2 PID: 30398 at lib/list_debug.c:53 __list_del_entry+0x10b/0x1e0()
> >> >>> list_del corruption, ffff880032d933b0->next is LIST_POISON1 (dead000000000100)
> >> >>> Modules linked in:
> >> >>> CPU: 2 PID: 30398 Comm: syz-executor Not tainted 4.4.0+ #241
> >> >>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> >> >>>  00000000ffffffff ffff8800627778d8 ffffffff82926eed ffff880062777948
> >> >>>  ffff880061c2af80 ffffffff8660b640 ffff880062777918 ffffffff81350c89
> >> >>>  ffffffff8298e77b ffffed000c4eef25 ffffffff8660b640 0000000000000035
> >> >>> Call Trace:
> >> >>>  [<     inline     >] __dump_stack lib/dump_stack.c:15
> >> >>>  [<ffffffff82926eed>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
> >> >>>  [<ffffffff81350c89>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:483
> >> >>>  [<ffffffff81350d99>] warn_slowpath_fmt+0xa9/0xd0 kernel/panic.c:495
> >> >>>  [<ffffffff8298e77b>] __list_del_entry+0x10b/0x1e0 lib/list_debug.c:51
> >> >>>  [<     inline     >] list_del_init include/linux/list.h:145
> >> >>>  [<ffffffff84ebd199>] _snd_timer_stop+0x119/0x450 sound/core/timer.c:501
> >> >>
> >> >> This is
> >> >>
> >> >>         list_del_init(&timeri->active_list);
> >> >>
> >> >> right?  Possibly the following oneliner covers it?
> >> >
> >> > Yes, that is this line.
> >> > Yes, these two patches fix use-after-frees and GPFs.
> >> >
> >> > Tested-by: Dmitry Vyukov <dvyukov@google.com>
> >>
> >>
> >> I've re-tested the programs that I reported. But when I started the
> >> fuzzer again I hit a similar use-after-free in snd_timer_interrupt:
> >
> > Is it the result with all patches, i.e. four patches (two for
> > sequencer and two for timer)?
> 
> Yes, with 4 recent patches.

OK, then this might be a possible race at the current snd_timer_stop()
implementation.  There is no sync action there, so the ISR might be
still alive after snd_timer_close() call.  Or might be another race.
This pattern looks a bit different, as it's involved with hrtimer.

I'll take a look at it tomorrow.


thanks,

Takashi

[toc] | [prev] | [next] | [standalone]

#1309452

FromTakashi Iwai <tiwai@suse.de>
Date2016-01-14 17:20 +0100
Message-ID<qQSMW-14b-9@gated-at.bofh.it>
In reply to#1308810
On Wed, 13 Jan 2016 21:54:10 +0100,
Takashi Iwai wrote:
> 
> OK, then this might be a possible race at the current snd_timer_stop()
> implementation.  There is no sync action there, so the ISR might be
> still alive after snd_timer_close() call.  Or might be another race.
> This pattern looks a bit different, as it's involved with hrtimer.
> 
> I'll take a look at it tomorrow.

I've audited the code today, but the open window doesn't look like
what I expected.  I found only some possible cases with slave timer
instances.

In anyway, below is a test fix patch.  Since I couldn't reproduce the
issue on my local machines, it's hard to say whether this covers the
holes you fell.  Let's see...


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: timer: Harden slave timer list handling

A slave timer instance might be still accessible in a racy way while
operating the master instance as it lacks of locking.  Since the
master operation is mostly protected with timer->lock, we should cope
with it while changing the slave instance, too.  Also, some linked
lists (active_list and ack_list) of slave instances aren't unlinked
immediately at stopping or closing, and this may lead to unexpected
accesses.

This patch tries to address these issues.  It adds spin lock of
timer->lock (either from master or slave, which is equivalent) in a
few places.  For avoiding a deadlock, we ensure that the global
slave_active_lock is always locked at first before each timer lock.

Also, ack and active_list of slave instances are properly unlinked at
snd_timer_stop() and snd_timer_close().

Last but not least, remove the superfluous call of _snd_timer_stop()
at removing slave links.  This is a noop, and calling it may confuse
readers wrt locking.  Further cleanup will follow in a later patch.

Actually we've got reports of use-after-free by syzkaller fuzzer, and
this hopefully fixes these issues.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/timer.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/sound/core/timer.c b/sound/core/timer.c
index 3810ee8f1205..4e8d7bfffff6 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -215,11 +215,13 @@ static void snd_timer_check_master(struct snd_timer_instance *master)
 		    slave->slave_id == master->slave_id) {
 			list_move_tail(&slave->open_list, &master->slave_list_head);
 			spin_lock_irq(&slave_active_lock);
+			spin_lock(&master->timer->lock);
 			slave->master = master;
 			slave->timer = master->timer;
 			if (slave->flags & SNDRV_TIMER_IFLG_RUNNING)
 				list_add_tail(&slave->active_list,
 					      &master->slave_active_head);
+			spin_unlock(&master->timer->lock);
 			spin_unlock_irq(&slave_active_lock);
 		}
 	}
@@ -346,15 +348,18 @@ int snd_timer_close(struct snd_timer_instance *timeri)
 		    timer->hw.close)
 			timer->hw.close(timer);
 		/* remove slave links */
+		spin_lock_irq(&slave_active_lock);
+		spin_lock(&timer->lock);
 		list_for_each_entry_safe(slave, tmp, &timeri->slave_list_head,
 					 open_list) {
-			spin_lock_irq(&slave_active_lock);
-			_snd_timer_stop(slave, 1, SNDRV_TIMER_EVENT_RESOLUTION);
 			list_move_tail(&slave->open_list, &snd_timer_slave_list);
 			slave->master = NULL;
 			slave->timer = NULL;
-			spin_unlock_irq(&slave_active_lock);
+			list_del_init(&slave->ack_list);
+			list_del_init(&slave->active_list);
 		}
+		spin_unlock(&timer->lock);
+		spin_unlock_irq(&slave_active_lock);
 		mutex_unlock(&register_mutex);
 	}
  out:
@@ -441,9 +446,12 @@ static int snd_timer_start_slave(struct snd_timer_instance *timeri)
 
 	spin_lock_irqsave(&slave_active_lock, flags);
 	timeri->flags |= SNDRV_TIMER_IFLG_RUNNING;
-	if (timeri->master)
+	if (timeri->master && timeri->timer) {
+		spin_lock(&timeri->timer->lock);
 		list_add_tail(&timeri->active_list,
 			      &timeri->master->slave_active_head);
+		spin_unlock(&timeri->timer->lock);
+	}
 	spin_unlock_irqrestore(&slave_active_lock, flags);
 	return 1; /* delayed start */
 }
@@ -489,6 +497,8 @@ static int _snd_timer_stop(struct snd_timer_instance * timeri,
 		if (!keep_flag) {
 			spin_lock_irqsave(&slave_active_lock, flags);
 			timeri->flags &= ~SNDRV_TIMER_IFLG_RUNNING;
+			list_del_init(&timeri->ack_list);
+			list_del_init(&timeri->active_list);
 			spin_unlock_irqrestore(&slave_active_lock, flags);
 		}
 		goto __end;
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1309935

FromDmitry Vyukov <dvyukov@google.com>
Date2016-01-15 09:10 +0100
Message-ID<qR7Ci-3ht-11@gated-at.bofh.it>
In reply to#1309452
On Thu, Jan 14, 2016 at 5:09 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 13 Jan 2016 21:54:10 +0100,
> Takashi Iwai wrote:
>>
>> OK, then this might be a possible race at the current snd_timer_stop()
>> implementation.  There is no sync action there, so the ISR might be
>> still alive after snd_timer_close() call.  Or might be another race.
>> This pattern looks a bit different, as it's involved with hrtimer.
>>
>> I'll take a look at it tomorrow.
>
> I've audited the code today, but the open window doesn't look like
> what I expected.  I found only some possible cases with slave timer
> instances.
>
> In anyway, below is a test fix patch.  Since I couldn't reproduce the
> issue on my local machines, it's hard to say whether this covers the
> holes you fell.  Let's see...


Hi Takashi,

I would be interested to understand why other people can't reproduce
issues that I hit pretty reliably.
I suspect that it can be due to .config. Please try with the following
config values.

I also start qemu with "-soundhw all" arg.

CONFIG_SOUND=y
CONFIG_SOUND_OSS_CORE=y
CONFIG_SOUND_OSS_CORE_PRECLAIM=y
CONFIG_SND=y
CONFIG_SND_TIMER=y
CONFIG_SND_PCM=y
CONFIG_SND_HWDEP=y
CONFIG_SND_RAWMIDI=y
CONFIG_SND_JACK=y
CONFIG_SND_SEQUENCER=y
CONFIG_SND_SEQ_DUMMY=y
CONFIG_SND_OSSEMUL=y
CONFIG_SND_MIXER_OSS=y
CONFIG_SND_PCM_OSS=y
CONFIG_SND_PCM_OSS_PLUGINS=y
CONFIG_SND_PCM_TIMER=y
CONFIG_SND_SEQUENCER_OSS=y
CONFIG_SND_HRTIMER=y
CONFIG_SND_SEQ_HRTIMER_DEFAULT=y
CONFIG_SND_SUPPORT_OLD_API=y
CONFIG_SND_PROC_FS=y
CONFIG_SND_VERBOSE_PROCFS=y
CONFIG_SND_VMASTER=y
CONFIG_SND_DMA_SGBUF=y
CONFIG_SND_RAWMIDI_SEQ=y
CONFIG_SND_OPL3_LIB_SEQ=y
CONFIG_SND_MPU401_UART=y
CONFIG_SND_OPL3_LIB=y
CONFIG_SND_AC97_CODEC=y
CONFIG_SND_DRIVERS=y
CONFIG_SND_AC97_POWER_SAVE=y
CONFIG_SND_AC97_POWER_SAVE_DEFAULT=0
CONFIG_SND_SB_COMMON=y
CONFIG_SND_PCI=y
CONFIG_SND_AD1889=y
CONFIG_SND_ALS300=y
CONFIG_SND_ALS4000=y
CONFIG_SND_ALI5451=y
CONFIG_SND_OXYGEN_LIB=y
CONFIG_SND_VIRTUOSO=y
CONFIG_SND_HDA=y
CONFIG_SND_HDA_INTEL=y
CONFIG_SND_HDA_HWDEP=y
CONFIG_SND_HDA_RECONFIG=y
CONFIG_SND_HDA_INPUT_BEEP=y
CONFIG_SND_HDA_INPUT_BEEP_MODE=1
CONFIG_SND_HDA_PATCH_LOADER=y
CONFIG_SND_HDA_CODEC_REALTEK=y
CONFIG_SND_HDA_CODEC_ANALOG=y
CONFIG_SND_HDA_CODEC_SIGMATEL=y
CONFIG_SND_HDA_CODEC_VIA=y
CONFIG_SND_HDA_CODEC_HDMI=y
CONFIG_SND_HDA_GENERIC=y
CONFIG_SND_HDA_POWER_SAVE_DEFAULT=0
CONFIG_SND_HDA_CORE=y
CONFIG_SND_HDA_I915=y
CONFIG_SND_HDA_PREALLOC_SIZE=64
CONFIG_SND_USB=y
CONFIG_SND_USB_AUDIO=y
CONFIG_SND_FIREWIRE=y
CONFIG_SND_FIREWIRE_LIB=y
CONFIG_SND_DICE=y
CONFIG_SND_OXFW=y
CONFIG_SND_ISIGHT=y
CONFIG_SND_SCS1X=y
CONFIG_SND_FIREWORKS=y
CONFIG_SND_BEBOB=y
CONFIG_SND_FIREWIRE_DIGI00X=y
CONFIG_SND_FIREWIRE_TASCAM=y
CONFIG_SND_PCMCIA=y



> Takashi
>
> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ALSA: timer: Harden slave timer list handling
>
> A slave timer instance might be still accessible in a racy way while
> operating the master instance as it lacks of locking.  Since the
> master operation is mostly protected with timer->lock, we should cope
> with it while changing the slave instance, too.  Also, some linked
> lists (active_list and ack_list) of slave instances aren't unlinked
> immediately at stopping or closing, and this may lead to unexpected
> accesses.
>
> This patch tries to address these issues.  It adds spin lock of
> timer->lock (either from master or slave, which is equivalent) in a
> few places.  For avoiding a deadlock, we ensure that the global
> slave_active_lock is always locked at first before each timer lock.
>
> Also, ack and active_list of slave instances are properly unlinked at
> snd_timer_stop() and snd_timer_close().
>
> Last but not least, remove the superfluous call of _snd_timer_stop()
> at removing slave links.  This is a noop, and calling it may confuse
> readers wrt locking.  Further cleanup will follow in a later patch.
>
> Actually we've got reports of use-after-free by syzkaller fuzzer, and
> this hopefully fixes these issues.
>
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/core/timer.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/sound/core/timer.c b/sound/core/timer.c
> index 3810ee8f1205..4e8d7bfffff6 100644
> --- a/sound/core/timer.c
> +++ b/sound/core/timer.c
> @@ -215,11 +215,13 @@ static void snd_timer_check_master(struct snd_timer_instance *master)
>                     slave->slave_id == master->slave_id) {
>                         list_move_tail(&slave->open_list, &master->slave_list_head);
>                         spin_lock_irq(&slave_active_lock);
> +                       spin_lock(&master->timer->lock);
>                         slave->master = master;
>                         slave->timer = master->timer;
>                         if (slave->flags & SNDRV_TIMER_IFLG_RUNNING)
>                                 list_add_tail(&slave->active_list,
>                                               &master->slave_active_head);
> +                       spin_unlock(&master->timer->lock);
>                         spin_unlock_irq(&slave_active_lock);
>                 }
>         }
> @@ -346,15 +348,18 @@ int snd_timer_close(struct snd_timer_instance *timeri)
>                     timer->hw.close)
>                         timer->hw.close(timer);
>                 /* remove slave links */
> +               spin_lock_irq(&slave_active_lock);
> +               spin_lock(&timer->lock);
>                 list_for_each_entry_safe(slave, tmp, &timeri->slave_list_head,
>                                          open_list) {
> -                       spin_lock_irq(&slave_active_lock);
> -                       _snd_timer_stop(slave, 1, SNDRV_TIMER_EVENT_RESOLUTION);
>                         list_move_tail(&slave->open_list, &snd_timer_slave_list);
>                         slave->master = NULL;
>                         slave->timer = NULL;
> -                       spin_unlock_irq(&slave_active_lock);
> +                       list_del_init(&slave->ack_list);
> +                       list_del_init(&slave->active_list);
>                 }
> +               spin_unlock(&timer->lock);
> +               spin_unlock_irq(&slave_active_lock);
>                 mutex_unlock(&register_mutex);
>         }
>   out:
> @@ -441,9 +446,12 @@ static int snd_timer_start_slave(struct snd_timer_instance *timeri)
>
>         spin_lock_irqsave(&slave_active_lock, flags);
>         timeri->flags |= SNDRV_TIMER_IFLG_RUNNING;
> -       if (timeri->master)
> +       if (timeri->master && timeri->timer) {
> +               spin_lock(&timeri->timer->lock);
>                 list_add_tail(&timeri->active_list,
>                               &timeri->master->slave_active_head);
> +               spin_unlock(&timeri->timer->lock);
> +       }
>         spin_unlock_irqrestore(&slave_active_lock, flags);
>         return 1; /* delayed start */
>  }
> @@ -489,6 +497,8 @@ static int _snd_timer_stop(struct snd_timer_instance * timeri,
>                 if (!keep_flag) {
>                         spin_lock_irqsave(&slave_active_lock, flags);
>                         timeri->flags &= ~SNDRV_TIMER_IFLG_RUNNING;
> +                       list_del_init(&timeri->ack_list);
> +                       list_del_init(&timeri->active_list);
>                         spin_unlock_irqrestore(&slave_active_lock, flags);
>                 }
>                 goto __end;
> --
> 2.7.0
>
>

[toc] | [prev] | [next] | [standalone]

#1308799

FromTakashi Iwai <tiwai@suse.de>
Date2016-01-13 21:50 +0100
Message-ID<qQAwF-50b-15@gated-at.bofh.it>
In reply to#1308767
On Wed, 13 Jan 2016 20:30:01 +0100,
Dmitry Vyukov wrote:
> 
> On Wed, Jan 13, 2016 at 8:05 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Wed, 13 Jan 2016 19:34:36 +0100,
> > Dmitry Vyukov wrote:
> >>
> >> On Wed, Jan 13, 2016 at 5:53 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> > This and your other relevant reports seem pointing the race of timer
> >> > ioctls.  Although snd_timer_close() itself calls snd_timer_stop(),
> >> > there is no other protection against the concurrent execution.
> >> >
> >> > If my guess is correct, a simplistic fix like below should work.  It
> >> > basically serializes the timer ioctl by using a new mutex (and
> >> > replacing the old tread_sem mutex).  They are no longtime blocking
> >> > calls, so this shouldn't be a big problem.  But certainly there can be
> >> > a less intrusive way to paper over this if this really matters.
> >> >
> >> > In this case for timer.c, I'd leave the final decision rather to
> >> > Jaroslav.  Jaroslav, what do you think?
> >>
> >>
> >> After applying this patch I still see the following WARNINGS:
> >>
> >> ------------[ cut here ]------------
> >> WARNING: CPU: 2 PID: 30398 at lib/list_debug.c:53 __list_del_entry+0x10b/0x1e0()
> >> list_del corruption, ffff880032d933b0->next is LIST_POISON1 (dead000000000100)
> >> Modules linked in:
> >> CPU: 2 PID: 30398 Comm: syz-executor Not tainted 4.4.0+ #241
> >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> >>  00000000ffffffff ffff8800627778d8 ffffffff82926eed ffff880062777948
> >>  ffff880061c2af80 ffffffff8660b640 ffff880062777918 ffffffff81350c89
> >>  ffffffff8298e77b ffffed000c4eef25 ffffffff8660b640 0000000000000035
> >> Call Trace:
> >>  [<     inline     >] __dump_stack lib/dump_stack.c:15
> >>  [<ffffffff82926eed>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
> >>  [<ffffffff81350c89>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:483
> >>  [<ffffffff81350d99>] warn_slowpath_fmt+0xa9/0xd0 kernel/panic.c:495
> >>  [<ffffffff8298e77b>] __list_del_entry+0x10b/0x1e0 lib/list_debug.c:51
> >>  [<     inline     >] list_del_init include/linux/list.h:145
> >>  [<ffffffff84ebd199>] _snd_timer_stop+0x119/0x450 sound/core/timer.c:501
> >
> > This is
> >
> >         list_del_init(&timeri->active_list);
> >
> > right?  Possibly the following oneliner covers it?
> 
> Yes, that is this line.
> Yes, these two patches fix use-after-frees and GPFs.
> 
> Tested-by: Dmitry Vyukov <dvyukov@google.com>

OK, I'm going to queue the list fix now, at least.  Below is the
patch with a proper description.

If there comes no objection, I'll queue the former ioctl mutex fix,
too.

> The only one that still happens is "sound: spinlock lockup in
> sound/core/timer.c".

Oh well, there is another :-<


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: timer: Fix double unlink of active_list

ALSA timer instance object has a couple of linked lists and they are
unlinked unconditionally at snd_timer_stop().  Meanwhile
snd_timer_interrupt() unlinks it, but it calls list_del() which leaves
the element list itself unchanged.  This ends up with unlinking twice,
and it was caught by syzkaller fuzzer.

The fix is to use list_del_init() variant properly there, too.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Tested-by: Dmitry Vyukov <dvyukov@google.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/core/timer.c b/sound/core/timer.c
index 31f40f03e5b7..9241784dfe7d 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -694,7 +694,7 @@ void snd_timer_interrupt(struct snd_timer * timer, unsigned long ticks_left)
 		} else {
 			ti->flags &= ~SNDRV_TIMER_IFLG_RUNNING;
 			if (--timer->running)
-				list_del(&ti->active_list);
+				list_del_init(&ti->active_list);
 		}
 		if ((timer->hw.flags & SNDRV_TIMER_HW_TASKLET) ||
 		    (ti->flags & SNDRV_TIMER_IFLG_FAST))
-- 
2.7.0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web