Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1471488 > unrolled thread

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

Started byVegard Nossum <vegard.nossum@oracle.com>
First post2016-08-29 00:40 +0200
Last post2016-09-02 15:20 +0200
Articles 7 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/3] ALSA: timer: fix NULL pointer dereference in read()/ioctl() race Vegard Nossum <vegard.nossum@oracle.com> - 2016-08-29 00:40 +0200
    [PATCH 2/3] ALSA: timer: fix division by zero after SNDRV_TIMER_IOCTL_CONTINUE Vegard Nossum <vegard.nossum@oracle.com> - 2016-08-29 00:40 +0200
      Re: [PATCH 2/3] ALSA: timer: fix division by zero after SNDRV_TIMER_IOCTL_CONTINUE Takashi Iwai <tiwai@suse.de> - 2016-08-29 09:10 +0200
    Re: [PATCH 1/3] ALSA: timer: fix NULL pointer dereference in read()/ioctl() race Takashi Iwai <tiwai@suse.de> - 2016-08-29 09:10 +0200
      Re: [PATCH 1/3] ALSA: timer: fix NULL pointer dereference in  read()/ioctl() race Vegard Nossum <vegard.nossum@oracle.com> - 2016-08-29 09:20 +0200
        Re: [PATCH 1/3] ALSA: timer: fix NULL pointer dereference in  read()/ioctl() race Vegard Nossum <vegard.nossum@oracle.com> - 2016-09-02 14:40 +0200
          Re: [PATCH 1/3] ALSA: timer: fix NULL pointer dereference in read()/ioctl() race Takashi Iwai <tiwai@suse.de> - 2016-09-02 15:20 +0200

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

FromVegard Nossum <vegard.nossum@oracle.com>
Date2016-08-29 00:40 +0200
Subject[PATCH 1/3] ALSA: timer: fix NULL pointer dereference in read()/ioctl() race
Message-ID<sbgU9-7Cf-3@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

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

FromVegard Nossum <vegard.nossum@oracle.com>
Date2016-08-29 00:40 +0200
Subject[PATCH 2/3] ALSA: timer: fix division by zero after SNDRV_TIMER_IOCTL_CONTINUE
Message-ID<sbgUa-7Cf-25@gated-at.bofh.it>
In reply to#1471488
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

[toc] | [prev] | [next] | [standalone]

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

FromTakashi Iwai <tiwai@suse.de>
Date2016-08-29 09:10 +0200
SubjectRe: [PATCH 2/3] ALSA: timer: fix division by zero after SNDRV_TIMER_IOCTL_CONTINUE
Message-ID<sboRI-4fp-17@gated-at.bofh.it>
In reply to#1471489
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
> 
>

[toc] | [prev] | [next] | [standalone]

#1471583

FromTakashi Iwai <tiwai@suse.de>
Date2016-08-29 09:10 +0200
Message-ID<sboRH-4fp-1@gated-at.bofh.it>
In reply to#1471488
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

[toc] | [prev] | [next] | [standalone]

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

FromVegard Nossum <vegard.nossum@oracle.com>
Date2016-08-29 09:20 +0200
SubjectRe: [PATCH 1/3] ALSA: timer: fix NULL pointer dereference in read()/ioctl() race
Message-ID<sbp1n-4ix-5@gated-at.bofh.it>
In reply to#1471583

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

FromVegard Nossum <vegard.nossum@oracle.com>
Date2016-09-02 14:40 +0200
SubjectRe: [PATCH 1/3] ALSA: timer: fix NULL pointer dereference in read()/ioctl() race
Message-ID<scVVf-7Vy-13@gated-at.bofh.it>
In reply to#1471587
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

[toc] | [prev] | [next] | [standalone]

#1475141

FromTakashi Iwai <tiwai@suse.de>
Date2016-09-02 15:20 +0200
Message-ID<scWxX-8o5-7@gated-at.bofh.it>
In reply to#1475109
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web