Re: sound: use-after-free in snd_timer_interrupt
From: Takashi Iwai
Date: Wed Jan 13 2016 - 11:53:53 EST
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@xxxxxxx>
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@xxxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx>
Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
---
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