From 6b760bb2c63a9e322c0e4a0b5daf335ad93d5a33 Mon Sep 17 00:00:00 2001 From: Vegard Nossum Date: Mon, 29 Aug 2016 00:33:50 +0200 Subject: ALSA: timer: fix division by zero after SNDRV_TIMER_IOCTL_CONTINUE 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:[] [] 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: [] __hrtimer_run_queues+0x347/0xa00 [] ? snd_hrtimer_close+0x130/0x130 [] ? retrigger_next_event+0x1b0/0x1b0 [] ? hrtimer_interrupt+0x136/0x4b0 [] hrtimer_interrupt+0x1b0/0x4b0 [] local_apic_timer_interrupt+0x6e/0xf0 [] ? kvm_guest_apic_eoi_write+0x13/0xc0 [] smp_apic_timer_interrupt+0x76/0xa0 [] apic_timer_interrupt+0x8c/0xa0 [] ? _raw_spin_unlock_irqrestore+0x2c/0x60 [] snd_timer_start1+0xdd/0x670 [] snd_timer_continue+0x45/0x80 [] snd_timer_user_ioctl+0x1030/0x2830 [] ? __follow_pte.isra.49+0x430/0x430 [] ? snd_timer_pause+0x80/0x80 [] ? do_wp_page+0x3aa/0x1c90 [] ? handle_mm_fault+0xbc8/0x27f0 [] ? __pmd_alloc+0x370/0x370 [] ? snd_timer_pause+0x80/0x80 [] do_vfs_ioctl+0x193/0x1050 [] ? ioctl_preallocate+0x200/0x200 [] ? syscall_trace_enter+0x3cf/0xdb0 [] ? __context_tracking_exit.part.4+0x9a/0x1e0 [] ? exit_to_usermode_loop+0x190/0x190 [] ? check_preemption_disabled+0x37/0x1e0 [] ? security_file_ioctl+0x89/0xb0 [] SyS_ioctl+0x8f/0xc0 [] ? do_vfs_ioctl+0x1050/0x1050 [] do_syscall_64+0x1c4/0x4e0 [] 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 [] snd_hrtimer_callback+0x1da/0x3f0 RSP ---[ 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 Cc: Signed-off-by: Takashi Iwai --- sound/core/timer.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sound/core/timer.c b/sound/core/timer.c index 9a6157ea6881..00618703c58f 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); -- cgit v1.2.3-59-g8ed1b From 8ddc05638ee42b18ba4fe99b5fb647fa3ad20456 Mon Sep 17 00:00:00 2001 From: Vegard Nossum Date: Mon, 29 Aug 2016 00:33:51 +0200 Subject: ALSA: timer: fix NULL pointer dereference on memory allocation failure 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:[] [] 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: [] snd_timer_start1+0x331/0x670 [] snd_timer_start+0x5d/0xa0 [] snd_timer_user_ioctl+0x88e/0x2830 [] ? __follow_pte.isra.49+0x430/0x430 [] ? snd_timer_pause+0x80/0x80 [] ? do_wp_page+0x3aa/0x1c90 [] ? put_prev_entity+0x108f/0x21a0 [] ? snd_timer_pause+0x80/0x80 [] do_vfs_ioctl+0x193/0x1050 [] ? cpuacct_account_field+0x12f/0x1a0 [] ? ioctl_preallocate+0x200/0x200 [] ? syscall_trace_enter+0x3cf/0xdb0 [] ? __context_tracking_exit.part.4+0x9a/0x1e0 [] ? exit_to_usermode_loop+0x190/0x190 [] ? check_preemption_disabled+0x37/0x1e0 [] ? security_file_ioctl+0x89/0xb0 [] SyS_ioctl+0x8f/0xc0 [] ? do_vfs_ioctl+0x1050/0x1050 [] do_syscall_64+0x1c4/0x4e0 [] 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 [] snd_hrtimer_start+0x77/0x100 RSP ---[ 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 Cc: Signed-off-by: Takashi Iwai --- 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 00618703c58f..2d6e3e76ddf5 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(®ister_mutex); + return err; + } + } + list_add_tail(&timeri->open_list, &timer->open_list_head); snd_timer_check_master(timeri); mutex_unlock(®ister_mutex); -- cgit v1.2.3-59-g8ed1b From 83d9956b7e6b310c1062df7894257251c625b22e Mon Sep 17 00:00:00 2001 From: Ken Lin Date: Fri, 12 Aug 2016 14:08:47 -0400 Subject: ALSA: usb-audio: Add sample rate inquiry quirk for B850V3 CP2114 Avoid getting sample rate on B850V3 CP2114 as it is unsupported and causes noisy "current rate is different from the runtime rate" messages when playback starts. Signed-off-by: Ken Lin Signed-off-by: Akshay Bhat Cc: Signed-off-by: Takashi Iwai --- sound/usb/quirks.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c index 6cf1f3597455..152292e5ee2b 100644 --- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -1141,6 +1141,7 @@ bool snd_usb_get_sample_rate_quirk(struct snd_usb_audio *chip) case USB_ID(0x0556, 0x0014): /* Phoenix Audio TMX320VC */ case USB_ID(0x05A3, 0x9420): /* ELP HD USB Camera */ case USB_ID(0x074D, 0x3553): /* Outlaw RR2150 (Micronas UAC3553B) */ + case USB_ID(0x1901, 0x0191): /* GE B850V3 CP2114 audio interface */ case USB_ID(0x1de7, 0x0013): /* Phoenix Audio MT202exe */ case USB_ID(0x1de7, 0x0014): /* Phoenix Audio TMX320 */ case USB_ID(0x1de7, 0x0114): /* Phoenix Audio MT202pcs */ -- cgit v1.2.3-59-g8ed1b From 311042d1b67d9a1856a8e1294e7729fb86f64014 Mon Sep 17 00:00:00 2001 From: Shrirang Bagul Date: Mon, 29 Aug 2016 15:19:27 +0800 Subject: ALSA: hda - Add headset mic quirk for Dell Inspiron 5468 This patch enables headset microphone on some variants of Dell Inspiron 5468. (Dell SSID 0x07ad) BugLink: https://bugs.launchpad.net/bugs/1617900 Signed-off-by: Shrirang Bagul Cc: Signed-off-by: Takashi Iwai --- sound/pci/hda/patch_realtek.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 7100f05e651a..02fd25277c10 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -5895,6 +5895,10 @@ static const struct snd_hda_pin_quirk alc269_pin_fixup_tbl[] = { {0x12, 0x90a60170}, {0x14, 0x90170120}, {0x21, 0x02211030}), + SND_HDA_PIN_QUIRK(0x10ec0256, 0x1028, "Dell Inspiron 5468", ALC255_FIXUP_DELL1_MIC_NO_PRESENCE, + {0x12, 0x90a60180}, + {0x14, 0x90170120}, + {0x21, 0x02211030}), SND_HDA_PIN_QUIRK(0x10ec0256, 0x1028, "Dell", ALC255_FIXUP_DELL1_MIC_NO_PRESENCE, ALC256_STANDARD_PINS), SND_HDA_PIN_QUIRK(0x10ec0280, 0x103c, "HP", ALC280_FIXUP_HP_GPIO4, -- cgit v1.2.3-59-g8ed1b From fd06c77eb9200b53d421da5fffe0dcd894b5d72a Mon Sep 17 00:00:00 2001 From: Kai-Heng Feng Date: Tue, 30 Aug 2016 15:36:34 +0800 Subject: ALSA: hda - Enable subwoofer on Dell Inspiron 7559 The subwoofer on Inspiron 7559 was disabled originally. Applying a pin fixup to node 0x1b can enable it and make it work. Old pin: 0x411111f0 New pin: 0x90170151 Signed-off-by: Kai-Heng Feng Cc: Signed-off-by: Takashi Iwai --- sound/pci/hda/patch_realtek.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 02fd25277c10..575cefd8cc4a 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -4855,6 +4855,7 @@ enum { ALC221_FIXUP_HP_FRONT_MIC, ALC292_FIXUP_TPT460, ALC298_FIXUP_SPK_VOLUME, + ALC256_FIXUP_DELL_INSPIRON_7559_SUBWOOFER, }; static const struct hda_fixup alc269_fixups[] = { @@ -5516,6 +5517,15 @@ static const struct hda_fixup alc269_fixups[] = { .chained = true, .chain_id = ALC298_FIXUP_DELL1_MIC_NO_PRESENCE, }, + [ALC256_FIXUP_DELL_INSPIRON_7559_SUBWOOFER] = { + .type = HDA_FIXUP_PINS, + .v.pins = (const struct hda_pintbl[]) { + { 0x1b, 0x90170151 }, + { } + }, + .chained = true, + .chain_id = ALC255_FIXUP_DELL1_MIC_NO_PRESENCE + }, }; static const struct snd_pci_quirk alc269_fixup_tbl[] = { @@ -5560,6 +5570,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = { SND_PCI_QUIRK(0x1028, 0x06df, "Dell", ALC293_FIXUP_DISABLE_AAMIX_MULTIJACK), SND_PCI_QUIRK(0x1028, 0x06e0, "Dell", ALC293_FIXUP_DISABLE_AAMIX_MULTIJACK), SND_PCI_QUIRK(0x1028, 0x0704, "Dell XPS 13 9350", ALC256_FIXUP_DELL_XPS_13_HEADPHONE_NOISE), + SND_PCI_QUIRK(0x1028, 0x0706, "Dell Inspiron 7559", ALC256_FIXUP_DELL_INSPIRON_7559_SUBWOOFER), SND_PCI_QUIRK(0x1028, 0x0725, "Dell Inspiron 3162", ALC255_FIXUP_DELL_SPK_NOISE), SND_PCI_QUIRK(0x1028, 0x075b, "Dell XPS 13 9360", ALC256_FIXUP_DELL_XPS_13_HEADPHONE_NOISE), SND_PCI_QUIRK(0x1028, 0x075d, "Dell AIO", ALC298_FIXUP_SPK_VOLUME), -- cgit v1.2.3-59-g8ed1b From 04b2d9c9c319277ad4fbbb71855c256a9f4d5f98 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Wed, 31 Aug 2016 20:15:32 +0900 Subject: ALSA: firewire-tascam: accessing to user space outside spinlock In hwdep interface of firewire-tascam driver, accessing to user space is in a critical section with disabled local interrupt. Depending on architecture, accessing to user space can cause page fault exception. Then local processor stores machine status and handle the synchronous event. A handler corresponding to the event can call task scheduler to wait for preparing pages. In a case of usage of single core processor, the state to disable local interrupt is worse because it doesn't handle usual interrupts from hardware. This commit fixes this bug, by performing the accessing outside spinlock. Reported-by: Vaishali Thakkar Cc: stable@vger.kernel.org Fixes: e5e0c3dd257b('ALSA: firewire-tascam: add hwdep interface') Signed-off-by: Takashi Sakamoto Signed-off-by: Takashi Iwai --- sound/firewire/tascam/tascam-hwdep.c | 33 +++++++++++---------------------- 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/sound/firewire/tascam/tascam-hwdep.c b/sound/firewire/tascam/tascam-hwdep.c index 131267c3a042..106406cbfaa3 100644 --- a/sound/firewire/tascam/tascam-hwdep.c +++ b/sound/firewire/tascam/tascam-hwdep.c @@ -16,31 +16,14 @@ #include "tascam.h" -static long hwdep_read_locked(struct snd_tscm *tscm, char __user *buf, - long count) -{ - union snd_firewire_event event; - - memset(&event, 0, sizeof(event)); - - event.lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS; - event.lock_status.status = (tscm->dev_lock_count > 0); - tscm->dev_lock_changed = false; - - count = min_t(long, count, sizeof(event.lock_status)); - - if (copy_to_user(buf, &event, count)) - return -EFAULT; - - return count; -} - static long hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count, loff_t *offset) { struct snd_tscm *tscm = hwdep->private_data; DEFINE_WAIT(wait); - union snd_firewire_event event; + union snd_firewire_event event = { + .lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS, + }; spin_lock_irq(&tscm->lock); @@ -54,10 +37,16 @@ static long hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count, spin_lock_irq(&tscm->lock); } - memset(&event, 0, sizeof(event)); - count = hwdep_read_locked(tscm, buf, count); + event.lock_status.status = (tscm->dev_lock_count > 0); + tscm->dev_lock_changed = false; + spin_unlock_irq(&tscm->lock); + count = min_t(long, count, sizeof(event.lock_status)); + + if (copy_to_user(buf, &event, count)) + return -EFAULT; + return count; } -- cgit v1.2.3-59-g8ed1b From 6b1ca4bcadf9ef077cc5f03c6822ba276ed14902 Mon Sep 17 00:00:00 2001 From: Takashi Sakamoto Date: Wed, 31 Aug 2016 22:58:42 +0900 Subject: ALSA: fireworks: accessing to user space outside spinlock In hwdep interface of fireworks driver, accessing to user space is in a critical section with disabled local interrupt. Depending on architecture, accessing to user space can cause page fault exception. Then local processor stores machine status and handles the synchronous event. A handler corresponding to the event can call task scheduler to wait for preparing pages. In a case of usage of single core processor, the state to disable local interrupt is worse because it don't handle usual interrupts from hardware. This commit fixes this bug, performing the accessing outside spinlock. This commit also gives up counting the number of queued response messages to simplify ring-buffer management. Reported-by: Vaishali Thakkar Cc: stable@vger.kernel.org Fixes: 555e8a8f7f14('ALSA: fireworks: Add command/response functionality into hwdep interface') Signed-off-by: Takashi Sakamoto Signed-off-by: Takashi Iwai --- sound/firewire/fireworks/fireworks.h | 1 - sound/firewire/fireworks/fireworks_hwdep.c | 71 +++++++++++++++++------- sound/firewire/fireworks/fireworks_proc.c | 4 +- sound/firewire/fireworks/fireworks_transaction.c | 5 +- 4 files changed, 56 insertions(+), 25 deletions(-) diff --git a/sound/firewire/fireworks/fireworks.h b/sound/firewire/fireworks/fireworks.h index 03ed35237e2b..d73c12b8753d 100644 --- a/sound/firewire/fireworks/fireworks.h +++ b/sound/firewire/fireworks/fireworks.h @@ -108,7 +108,6 @@ struct snd_efw { u8 *resp_buf; u8 *pull_ptr; u8 *push_ptr; - unsigned int resp_queues; }; int snd_efw_transaction_cmd(struct fw_unit *unit, diff --git a/sound/firewire/fireworks/fireworks_hwdep.c b/sound/firewire/fireworks/fireworks_hwdep.c index 33df8655fe81..2e1d9a23920c 100644 --- a/sound/firewire/fireworks/fireworks_hwdep.c +++ b/sound/firewire/fireworks/fireworks_hwdep.c @@ -25,6 +25,7 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained, { unsigned int length, till_end, type; struct snd_efw_transaction *t; + u8 *pull_ptr; long count = 0; if (remained < sizeof(type) + sizeof(struct snd_efw_transaction)) @@ -38,8 +39,17 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained, buf += sizeof(type); /* write into buffer as many responses as possible */ - while (efw->resp_queues > 0) { - t = (struct snd_efw_transaction *)(efw->pull_ptr); + spin_lock_irq(&efw->lock); + + /* + * When another task reaches here during this task's access to user + * space, it picks up current position in buffer and can read the same + * series of responses. + */ + pull_ptr = efw->pull_ptr; + + while (efw->push_ptr != pull_ptr) { + t = (struct snd_efw_transaction *)(pull_ptr); length = be32_to_cpu(t->length) * sizeof(__be32); /* confirm enough space for this response */ @@ -49,26 +59,39 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained, /* copy from ring buffer to user buffer */ while (length > 0) { till_end = snd_efw_resp_buf_size - - (unsigned int)(efw->pull_ptr - efw->resp_buf); + (unsigned int)(pull_ptr - efw->resp_buf); till_end = min_t(unsigned int, length, till_end); - if (copy_to_user(buf, efw->pull_ptr, till_end)) + spin_unlock_irq(&efw->lock); + + if (copy_to_user(buf, pull_ptr, till_end)) return -EFAULT; - efw->pull_ptr += till_end; - if (efw->pull_ptr >= efw->resp_buf + - snd_efw_resp_buf_size) - efw->pull_ptr -= snd_efw_resp_buf_size; + spin_lock_irq(&efw->lock); + + pull_ptr += till_end; + if (pull_ptr >= efw->resp_buf + snd_efw_resp_buf_size) + pull_ptr -= snd_efw_resp_buf_size; length -= till_end; buf += till_end; count += till_end; remained -= till_end; } - - efw->resp_queues--; } + /* + * All of tasks can read from the buffer nearly simultaneously, but the + * last position for each task is different depending on the length of + * given buffer. Here, for simplicity, a position of buffer is set by + * the latest task. It's better for a listening application to allow one + * thread to read from the buffer. Unless, each task can read different + * sequence of responses depending on variation of buffer length. + */ + efw->pull_ptr = pull_ptr; + + spin_unlock_irq(&efw->lock); + return count; } @@ -76,14 +99,17 @@ static long hwdep_read_locked(struct snd_efw *efw, char __user *buf, long count, loff_t *offset) { - union snd_firewire_event event; + union snd_firewire_event event = { + .lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS, + }; - memset(&event, 0, sizeof(event)); + spin_lock_irq(&efw->lock); - event.lock_status.type = SNDRV_FIREWIRE_EVENT_LOCK_STATUS; event.lock_status.status = (efw->dev_lock_count > 0); efw->dev_lock_changed = false; + spin_unlock_irq(&efw->lock); + count = min_t(long, count, sizeof(event.lock_status)); if (copy_to_user(buf, &event, count)) @@ -98,10 +124,15 @@ hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count, { struct snd_efw *efw = hwdep->private_data; DEFINE_WAIT(wait); + bool dev_lock_changed; + bool queued; spin_lock_irq(&efw->lock); - while ((!efw->dev_lock_changed) && (efw->resp_queues == 0)) { + dev_lock_changed = efw->dev_lock_changed; + queued = efw->push_ptr != efw->pull_ptr; + + while (!dev_lock_changed && !queued) { prepare_to_wait(&efw->hwdep_wait, &wait, TASK_INTERRUPTIBLE); spin_unlock_irq(&efw->lock); schedule(); @@ -109,15 +140,17 @@ hwdep_read(struct snd_hwdep *hwdep, char __user *buf, long count, if (signal_pending(current)) return -ERESTARTSYS; spin_lock_irq(&efw->lock); + dev_lock_changed = efw->dev_lock_changed; + queued = efw->push_ptr != efw->pull_ptr; } - if (efw->dev_lock_changed) + spin_unlock_irq(&efw->lock); + + if (dev_lock_changed) count = hwdep_read_locked(efw, buf, count, offset); - else if (efw->resp_queues > 0) + else if (queued) count = hwdep_read_resp_buf(efw, buf, count, offset); - spin_unlock_irq(&efw->lock); - return count; } @@ -160,7 +193,7 @@ hwdep_poll(struct snd_hwdep *hwdep, struct file *file, poll_table *wait) poll_wait(file, &efw->hwdep_wait, wait); spin_lock_irq(&efw->lock); - if (efw->dev_lock_changed || (efw->resp_queues > 0)) + if (efw->dev_lock_changed || efw->pull_ptr != efw->push_ptr) events = POLLIN | POLLRDNORM; else events = 0; diff --git a/sound/firewire/fireworks/fireworks_proc.c b/sound/firewire/fireworks/fireworks_proc.c index 0639dcb13f7d..beb0a0ffee57 100644 --- a/sound/firewire/fireworks/fireworks_proc.c +++ b/sound/firewire/fireworks/fireworks_proc.c @@ -188,8 +188,8 @@ proc_read_queues_state(struct snd_info_entry *entry, else consumed = (unsigned int)(efw->push_ptr - efw->pull_ptr); - snd_iprintf(buffer, "%d %d/%d\n", - efw->resp_queues, consumed, snd_efw_resp_buf_size); + snd_iprintf(buffer, "%d/%d\n", + consumed, snd_efw_resp_buf_size); } static void diff --git a/sound/firewire/fireworks/fireworks_transaction.c b/sound/firewire/fireworks/fireworks_transaction.c index f550808d1784..36a08ba51ec7 100644 --- a/sound/firewire/fireworks/fireworks_transaction.c +++ b/sound/firewire/fireworks/fireworks_transaction.c @@ -121,11 +121,11 @@ copy_resp_to_buf(struct snd_efw *efw, void *data, size_t length, int *rcode) size_t capacity, till_end; struct snd_efw_transaction *t; - spin_lock_irq(&efw->lock); - t = (struct snd_efw_transaction *)data; length = min_t(size_t, be32_to_cpu(t->length) * sizeof(u32), length); + spin_lock_irq(&efw->lock); + if (efw->push_ptr < efw->pull_ptr) capacity = (unsigned int)(efw->pull_ptr - efw->push_ptr); else @@ -155,7 +155,6 @@ copy_resp_to_buf(struct snd_efw *efw, void *data, size_t length, int *rcode) } /* for hwdep */ - efw->resp_queues++; wake_up(&efw->hwdep_wait); *rcode = RCODE_COMPLETE; -- cgit v1.2.3-59-g8ed1b From 11749e086b2766cccf6217a527ef5c5604ba069c Mon Sep 17 00:00:00 2001 From: Vegard Nossum Date: Sun, 28 Aug 2016 10:13:07 +0200 Subject: 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 Cc: Signed-off-by: Takashi Iwai --- sound/core/timer.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sound/core/timer.c b/sound/core/timer.c index 2d6e3e76ddf5..2706061fc1ea 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -1972,6 +1972,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))) @@ -1981,6 +1982,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) -- cgit v1.2.3-59-g8ed1b From 9f8a7658bcafb2a7853f7a2eae8a94e87e6e695b Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Wed, 7 Sep 2016 15:45:31 +0200 Subject: ALSA: timer: Fix zero-division by continue of uninitialized instance When a user timer instance is continued without the explicit start beforehand, the system gets eventually zero-division error like: divide error: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN CPU: 1 PID: 27320 Comm: syz-executor Not tainted 4.8.0-rc3-next-20160825+ #8 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 task: ffff88003c9b2280 task.stack: ffff880027280000 RIP: 0010:[] [< inline >] ktime_divns include/linux/ktime.h:195 RIP: 0010:[] [] snd_hrtimer_callback+0x1bc/0x3c0 sound/core/hrtimer.c:62 Call Trace: [< inline >] __run_hrtimer kernel/time/hrtimer.c:1238 [] __hrtimer_run_queues+0x325/0xe70 kernel/time/hrtimer.c:1302 [] hrtimer_interrupt+0x18b/0x420 kernel/time/hrtimer.c:1336 [] local_apic_timer_interrupt+0x6f/0xe0 arch/x86/kernel/apic/apic.c:933 [] smp_apic_timer_interrupt+0x76/0xa0 arch/x86/kernel/apic/apic.c:957 [] apic_timer_interrupt+0x8c/0xa0 arch/x86/entry/entry_64.S:487 ..... Although a similar issue was spotted and a fix patch was merged in commit [6b760bb2c63a: ALSA: timer: fix division by zero after SNDRV_TIMER_IOCTL_CONTINUE], it seems covering only a part of iceberg. In this patch, we fix the issue a bit more drastically. Basically the continue of an uninitialized timer is supposed to be a fresh start, so we do it for user timers. For the direct snd_timer_continue() call, there is no way to pass the initial tick value, so we kick out for the uninitialized case. Reported-by: Dmitry Vyukov Cc: Signed-off-by: Takashi Iwai --- sound/core/timer.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/sound/core/timer.c b/sound/core/timer.c index 2706061fc1ea..fc144f43faa6 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -35,6 +35,9 @@ #include #include +/* internal flags */ +#define SNDRV_TIMER_IFLG_PAUSED 0x00010000 + #if IS_ENABLED(CONFIG_SND_HRTIMER) #define DEFAULT_TIMER_LIMIT 4 #else @@ -539,6 +542,10 @@ static int snd_timer_stop1(struct snd_timer_instance *timeri, bool stop) } } timeri->flags &= ~(SNDRV_TIMER_IFLG_RUNNING | SNDRV_TIMER_IFLG_START); + if (stop) + timeri->flags &= ~SNDRV_TIMER_IFLG_PAUSED; + else + timeri->flags |= SNDRV_TIMER_IFLG_PAUSED; snd_timer_notify1(timeri, stop ? SNDRV_TIMER_EVENT_STOP : SNDRV_TIMER_EVENT_CONTINUE); unlock: @@ -600,6 +607,10 @@ int snd_timer_stop(struct snd_timer_instance *timeri) */ int snd_timer_continue(struct snd_timer_instance *timeri) { + /* timer can continue only after pause */ + if (!(timeri->flags & SNDRV_TIMER_IFLG_PAUSED)) + return -EINVAL; + if (timeri->flags & SNDRV_TIMER_IFLG_SLAVE) return snd_timer_start_slave(timeri, false); else @@ -1831,6 +1842,9 @@ static int snd_timer_user_continue(struct file *file) tu = file->private_data; if (!tu->timeri) return -EBADFD; + /* start timer instead of continue if it's not used before */ + if (!(tu->timeri->flags & SNDRV_TIMER_IFLG_PAUSED)) + return snd_timer_user_start(file); tu->timeri->lost = 0; return (err = snd_timer_continue(tu->timeri)) < 0 ? err : 0; } -- cgit v1.2.3-59-g8ed1b From 816f318b2364262a51024096da7ca3b84e78e3b5 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Tue, 30 Aug 2016 14:45:46 +0200 Subject: ALSA: rawmidi: Fix possible deadlock with virmidi registration When a seq-virmidi driver is initialized, it registers a rawmidi instance with its callback to create an associated seq kernel client. Currently it's done throughly in rawmidi's register_mutex context. Recently it was found that this may lead to a deadlock another rawmidi device that is being attached with the sequencer is accessed, as both open with the same register_mutex. This was actually triggered by syzkaller, as Dmitry Vyukov reported: ====================================================== [ INFO: possible circular locking dependency detected ] 4.8.0-rc1+ #11 Not tainted ------------------------------------------------------- syz-executor/7154 is trying to acquire lock: (register_mutex#5){+.+.+.}, at: [] snd_rawmidi_kernel_open+0x4b/0x260 sound/core/rawmidi.c:341 but task is already holding lock: (&grp->list_mutex){++++.+}, at: [] check_and_subscribe_port+0x5b/0x5c0 sound/core/seq/seq_ports.c:495 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (&grp->list_mutex){++++.+}: [] lock_acquire+0x208/0x430 kernel/locking/lockdep.c:3746 [] down_read+0x49/0xc0 kernel/locking/rwsem.c:22 [< inline >] deliver_to_subscribers sound/core/seq/seq_clientmgr.c:681 [] snd_seq_deliver_event+0x35e/0x890 sound/core/seq/seq_clientmgr.c:822 [] > snd_seq_kernel_client_dispatch+0x126/0x170 sound/core/seq/seq_clientmgr.c:2418 [] snd_seq_system_broadcast+0xb2/0xf0 sound/core/seq/seq_system.c:101 [] snd_seq_create_kernel_client+0x24a/0x330 sound/core/seq/seq_clientmgr.c:2297 [< inline >] snd_virmidi_dev_attach_seq sound/core/seq/seq_virmidi.c:383 [] snd_virmidi_dev_register+0x29f/0x750 sound/core/seq/seq_virmidi.c:450 [] snd_rawmidi_dev_register+0x30c/0xd40 sound/core/rawmidi.c:1645 [] __snd_device_register.part.0+0x63/0xc0 sound/core/device.c:164 [< inline >] __snd_device_register sound/core/device.c:162 [] snd_device_register_all+0xad/0x110 sound/core/device.c:212 [] snd_card_register+0xef/0x6c0 sound/core/init.c:749 [] snd_virmidi_probe+0x3ef/0x590 sound/drivers/virmidi.c:123 [] platform_drv_probe+0x8b/0x170 drivers/base/platform.c:564 ...... -> #0 (register_mutex#5){+.+.+.}: [< inline >] check_prev_add kernel/locking/lockdep.c:1829 [< inline >] check_prevs_add kernel/locking/lockdep.c:1939 [< inline >] validate_chain kernel/locking/lockdep.c:2266 [] __lock_acquire+0x4d44/0x4d80 kernel/locking/lockdep.c:3335 [] lock_acquire+0x208/0x430 kernel/locking/lockdep.c:3746 [< inline >] __mutex_lock_common kernel/locking/mutex.c:521 [] mutex_lock_nested+0xb1/0xa20 kernel/locking/mutex.c:621 [] snd_rawmidi_kernel_open+0x4b/0x260 sound/core/rawmidi.c:341 [] midisynth_subscribe+0xf7/0x350 sound/core/seq/seq_midi.c:188 [< inline >] subscribe_port sound/core/seq/seq_ports.c:427 [] check_and_subscribe_port+0x467/0x5c0 sound/core/seq/seq_ports.c:510 [] snd_seq_port_connect+0x2c9/0x500 sound/core/seq/seq_ports.c:579 [] snd_seq_ioctl_subscribe_port+0x1d8/0x2b0 sound/core/seq/seq_clientmgr.c:1480 [] snd_seq_do_ioctl+0x184/0x1e0 sound/core/seq/seq_clientmgr.c:2225 [] snd_seq_kernel_client_ctl+0xa8/0x110 sound/core/seq/seq_clientmgr.c:2440 [] snd_seq_oss_midi_open+0x3b4/0x610 sound/core/seq/oss/seq_oss_midi.c:375 [] snd_seq_oss_synth_setup_midi+0x107/0x4c0 sound/core/seq/oss/seq_oss_synth.c:281 [] snd_seq_oss_open+0x748/0x8d0 sound/core/seq/oss/seq_oss_init.c:274 [] odev_open+0x6a/0x90 sound/core/seq/oss/seq_oss.c:138 [] soundcore_open+0x30f/0x640 sound/sound_core.c:639 ...... other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(&grp->list_mutex); lock(register_mutex#5); lock(&grp->list_mutex); lock(register_mutex#5); *** DEADLOCK *** ====================================================== The fix is to simply move the registration parts in snd_rawmidi_dev_register() to the outside of the register_mutex lock. The lock is needed only to manage the linked list, and it's not necessarily to cover the whole initialization process. Reported-by: Dmitry Vyukov Cc: Signed-off-by: Takashi Iwai --- sound/core/rawmidi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c index 795437b10082..b450a27588c8 100644 --- a/sound/core/rawmidi.c +++ b/sound/core/rawmidi.c @@ -1633,11 +1633,13 @@ static int snd_rawmidi_dev_register(struct snd_device *device) return -EBUSY; } list_add_tail(&rmidi->list, &snd_rawmidi_devices); + mutex_unlock(®ister_mutex); err = snd_register_device(SNDRV_DEVICE_TYPE_RAWMIDI, rmidi->card, rmidi->device, &snd_rawmidi_f_ops, rmidi, &rmidi->dev); if (err < 0) { rmidi_err(rmidi, "unable to register\n"); + mutex_lock(®ister_mutex); list_del(&rmidi->list); mutex_unlock(®ister_mutex); return err; @@ -1645,6 +1647,7 @@ static int snd_rawmidi_dev_register(struct snd_device *device) if (rmidi->ops && rmidi->ops->dev_register && (err = rmidi->ops->dev_register(rmidi)) < 0) { snd_unregister_device(&rmidi->dev); + mutex_lock(®ister_mutex); list_del(&rmidi->list); mutex_unlock(®ister_mutex); return err; @@ -1677,7 +1680,6 @@ static int snd_rawmidi_dev_register(struct snd_device *device) } } #endif /* CONFIG_SND_OSSEMUL */ - mutex_unlock(®ister_mutex); sprintf(name, "midi%d", rmidi->device); entry = snd_info_create_card_entry(rmidi->card, name, rmidi->card->proc_root); if (entry) { -- cgit v1.2.3-59-g8ed1b