From patchwork Mon Aug 29 07:14:15 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Vegard Nossum X-Patchwork-Id: 9304869 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 529BC60756 for ; Tue, 30 Aug 2016 05:47:54 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4291328A40 for ; Tue, 30 Aug 2016 05:47:54 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 370A328AB7; Tue, 30 Aug 2016 05:47:54 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE autolearn=unavailable version=3.3.1 Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 71D2828A40 for ; Tue, 30 Aug 2016 05:47:53 +0000 (UTC) Received: by alsa0.perex.cz (Postfix, from userid 1000) id 90A6B266CFE; Tue, 30 Aug 2016 07:47:52 +0200 (CEST) Received: from alsa0.perex.cz (localhost [127.0.0.1]) by alsa0.perex.cz (Postfix) with ESMTP id 60625266885; Tue, 30 Aug 2016 07:44:49 +0200 (CEST) X-Original-To: alsa-devel@alsa-project.org Delivered-To: alsa-devel@alsa-project.org Received: by alsa0.perex.cz (Postfix, from userid 1000) id 3A79A267630; Mon, 29 Aug 2016 19:10:46 +0200 (CEST) Received: from userp1050.oracle.com (userp1050.oracle.com [156.151.31.82]) by alsa0.perex.cz (Postfix) with ESMTP id 6F7562676D2 for ; Mon, 29 Aug 2016 18:28:49 +0200 (CEST) Received: from userp1040.oracle.com (userp1040.oracle.com [156.151.31.81]) by userp1050.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id u7T7JM8x026042 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Mon, 29 Aug 2016 07:19:24 GMT Received: from userv0021.oracle.com (userv0021.oracle.com [156.151.31.71]) by userp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id u7T7EKll017922 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 29 Aug 2016 07:14:20 GMT Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by userv0021.oracle.com (8.13.8/8.13.8) with ESMTP id u7T7EJSB013470 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 29 Aug 2016 07:14:20 GMT Received: from abhmp0015.oracle.com (abhmp0015.oracle.com [141.146.116.21]) by aserv0121.oracle.com (8.13.8/8.13.8) with ESMTP id u7T7EI5P003193; Mon, 29 Aug 2016 07:14:19 GMT Received: from [10.175.181.63] (/10.175.181.63) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Mon, 29 Aug 2016 00:14:18 -0700 To: Takashi Iwai References: <20160828223351.32489-1-vegard.nossum@oracle.com> From: Vegard Nossum Message-ID: <0560accf-2461-8205-6377-32b464bfce7e@oracle.com> Date: Mon, 29 Aug 2016 09:14:15 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: X-Source-IP: userp1040.oracle.com [156.151.31.81] X-Mailman-Approved-At: Tue, 30 Aug 2016 07:44:42 +0200 Cc: alsa-devel@alsa-project.org, syzkaller , linux-kernel@vger.kernel.org Subject: Re: [alsa-devel] [PATCH 1/3] ALSA: timer: fix NULL pointer dereference in read()/ioctl() race X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org X-Virus-Scanned: ClamAV using ClamSMTP On 08/29/2016 09:02 AM, Takashi Iwai wrote: > On Mon, 29 Aug 2016 00:33:49 +0200, > Vegard Nossum wrote: >> @@ -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... You're right, this hunk is garbage. Please see the new patch (attached), I also changed the patch description slightly to match the changes. I'll start running some tests on the new patch. Thanks! Vegard From fa0c216554c991347fb9a0c86832d45c63343e28 Mon Sep 17 00:00:00 2001 From: Vegard Nossum Date: Sun, 28 Aug 2016 10:13:07 +0200 Subject: [PATCH] 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 --- sound/core/timer.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sound/core/timer.c b/sound/core/timer.c index 9a6157e..083c57f 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -1958,6 +1958,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))) @@ -1967,6 +1968,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) -- 2.10.0.rc0.1.g07c9292