From: Vegard Nossum Date: Sun, 28 Aug 2016 08:13:07 +0000 (+0200) Subject: ALSA: timer: fix NULL pointer dereference in read()/ioctl() race X-Git-Tag: v4.8-rc6~13^2~2 X-Git-Url: http://git.cascardo.info/?p=cascardo%2Flinux.git;a=commitdiff_plain;h=11749e086b2766cccf6217a527ef5c5604ba069c ALSA: timer: fix NULL pointer dereference in read()/ioctl() race I got this with syzkaller: ================================================================== BUG: KASAN: null-ptr-deref on address 0000000000000020 Read of size 32 by task syz-executor/22519 CPU: 1 PID: 22519 Comm: syz-executor Not tainted 4.8.0-rc2+ #169 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.3-0-ge2fc41e-prebuilt.qemu-project.org 04/01/2 014 0000000000000001 ffff880111a17a00 ffffffff81f9f141 ffff880111a17a90 ffff880111a17c50 ffff880114584a58 ffff880114584a10 ffff880111a17a80 ffffffff8161fe3f ffff880100000000 ffff880118d74a48 ffff880118d74a68 Call Trace: [] dump_stack+0x83/0xb2 [] kasan_report_error+0x41f/0x4c0 [] kasan_report+0x34/0x40 [] ? snd_timer_user_read+0x554/0x790 [] check_memory_region+0x13e/0x1a0 [] kasan_check_read+0x11/0x20 [] snd_timer_user_read+0x554/0x790 [] ? snd_timer_user_info_compat.isra.5+0x2b0/0x2b0 [] ? proc_fault_inject_write+0x1c1/0x250 [] ? next_tgid+0x2a0/0x2a0 [] ? do_group_exit+0x108/0x330 [] ? fsnotify+0x72a/0xca0 [] __vfs_read+0x10e/0x550 [] ? snd_timer_user_info_compat.isra.5+0x2b0/0x2b0 [] ? do_sendfile+0xc50/0xc50 [] ? __fsnotify_update_child_dentry_flags+0x60/0x60 [] ? kcov_ioctl+0x56/0x190 [] ? common_file_perm+0x2e2/0x380 [] ? __fsnotify_parent+0x5e/0x2b0 [] ? security_file_permission+0x86/0x1e0 [] ? rw_verify_area+0xe5/0x2b0 [] vfs_read+0x115/0x330 [] SyS_read+0xd1/0x1a0 [] ? vfs_write+0x4b0/0x4b0 [] ? __this_cpu_preempt_check+0x1c/0x20 [] ? __context_tracking_exit.part.4+0x3a/0x1e0 [] ? vfs_write+0x4b0/0x4b0 [] do_syscall_64+0x1c4/0x4e0 [] ? syscall_return_slowpath+0x16c/0x1d0 [] entry_SYSCALL64_slow_path+0x25/0x25 ================================================================== There are a couple of problems that I can see: - ioctl(SNDRV_TIMER_IOCTL_SELECT), which potentially sets tu->queue/tu->tqueue to NULL on memory allocation failure, so read() would get a NULL pointer dereference like the above splat - the same ioctl() can free tu->queue/to->tqueue which means read() could potentially see (and dereference) the freed pointer We can fix both by taking the ioctl_lock mutex when dereferencing ->queue/->tqueue, since that's always held over all the ioctl() code. Just looking at the code I find it likely that there are more problems here such as tu->qhead pointing outside the buffer if the size is changed concurrently using SNDRV_TIMER_IOCTL_PARAMS. Signed-off-by: Vegard Nossum Cc: Signed-off-by: Takashi Iwai --- diff --git a/sound/core/timer.c b/sound/core/timer.c index 2d6e3e76ddf5..2706061fc1ea 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -1972,6 +1972,7 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer, tu->qused--; spin_unlock_irq(&tu->qlock); + mutex_lock(&tu->ioctl_lock); if (tu->tread) { if (copy_to_user(buffer, &tu->tqueue[qhead], sizeof(struct snd_timer_tread))) @@ -1981,6 +1982,7 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer, sizeof(struct snd_timer_read))) err = -EFAULT; } + mutex_unlock(&tu->ioctl_lock); spin_lock_irq(&tu->qlock); if (err < 0)