Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753813AbdHKVXq (ORCPT ); Fri, 11 Aug 2017 17:23:46 -0400 Received: from mail-it0-f42.google.com ([209.85.214.42]:37854 "EHLO mail-it0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753790AbdHKVXo (ORCPT ); Fri, 11 Aug 2017 17:23:44 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170811030734.37741-1-danielmentz@google.com> From: Daniel Mentz Date: Fri, 11 Aug 2017 14:23:42 -0700 Message-ID: Subject: Re: [PATCH] ALSA: seq: 2nd attempt at fixing race creating a q To: Takashi Iwai Cc: alsa-devel@alsa-project.org, lkml , Jaroslav Kysela , stable@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6424 Lines: 133 On Fri, Aug 11, 2017 at 7:42 AM, Takashi Iwai wrote: > > On Fri, 11 Aug 2017 05:07:34 +0200, > Daniel Mentz wrote: > > > > commit 4842e98f26dd80be3623c4714a244ba52ea096a8 ("ALSA: seq: Fix race at > > creating a queue") attempted to fix a race reported by syzkaller. That > > fix has been described as follows: > > > > " > > When a sequencer queue is created in snd_seq_queue_alloc(),it adds the > > new queue element to the public list before referencing it. Thus the > > queue might be deleted before the call of snd_seq_queue_use(), and it > > results in the use-after-free error, as spotted by syzkaller. > > > > The fix is to reference the queue object at the right time. > > " > > > > The last phrase in that commit message refers to calling queue_use(q, client, > > 1) which increments queue->clients, but that does not prevent the > > DELETE_QUEUE ioctl() and queue_delete() from kfree()ing the queue. > > Hence, the commit is not effective at preventing the race. > > kfree() is performed only after snd_use_lock_sync(), thus by acquiring > snd_use_lock() it doesn't race. If it were already deleted, > queue_use() returns NULL so it's also fine. queue_use() is defined to return void. I am assuming you're referring to queueptr(). Where do you acquire the snd_use_lock i.e. where do you call snd_use_lock_use()? My understanding is that it's called from queueptr(). With that said, there is a tiny gap between the new queue being added to the list in queue_list_add() (from snd_seq_queue_alloc()) and the call to queueptr() in snd_seq_ioctl_create_queue(). syzkaller specifically points to the last instruction "return q->queue;" in snd_seq_queue_alloc(). This is *after* q has been added to the list (and is therefore visible to other threads) but *before* it has been locked through snd_seq_ioctl_create_queue()->queueptr()->snd_use_lock_use(). Hence, there is a possibility that the pointer q is no longer valid. You could capture the return value from queue_list_add() which is identical to q->queue and then return that. However, the other issue is that queueptr() indeed returns NULL if the queue with that index has been deleted, but it's theoretically possible that the queue has been deleted and a *different* been created that now occupies the same spot in the queue_list. > Or do you actually see any crash or the wild behavior? syzkaller reported a crash: syzkaller hit the following crash on 7b2727c68878444d8f47d2d394395e4a11929624 https://android.googlesource.com/kernel/common/android-4.9 compiler: gcc (GCC) 7.1.1 20170620 ================================================================== BUG: KASAN: use-after-free in snd_seq_queue_alloc+0x47e/0x4a0 sound/core/seq/seq_queue.c:202 at addr ffff8801c7622000 Read of size 4 by task syz-executor1/23085 CPU: 1 PID: 23085 Comm: syz-executor1 Not tainted 4.9.40-g7b2727c #16 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 ffff8801cc49faa8 ffffffff81d8f109 ffff8801da001280 ffff8801c7622000 ffff8801c7622200 ffffed0038ec4400 ffff8801c7622000 ffff8801cc49fad0 ffffffff8153931c ffffed0038ec4400 ffff8801da001280 0000000000000000 Call Trace: [] __dump_stack lib/dump_stack.c:15 [inline] [] dump_stack+0xc1/0x128 lib/dump_stack.c:51 [] kasan_object_err+0x1c/0x70 mm/kasan/report.c:160 [] print_address_description mm/kasan/report.c:198 [inline] [] kasan_report_error mm/kasan/report.c:287 [inline] [] kasan_report.part.1+0x21c/0x500 mm/kasan/report.c:309 [] kasan_report mm/kasan/report.c:329 [inline] [] __asan_report_load4_noabort+0x29/0x30 mm/kasan/report.c:329 [] snd_seq_queue_alloc+0x47e/0x4a0 sound/core/seq/seq_queue.c:202 [] snd_seq_ioctl_create_queue+0xad/0x310 sound/core/seq/seq_clientmgr.c:1508 [] snd_seq_ioctl+0x226/0x4a0 sound/core/seq/seq_clientmgr.c:2131 [] vfs_ioctl fs/ioctl.c:43 [inline] [] do_vfs_ioctl+0x1aa/0x10c0 fs/ioctl.c:679 [] SYSC_ioctl fs/ioctl.c:694 [inline] [] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:685 [] entry_SYSCALL_64_fastpath+0x23/0xc6 Object at ffff8801c7622000, in cache kmalloc-512 size: 512 Allocated: PID = 23085 save_stack_trace+0x16/0x20 arch/x86/kernel/stacktrace.c:57 save_stack+0x43/0xd0 mm/kasan/kasan.c:495 set_track mm/kasan/kasan.c:507 [inline] kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:598 kmem_cache_alloc_trace+0xfb/0x2a0 mm/slub.c:2742 kmalloc include/linux/slab.h:490 [inline] kzalloc include/linux/slab.h:636 [inline] queue_new sound/core/seq/seq_queue.c:113 [inline] snd_seq_queue_alloc+0x5d/0x4a0 sound/core/seq/seq_queue.c:193 snd_seq_ioctl_create_queue+0xad/0x310 sound/core/seq/seq_clientmgr.c:1508 snd_seq_ioctl+0x226/0x4a0 sound/core/seq/seq_clientmgr.c:2131 vfs_ioctl fs/ioctl.c:43 [inline] do_vfs_ioctl+0x1aa/0x10c0 fs/ioctl.c:679 SYSC_ioctl fs/ioctl.c:694 [inline] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:685 entry_SYSCALL_64_fastpath+0x23/0xc6 Freed: PID = 23098 save_stack_trace+0x16/0x20 arch/x86/kernel/stacktrace.c:57 save_stack+0x43/0xd0 mm/kasan/kasan.c:495 set_track mm/kasan/kasan.c:507 [inline] kasan_slab_free+0x73/0xc0 mm/kasan/kasan.c:571 slab_free_hook mm/slub.c:1355 [inline] slab_free_freelist_hook mm/slub.c:1377 [inline] slab_free mm/slub.c:2958 [inline] kfree+0xf0/0x2f0 mm/slub.c:3878 queue_delete+0x90/0xb0 sound/core/seq/seq_queue.c:156 snd_seq_queue_delete+0x3c/0x50 sound/core/seq/seq_queue.c:215 snd_seq_ioctl_delete_queue+0x6a/0x90 sound/core/seq/seq_clientmgr.c:1534 snd_seq_ioctl+0x226/0x4a0 sound/core/seq/seq_clientmgr.c:2131 vfs_ioctl fs/ioctl.c:43 [inline] do_vfs_ioctl+0x1aa/0x10c0 fs/ioctl.c:679 SYSC_ioctl fs/ioctl.c:694 [inline] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:685 entry_SYSCALL_64_fastpath+0x23/0xc6 Memory state around the buggy address: ffff8801c7621f00: fb fb fb fb fb fb fb fb fc fc fc fc fb fb fb fb ffff8801c7621f80: fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc fc >ffff8801c7622000: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ^ ffff8801c7622080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ffff8801c7622100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ==================================================================