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:
> [<ffffffff81f9f141>] dump_stack+0x83/0xb2
> [<ffffffff8161fe3f>] kasan_report_error+0x41f/0x4c0
> [<ffffffff8161ff74>] kasan_report+0x34/0x40
> [<ffffffff82c84b54>] ? snd_timer_user_read+0x554/0x790
> [<ffffffff8161e79e>] check_memory_region+0x13e/0x1a0
> [<ffffffff8161e9c1>] kasan_check_read+0x11/0x20
> [<ffffffff82c84b54>] snd_timer_user_read+0x554/0x790
> [<ffffffff82c84600>] ? snd_timer_user_info_compat.isra.5+0x2b0/0x2b0
> [<ffffffff817d0831>] ? proc_fault_inject_write+0x1c1/0x250
> [<ffffffff817d0670>] ? next_tgid+0x2a0/0x2a0
> [<ffffffff8127c278>] ? do_group_exit+0x108/0x330
> [<ffffffff8174653a>] ? fsnotify+0x72a/0xca0
> [<ffffffff81674dfe>] __vfs_read+0x10e/0x550
> [<ffffffff82c84600>] ? snd_timer_user_info_compat.isra.5+0x2b0/0x2b0
> [<ffffffff81674cf0>] ? do_sendfile+0xc50/0xc50
> [<ffffffff81745e10>] ? __fsnotify_update_child_dentry_flags+0x60/0x60
> [<ffffffff8143fec6>] ? kcov_ioctl+0x56/0x190
> [<ffffffff81e5ada2>] ? common_file_perm+0x2e2/0x380
> [<ffffffff81746b0e>] ? __fsnotify_parent+0x5e/0x2b0
> [<ffffffff81d93536>] ? security_file_permission+0x86/0x1e0
> [<ffffffff816728f5>] ? rw_verify_area+0xe5/0x2b0
> [<ffffffff81675355>] vfs_read+0x115/0x330
> [<ffffffff81676371>] SyS_read+0xd1/0x1a0
> [<ffffffff816762a0>] ? vfs_write+0x4b0/0x4b0
> [<ffffffff82001c2c>] ? __this_cpu_preempt_check+0x1c/0x20
> [<ffffffff8150455a>] ? __context_tracking_exit.part.4+0x3a/0x1e0
> [<ffffffff816762a0>] ? vfs_write+0x4b0/0x4b0
> [<ffffffff81005524>] do_syscall_64+0x1c4/0x4e0
> [<ffffffff810052fc>] ? syscall_return_slowpath+0x16c/0x1d0
> [<ffffffff83c3276a>] 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 <vegard.nossum@xxxxxxxxxx>
> ---
> 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
