From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932507AbcH2HDR (ORCPT ); Mon, 29 Aug 2016 03:03:17 -0400 Received: from mx2.suse.de ([195.135.220.15]:51506 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756487AbcH2HCk (ORCPT ); Mon, 29 Aug 2016 03:02:40 -0400 Date: Mon, 29 Aug 2016 09:02:38 +0200 Message-ID: From: Takashi Iwai To: "Vegard Nossum" Cc: "Jaroslav Kysela" , , "syzkaller" , Subject: Re: [PATCH 1/3] ALSA: timer: fix NULL pointer dereference in read()/ioctl() race In-Reply-To: <20160828223351.32489-1-vegard.nossum@oracle.com> References: <20160828223351.32489-1-vegard.nossum@oracle.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.5 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 29 Aug 2016 00:33:49 +0200, Vegard Nossum wrote: > > 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 the NULL pointer dereference by not touching the pointers > until we have allocated the new memory (similar to what is done in > ioctl(SNDRV_TIMER_IOCTL_PARAMS)) and we can fix the use-after-free 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 > --- > sound/core/timer.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/sound/core/timer.c b/sound/core/timer.c > index 9a6157e..7899e37 100644 > --- a/sound/core/timer.c > +++ b/sound/core/timer.c > @@ -1602,15 +1602,25 @@ static int snd_timer_user_tselect(struct file *file, > kfree(tu->tqueue); > tu->tqueue = NULL; > if (tu->tread) { > - tu->tqueue = kmalloc(tu->queue_size * sizeof(struct snd_timer_tread), > + struct snd_timer_tread *ttr; > + ttr = kmalloc(tu->queue_size * sizeof(struct snd_timer_tread), > GFP_KERNEL); > - if (tu->tqueue == NULL) > + if (ttr) { > + kfree(tu->tqueue); > + tu->tqueue = ttr; This looks like the double-tree, as you didn't remove the kfree() call in the above. But, I guess this change is superfluous when you introduce the mutex at... > @@ -1958,6 +1968,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))) ... here. The mutex alone should suffice. thanks, Takashi