[PATCH 1/3] ALSA: timer: fix NULL pointer dereference in read()/ioctl() race

[PATCH 1/3] ALSA: timer: fix NULL pointer dereference in read()/ioctl() race

[Vegard Nossum]

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@oracle.com>
---
 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;
+		} else {
 			err = -ENOMEM;
+		}
 	} else {
-		tu->queue = kmalloc(tu->queue_size * sizeof(struct snd_timer_read),
+		struct snd_timer_read *tr;
+		tr = kmalloc(tu->queue_size * sizeof(struct snd_timer_read),
 				    GFP_KERNEL);
-		if (tu->queue == NULL)
+		if (tr) {
+			kfree(tu->queue);
+			tu->queue = tr;
+		} else {
 			err = -ENOMEM;
+		}
 	}
 
       	if (err < 0) {
@@ -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)))
@@ -1967,6 +1978,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

[PATCH 2/3] ALSA: timer: fix division by zero after SNDRV_TIMER_IOCTL_CONTINUE

[Vegard Nossum]

I got this:

    divide error: 0000 [#1] PREEMPT SMP KASAN
    CPU: 1 PID: 1327 Comm: a.out Not tainted 4.8.0-rc2+ #189
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.3-0-ge2fc41e-prebuilt.qemu-project.org 04/01/2014
    task: ffff8801120a9580 task.stack: ffff8801120b0000
    RIP: 0010:[<ffffffff82c8bd9a>]  [<ffffffff82c8bd9a>] snd_hrtimer_callback+0x1da/0x3f0
    RSP: 0018:ffff88011aa87da8  EFLAGS: 00010006
    RAX: 0000000000004f76 RBX: ffff880112655e88 RCX: 0000000000000000
    RDX: 0000000000000000 RSI: ffff880112655ea0 RDI: 0000000000000001
    RBP: ffff88011aa87e00 R08: ffff88013fff905c R09: ffff88013fff9048
    R10: ffff88013fff9050 R11: 00000001050a7b8c R12: ffff880114778a00
    R13: ffff880114778ab4 R14: ffff880114778b30 R15: 0000000000000000
    FS:  00007f071647c700(0000) GS:ffff88011aa80000(0000) knlGS:0000000000000000
    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    CR2: 0000000000603001 CR3: 0000000112021000 CR4: 00000000000006e0
    Stack:
     0000000000000000 ffff880114778ab8 ffff880112655ea0 0000000000004f76
     ffff880112655ec8 ffff880112655e80 ffff880112655e88 ffff88011aa98fc0
     00000000b97ccf2b dffffc0000000000 ffff88011aa98fc0 ffff88011aa87ef0
    Call Trace:
     <IRQ>
     [<ffffffff813abce7>] __hrtimer_run_queues+0x347/0xa00
     [<ffffffff82c8bbc0>] ? snd_hrtimer_close+0x130/0x130
     [<ffffffff813ab9a0>] ? retrigger_next_event+0x1b0/0x1b0
     [<ffffffff813ae1a6>] ? hrtimer_interrupt+0x136/0x4b0
     [<ffffffff813ae220>] hrtimer_interrupt+0x1b0/0x4b0
     [<ffffffff8120f91e>] local_apic_timer_interrupt+0x6e/0xf0
     [<ffffffff81227ad3>] ? kvm_guest_apic_eoi_write+0x13/0xc0
     [<ffffffff83c35086>] smp_apic_timer_interrupt+0x76/0xa0
     [<ffffffff83c3416c>] apic_timer_interrupt+0x8c/0xa0
     <EOI>
     [<ffffffff83c3239c>] ? _raw_spin_unlock_irqrestore+0x2c/0x60
     [<ffffffff82c8185d>] snd_timer_start1+0xdd/0x670
     [<ffffffff82c87015>] snd_timer_continue+0x45/0x80
     [<ffffffff82c88100>] snd_timer_user_ioctl+0x1030/0x2830
     [<ffffffff8159f3a0>] ? __follow_pte.isra.49+0x430/0x430
     [<ffffffff82c870d0>] ? snd_timer_pause+0x80/0x80
     [<ffffffff815a26fa>] ? do_wp_page+0x3aa/0x1c90
     [<ffffffff815aa4f8>] ? handle_mm_fault+0xbc8/0x27f0
     [<ffffffff815a9930>] ? __pmd_alloc+0x370/0x370
     [<ffffffff82c870d0>] ? snd_timer_pause+0x80/0x80
     [<ffffffff816b0733>] do_vfs_ioctl+0x193/0x1050
     [<ffffffff816b05a0>] ? ioctl_preallocate+0x200/0x200
     [<ffffffff81002f2f>] ? syscall_trace_enter+0x3cf/0xdb0
     [<ffffffff815045ba>] ? __context_tracking_exit.part.4+0x9a/0x1e0
     [<ffffffff81002b60>] ? exit_to_usermode_loop+0x190/0x190
     [<ffffffff82001a97>] ? check_preemption_disabled+0x37/0x1e0
     [<ffffffff81d93889>] ? security_file_ioctl+0x89/0xb0
     [<ffffffff816b167f>] SyS_ioctl+0x8f/0xc0
     [<ffffffff816b15f0>] ? do_vfs_ioctl+0x1050/0x1050
     [<ffffffff81005524>] do_syscall_64+0x1c4/0x4e0
     [<ffffffff83c32b2a>] entry_SYSCALL64_slow_path+0x25/0x25
    Code: e8 fc 42 7b fe 8b 0d 06 8a 50 03 49 0f af cf 48 85 c9 0f 88 7c 01 00 00 48 89 4d a8 e8 e0 42 7b fe 48 8b 45 c0 48 8b 4d a8 48 99 <48> f7 f9 49 01 c7 e8 cb 42 7b fe 48 8b 55 d0 48 b8 00 00 00 00
    RIP  [<ffffffff82c8bd9a>] snd_hrtimer_callback+0x1da/0x3f0
     RSP <ffff88011aa87da8>
    ---[ end trace 6aa380f756a21074 ]---

The problem happens when you call ioctl(SNDRV_TIMER_IOCTL_CONTINUE) on a
completely new/unused timer -- it will have ->sticks == 0, which causes a
divide by 0 in snd_hrtimer_callback().

Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 sound/core/timer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/core/timer.c b/sound/core/timer.c
index 7899e37..faa18f4 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -813,6 +813,7 @@ int snd_timer_new(struct snd_card *card, char *id, struct snd_timer_id *tid,
 	timer->tmr_subdevice = tid->subdevice;
 	if (id)
 		strlcpy(timer->id, id, sizeof(timer->id));
+	timer->sticks = 1;
 	INIT_LIST_HEAD(&timer->device_list);
 	INIT_LIST_HEAD(&timer->open_list_head);
 	INIT_LIST_HEAD(&timer->active_list_head);
-- 
2.10.0.rc0.1.g07c9292

[PATCH 3/3] ALSA: timer: fix NULL pointer dereference on memory allocation failure

[Vegard Nossum]

I hit this with syzkaller:

    kasan: CONFIG_KASAN_INLINE enabled
    kasan: GPF could be caused by NULL-ptr deref or user memory access
    general protection fault: 0000 [#1] PREEMPT SMP KASAN
    CPU: 0 PID: 1327 Comm: a.out Not tainted 4.8.0-rc2+ #190
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.3-0-ge2fc41e-prebuilt.qemu-project.org 04/01/2014
    task: ffff88011278d600 task.stack: ffff8801120c0000
    RIP: 0010:[<ffffffff82c8ba07>]  [<ffffffff82c8ba07>] snd_hrtimer_start+0x77/0x100
    RSP: 0018:ffff8801120c7a60  EFLAGS: 00010006
    RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000007
    RDX: 0000000000000009 RSI: 1ffff10023483091 RDI: 0000000000000048
    RBP: ffff8801120c7a78 R08: ffff88011a5cf768 R09: ffff88011a5ba790
    R10: 0000000000000002 R11: ffffed00234b9ef1 R12: ffff880114843980
    R13: ffffffff84213c00 R14: ffff880114843ab0 R15: 0000000000000286
    FS:  00007f72958f3700(0000) GS:ffff88011aa00000(0000) knlGS:0000000000000000
    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    CR2: 0000000000603001 CR3: 00000001126ab000 CR4: 00000000000006f0
    Stack:
     ffff880114843980 ffff880111eb2dc0 ffff880114843a34 ffff8801120c7ad0
     ffffffff82c81ab1 0000000000000000 ffffffff842138e0 0000000100000000
     ffff880111eb2dd0 ffff880111eb2dc0 0000000000000001 ffff880111eb2dc0
    Call Trace:
     [<ffffffff82c81ab1>] snd_timer_start1+0x331/0x670
     [<ffffffff82c85bfd>] snd_timer_start+0x5d/0xa0
     [<ffffffff82c8795e>] snd_timer_user_ioctl+0x88e/0x2830
     [<ffffffff8159f3a0>] ? __follow_pte.isra.49+0x430/0x430
     [<ffffffff82c870d0>] ? snd_timer_pause+0x80/0x80
     [<ffffffff815a26fa>] ? do_wp_page+0x3aa/0x1c90
     [<ffffffff8132762f>] ? put_prev_entity+0x108f/0x21a0
     [<ffffffff82c870d0>] ? snd_timer_pause+0x80/0x80
     [<ffffffff816b0733>] do_vfs_ioctl+0x193/0x1050
     [<ffffffff813510af>] ? cpuacct_account_field+0x12f/0x1a0
     [<ffffffff816b05a0>] ? ioctl_preallocate+0x200/0x200
     [<ffffffff81002f2f>] ? syscall_trace_enter+0x3cf/0xdb0
     [<ffffffff815045ba>] ? __context_tracking_exit.part.4+0x9a/0x1e0
     [<ffffffff81002b60>] ? exit_to_usermode_loop+0x190/0x190
     [<ffffffff82001a97>] ? check_preemption_disabled+0x37/0x1e0
     [<ffffffff81d93889>] ? security_file_ioctl+0x89/0xb0
     [<ffffffff816b167f>] SyS_ioctl+0x8f/0xc0
     [<ffffffff816b15f0>] ? do_vfs_ioctl+0x1050/0x1050
     [<ffffffff81005524>] do_syscall_64+0x1c4/0x4e0
     [<ffffffff83c32b2a>] entry_SYSCALL64_slow_path+0x25/0x25
    Code: c7 c7 c4 b9 c8 82 48 89 d9 4c 89 ee e8 63 88 7f fe e8 7e 46 7b fe 48 8d 7b 48 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 04 84 c0 7e 65 80 7b 48 00 74 0e e8 52 46
    RIP  [<ffffffff82c8ba07>] snd_hrtimer_start+0x77/0x100
     RSP <ffff8801120c7a60>
    ---[ end trace 5955b08db7f2b029 ]---

This can happen if snd_hrtimer_open() fails to allocate memory and
returns an error, which is currently not checked by snd_timer_open():

    ioctl(SNDRV_TIMER_IOCTL_SELECT)
     - snd_timer_user_tselect()
	- snd_timer_close()
	   - snd_hrtimer_close()
	      - (struct snd_timer *) t->private_data = NULL
        - snd_timer_open()
           - snd_hrtimer_open()
              - kzalloc() fails; t->private_data is still NULL

    ioctl(SNDRV_TIMER_IOCTL_START)
     - snd_timer_user_start()
	- snd_timer_start()
	   - snd_timer_start1()
	      - snd_hrtimer_start()
		- t->private_data == NULL // boom

Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 sound/core/timer.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/sound/core/timer.c b/sound/core/timer.c
index faa18f4..056d757 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -294,8 +294,21 @@ int snd_timer_open(struct snd_timer_instance **ti,
 		get_device(&timer->card->card_dev);
 	timeri->slave_class = tid->dev_sclass;
 	timeri->slave_id = slave_id;
-	if (list_empty(&timer->open_list_head) && timer->hw.open)
-		timer->hw.open(timer);
+
+	if (list_empty(&timer->open_list_head) && timer->hw.open) {
+		int err = timer->hw.open(timer);
+		if (err) {
+			kfree(timeri->owner);
+			kfree(timeri);
+
+			if (timer->card)
+				put_device(&timer->card->card_dev);
+			module_put(timer->module);
+			mutex_unlock(&register_mutex);
+			return err;
+		}
+	}
+
 	list_add_tail(&timeri->open_list, &timer->open_list_head);
 	snd_timer_check_master(timeri);
 	mutex_unlock(&register_mutex);
-- 
2.10.0.rc0.1.g07c9292

Re: [PATCH 1/3] ALSA: timer: fix NULL pointer dereference in read()/ioctl() race

[Takashi Iwai]

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@oracle.com>
> ---
>  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

Re: [PATCH 2/3] ALSA: timer: fix division by zero after SNDRV_TIMER_IOCTL_CONTINUE

[Takashi Iwai]

On Mon, 29 Aug 2016 00:33:50 +0200,
Vegard Nossum wrote:
> 
> I got this:
> 
>     divide error: 0000 [#1] PREEMPT SMP KASAN
>     CPU: 1 PID: 1327 Comm: a.out Not tainted 4.8.0-rc2+ #189
>     Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.3-0-ge2fc41e-prebuilt.qemu-project.org 04/01/2014
>     task: ffff8801120a9580 task.stack: ffff8801120b0000
>     RIP: 0010:[<ffffffff82c8bd9a>]  [<ffffffff82c8bd9a>] snd_hrtimer_callback+0x1da/0x3f0
>     RSP: 0018:ffff88011aa87da8  EFLAGS: 00010006
>     RAX: 0000000000004f76 RBX: ffff880112655e88 RCX: 0000000000000000
>     RDX: 0000000000000000 RSI: ffff880112655ea0 RDI: 0000000000000001
>     RBP: ffff88011aa87e00 R08: ffff88013fff905c R09: ffff88013fff9048
>     R10: ffff88013fff9050 R11: 00000001050a7b8c R12: ffff880114778a00
>     R13: ffff880114778ab4 R14: ffff880114778b30 R15: 0000000000000000
>     FS:  00007f071647c700(0000) GS:ffff88011aa80000(0000) knlGS:0000000000000000
>     CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>     CR2: 0000000000603001 CR3: 0000000112021000 CR4: 00000000000006e0
>     Stack:
>      0000000000000000 ffff880114778ab8 ffff880112655ea0 0000000000004f76
>      ffff880112655ec8 ffff880112655e80 ffff880112655e88 ffff88011aa98fc0
>      00000000b97ccf2b dffffc0000000000 ffff88011aa98fc0 ffff88011aa87ef0
>     Call Trace:
>      <IRQ>
>      [<ffffffff813abce7>] __hrtimer_run_queues+0x347/0xa00
>      [<ffffffff82c8bbc0>] ? snd_hrtimer_close+0x130/0x130
>      [<ffffffff813ab9a0>] ? retrigger_next_event+0x1b0/0x1b0
>      [<ffffffff813ae1a6>] ? hrtimer_interrupt+0x136/0x4b0
>      [<ffffffff813ae220>] hrtimer_interrupt+0x1b0/0x4b0
>      [<ffffffff8120f91e>] local_apic_timer_interrupt+0x6e/0xf0
>      [<ffffffff81227ad3>] ? kvm_guest_apic_eoi_write+0x13/0xc0
>      [<ffffffff83c35086>] smp_apic_timer_interrupt+0x76/0xa0
>      [<ffffffff83c3416c>] apic_timer_interrupt+0x8c/0xa0
>      <EOI>
>      [<ffffffff83c3239c>] ? _raw_spin_unlock_irqrestore+0x2c/0x60
>      [<ffffffff82c8185d>] snd_timer_start1+0xdd/0x670
>      [<ffffffff82c87015>] snd_timer_continue+0x45/0x80
>      [<ffffffff82c88100>] snd_timer_user_ioctl+0x1030/0x2830
>      [<ffffffff8159f3a0>] ? __follow_pte.isra.49+0x430/0x430
>      [<ffffffff82c870d0>] ? snd_timer_pause+0x80/0x80
>      [<ffffffff815a26fa>] ? do_wp_page+0x3aa/0x1c90
>      [<ffffffff815aa4f8>] ? handle_mm_fault+0xbc8/0x27f0
>      [<ffffffff815a9930>] ? __pmd_alloc+0x370/0x370
>      [<ffffffff82c870d0>] ? snd_timer_pause+0x80/0x80
>      [<ffffffff816b0733>] do_vfs_ioctl+0x193/0x1050
>      [<ffffffff816b05a0>] ? ioctl_preallocate+0x200/0x200
>      [<ffffffff81002f2f>] ? syscall_trace_enter+0x3cf/0xdb0
>      [<ffffffff815045ba>] ? __context_tracking_exit.part.4+0x9a/0x1e0
>      [<ffffffff81002b60>] ? exit_to_usermode_loop+0x190/0x190
>      [<ffffffff82001a97>] ? check_preemption_disabled+0x37/0x1e0
>      [<ffffffff81d93889>] ? security_file_ioctl+0x89/0xb0
>      [<ffffffff816b167f>] SyS_ioctl+0x8f/0xc0
>      [<ffffffff816b15f0>] ? do_vfs_ioctl+0x1050/0x1050
>      [<ffffffff81005524>] do_syscall_64+0x1c4/0x4e0
>      [<ffffffff83c32b2a>] entry_SYSCALL64_slow_path+0x25/0x25
>     Code: e8 fc 42 7b fe 8b 0d 06 8a 50 03 49 0f af cf 48 85 c9 0f 88 7c 01 00 00 48 89 4d a8 e8 e0 42 7b fe 48 8b 45 c0 48 8b 4d a8 48 99 <48> f7 f9 49 01 c7 e8 cb 42 7b fe 48 8b 55 d0 48 b8 00 00 00 00
>     RIP  [<ffffffff82c8bd9a>] snd_hrtimer_callback+0x1da/0x3f0
>      RSP <ffff88011aa87da8>
>     ---[ end trace 6aa380f756a21074 ]---
> 
> The problem happens when you call ioctl(SNDRV_TIMER_IOCTL_CONTINUE) on a
> completely new/unused timer -- it will have ->sticks == 0, which causes a
> divide by 0 in snd_hrtimer_callback().
> 
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>

Applied with Cc to stable.  Thanks.


Takashi


> ---
>  sound/core/timer.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/sound/core/timer.c b/sound/core/timer.c
> index 7899e37..faa18f4 100644
> --- a/sound/core/timer.c
> +++ b/sound/core/timer.c
> @@ -813,6 +813,7 @@ int snd_timer_new(struct snd_card *card, char *id, struct snd_timer_id *tid,
>  	timer->tmr_subdevice = tid->subdevice;
>  	if (id)
>  		strlcpy(timer->id, id, sizeof(timer->id));
> +	timer->sticks = 1;
>  	INIT_LIST_HEAD(&timer->device_list);
>  	INIT_LIST_HEAD(&timer->open_list_head);
>  	INIT_LIST_HEAD(&timer->active_list_head);
> -- 
> 2.10.0.rc0.1.g07c9292
> 
> 

Re: [PATCH 3/3] ALSA: timer: fix NULL pointer dereference on memory allocation failure

[Takashi Iwai]

On Mon, 29 Aug 2016 00:33:51 +0200,
Vegard Nossum wrote:
> 
> I hit this with syzkaller:
> 
>     kasan: CONFIG_KASAN_INLINE enabled
>     kasan: GPF could be caused by NULL-ptr deref or user memory access
>     general protection fault: 0000 [#1] PREEMPT SMP KASAN
>     CPU: 0 PID: 1327 Comm: a.out Not tainted 4.8.0-rc2+ #190
>     Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.3-0-ge2fc41e-prebuilt.qemu-project.org 04/01/2014
>     task: ffff88011278d600 task.stack: ffff8801120c0000
>     RIP: 0010:[<ffffffff82c8ba07>]  [<ffffffff82c8ba07>] snd_hrtimer_start+0x77/0x100
>     RSP: 0018:ffff8801120c7a60  EFLAGS: 00010006
>     RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000007
>     RDX: 0000000000000009 RSI: 1ffff10023483091 RDI: 0000000000000048
>     RBP: ffff8801120c7a78 R08: ffff88011a5cf768 R09: ffff88011a5ba790
>     R10: 0000000000000002 R11: ffffed00234b9ef1 R12: ffff880114843980
>     R13: ffffffff84213c00 R14: ffff880114843ab0 R15: 0000000000000286
>     FS:  00007f72958f3700(0000) GS:ffff88011aa00000(0000) knlGS:0000000000000000
>     CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>     CR2: 0000000000603001 CR3: 00000001126ab000 CR4: 00000000000006f0
>     Stack:
>      ffff880114843980 ffff880111eb2dc0 ffff880114843a34 ffff8801120c7ad0
>      ffffffff82c81ab1 0000000000000000 ffffffff842138e0 0000000100000000
>      ffff880111eb2dd0 ffff880111eb2dc0 0000000000000001 ffff880111eb2dc0
>     Call Trace:
>      [<ffffffff82c81ab1>] snd_timer_start1+0x331/0x670
>      [<ffffffff82c85bfd>] snd_timer_start+0x5d/0xa0
>      [<ffffffff82c8795e>] snd_timer_user_ioctl+0x88e/0x2830
>      [<ffffffff8159f3a0>] ? __follow_pte.isra.49+0x430/0x430
>      [<ffffffff82c870d0>] ? snd_timer_pause+0x80/0x80
>      [<ffffffff815a26fa>] ? do_wp_page+0x3aa/0x1c90
>      [<ffffffff8132762f>] ? put_prev_entity+0x108f/0x21a0
>      [<ffffffff82c870d0>] ? snd_timer_pause+0x80/0x80
>      [<ffffffff816b0733>] do_vfs_ioctl+0x193/0x1050
>      [<ffffffff813510af>] ? cpuacct_account_field+0x12f/0x1a0
>      [<ffffffff816b05a0>] ? ioctl_preallocate+0x200/0x200
>      [<ffffffff81002f2f>] ? syscall_trace_enter+0x3cf/0xdb0
>      [<ffffffff815045ba>] ? __context_tracking_exit.part.4+0x9a/0x1e0
>      [<ffffffff81002b60>] ? exit_to_usermode_loop+0x190/0x190
>      [<ffffffff82001a97>] ? check_preemption_disabled+0x37/0x1e0
>      [<ffffffff81d93889>] ? security_file_ioctl+0x89/0xb0
>      [<ffffffff816b167f>] SyS_ioctl+0x8f/0xc0
>      [<ffffffff816b15f0>] ? do_vfs_ioctl+0x1050/0x1050
>      [<ffffffff81005524>] do_syscall_64+0x1c4/0x4e0
>      [<ffffffff83c32b2a>] entry_SYSCALL64_slow_path+0x25/0x25
>     Code: c7 c7 c4 b9 c8 82 48 89 d9 4c 89 ee e8 63 88 7f fe e8 7e 46 7b fe 48 8d 7b 48 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 04 84 c0 7e 65 80 7b 48 00 74 0e e8 52 46
>     RIP  [<ffffffff82c8ba07>] snd_hrtimer_start+0x77/0x100
>      RSP <ffff8801120c7a60>
>     ---[ end trace 5955b08db7f2b029 ]---
> 
> This can happen if snd_hrtimer_open() fails to allocate memory and
> returns an error, which is currently not checked by snd_timer_open():
> 
>     ioctl(SNDRV_TIMER_IOCTL_SELECT)
>      - snd_timer_user_tselect()
> 	- snd_timer_close()
> 	   - snd_hrtimer_close()
> 	      - (struct snd_timer *) t->private_data = NULL
>         - snd_timer_open()
>            - snd_hrtimer_open()
>               - kzalloc() fails; t->private_data is still NULL
> 
>     ioctl(SNDRV_TIMER_IOCTL_START)
>      - snd_timer_user_start()
> 	- snd_timer_start()
> 	   - snd_timer_start1()
> 	      - snd_hrtimer_start()
> 		- t->private_data == NULL // boom
> 
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>

Applied this one with Cc to stable, too.  Thanks.


Takashi


> ---
>  sound/core/timer.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/core/timer.c b/sound/core/timer.c
> index faa18f4..056d757 100644
> --- a/sound/core/timer.c
> +++ b/sound/core/timer.c
> @@ -294,8 +294,21 @@ int snd_timer_open(struct snd_timer_instance **ti,
>  		get_device(&timer->card->card_dev);
>  	timeri->slave_class = tid->dev_sclass;
>  	timeri->slave_id = slave_id;
> -	if (list_empty(&timer->open_list_head) && timer->hw.open)
> -		timer->hw.open(timer);
> +
> +	if (list_empty(&timer->open_list_head) && timer->hw.open) {
> +		int err = timer->hw.open(timer);
> +		if (err) {
> +			kfree(timeri->owner);
> +			kfree(timeri);
> +
> +			if (timer->card)
> +				put_device(&timer->card->card_dev);
> +			module_put(timer->module);
> +			mutex_unlock(&register_mutex);
> +			return err;
> +		}
> +	}
> +
>  	list_add_tail(&timeri->open_list, &timer->open_list_head);
>  	snd_timer_check_master(timeri);
>  	mutex_unlock(&register_mutex);
> -- 
> 2.10.0.rc0.1.g07c9292
> 
> 

Re: [PATCH 1/3] ALSA: timer: fix NULL pointer dereference in read()/ioctl() race

[Vegard Nossum]

[-- Attachment #1: Type: text/plain, Size: 938 bytes --]

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

[-- Attachment #2: 0001-ALSA-timer-fix-NULL-pointer-dereference-in-read-ioct.patch --]
[-- Type: text/x-patch, Size: 4170 bytes --]

>From fa0c216554c991347fb9a0c86832d45c63343e28 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@oracle.com>
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:
     [<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 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 <vegard.nossum@oracle.com>
---
 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

Re: [PATCH 1/3] ALSA: timer: fix NULL pointer dereference in read()/ioctl() race

[Vegard Nossum]

On 08/29/2016 09:14 AM, Vegard Nossum wrote:
> 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.

I tested the new patch (from the email I'm replying to). It seems to
work for me.

Thanks,


Vegard

Re: [PATCH 1/3] ALSA: timer: fix NULL pointer dereference in read()/ioctl() race

[Takashi Iwai]

On Fri, 02 Sep 2016 14:34:47 +0200,
Vegard Nossum wrote:
> 
> On 08/29/2016 09:14 AM, Vegard Nossum wrote:
> > 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.
> 
> I tested the new patch (from the email I'm replying to). It seems to
> work for me.

OK, queued now.  Thanks!


Takashi