[alsa-devel] [PATCH 0/4] Yet another fixes for ALSA timer info leaks
Hi,
this is a patchset to paper over a bug in ALSA timer core code about the race between read and ioctl. As a side-effect of the race, it may allow to read some uninitialized kmalloced memory. The first two patches cover the issue, so marked as stable, while the rest two patches are rather cleanups and more hardening.
I'm going to queue the first to for-linus for 4.12-rc, and the latter two to for-next, for 4.13.
thanks,
Takashi
===
Takashi Iwai (4): ALSA: timer: Fix race between read and ioctl ALSA: timer: Fix missing queue indices reset at SNDRV_TIMER_IOCTL_SELECT ALSA: timer: Improve user queue reallocation ALSA: timer: Wrap with spinlock for queue access
sound/core/timer.c | 103 ++++++++++++++++++++++++++--------------------------- 1 file changed, 51 insertions(+), 52 deletions(-)
The read from ALSA timer device, the function snd_timer_user_tread(), may access to an uninitialized struct snd_timer_user fields when the read is concurrently performed while the ioctl like snd_timer_user_tselect() is invoked. We have already fixed the races among ioctls via a mutex, but we seem to have forgotten the race between read vs ioctl.
This patch simply applies (more exactly extends the already applied range of) tu->ioctl_lock in snd_timer_user_tread() for closing the race window.
Reported-by: Alexander Potapenko glider@google.com Tested-by: Alexander Potapenko glider@google.com Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/timer.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/sound/core/timer.c b/sound/core/timer.c index 2f836ca09860..1118bd8e2d3c 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -1959,6 +1959,7 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer,
tu = file->private_data; unit = tu->tread ? sizeof(struct snd_timer_tread) : sizeof(struct snd_timer_read); + mutex_lock(&tu->ioctl_lock); spin_lock_irq(&tu->qlock); while ((long)count - result >= unit) { while (!tu->qused) { @@ -1974,7 +1975,9 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer, add_wait_queue(&tu->qchange_sleep, &wait);
spin_unlock_irq(&tu->qlock); + mutex_unlock(&tu->ioctl_lock); schedule(); + mutex_lock(&tu->ioctl_lock); spin_lock_irq(&tu->qlock);
remove_wait_queue(&tu->qchange_sleep, &wait); @@ -1994,7 +1997,6 @@ 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))) @@ -2004,7 +2006,6 @@ 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) @@ -2014,6 +2015,7 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer, } _error: spin_unlock_irq(&tu->qlock); + mutex_unlock(&tu->ioctl_lock); return result > 0 ? result : err; }
snd_timer_user_tselect() reallocates the queue buffer dynamically, but it forgot to reset its indices. Since the read may happen concurrently with ioctl and snd_timer_user_tselect() allocates the buffer via kmalloc(), this may lead to the leak of uninitialized kernel-space data, as spotted via KMSAN:
BUG: KMSAN: use of unitialized memory in snd_timer_user_read+0x6c4/0xa10 CPU: 0 PID: 1037 Comm: probe Not tainted 4.11.0-rc5+ #2739 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:16 dump_stack+0x143/0x1b0 lib/dump_stack.c:52 kmsan_report+0x12a/0x180 mm/kmsan/kmsan.c:1007 kmsan_check_memory+0xc2/0x140 mm/kmsan/kmsan.c:1086 copy_to_user ./arch/x86/include/asm/uaccess.h:725 snd_timer_user_read+0x6c4/0xa10 sound/core/timer.c:2004 do_loop_readv_writev fs/read_write.c:716 __do_readv_writev+0x94c/0x1380 fs/read_write.c:864 do_readv_writev fs/read_write.c:894 vfs_readv fs/read_write.c:908 do_readv+0x52a/0x5d0 fs/read_write.c:934 SYSC_readv+0xb6/0xd0 fs/read_write.c:1021 SyS_readv+0x87/0xb0 fs/read_write.c:1018
This patch adds the missing reset of queue indices. Together with the previous fix for the ioctl/read race, we cover the whole problem.
Reported-by: Alexander Potapenko glider@google.com Tested-by: Alexander Potapenko glider@google.com Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/timer.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/core/timer.c b/sound/core/timer.c index 1118bd8e2d3c..cd67d1c12cf1 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -1618,6 +1618,7 @@ static int snd_timer_user_tselect(struct file *file, if (err < 0) goto __err;
+ tu->qhead = tu->qtail = tu->qused = 0; kfree(tu->queue); tu->queue = NULL; kfree(tu->tqueue);
ALSA timer may reallocate the user queue upon request, and it happens at three places for now: at opening, at SNDRV_TIMER_IOCTL_PARAMS, and at SNDRV_TIMER_IOCTL_SELECT. However, the last one, snd_timer_user_tselect(), doesn't need to reallocate the buffer since it doesn't change the queue size. It does just because tu->tread might have been changed before starting the timer.
Instead of *_SELECT ioctl, we should reallocate the queue at SNDRV_TIMER_IOCTL_TREAD; then the timer is guaranteed to be stopped, thus we can reassign the buffer more safely.
This patch implements that with a slight code refactoring. Essentially, the patch achieves: - Introduce realloc_user_queue() for (re-)allocating the ring buffer, and call it from all places. Also, realloc_user_queue() uses kcalloc() for avoiding possible leaks. - Add the buffer reallocation at SNDRV_TIMER_IOCTL_TREAD. When it fails, tu->tread is restored to the old value, too. - Drop the buffer reallocation at snd_timer_user_tselect().
Tested-by: Alexander Potapenko glider@google.com Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/timer.c | 94 +++++++++++++++++++++++++----------------------------- 1 file changed, 43 insertions(+), 51 deletions(-)
diff --git a/sound/core/timer.c b/sound/core/timer.c index cd67d1c12cf1..96cffb1be57f 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -1327,6 +1327,33 @@ static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri, wake_up(&tu->qchange_sleep); }
+static int realloc_user_queue(struct snd_timer_user *tu, int size) +{ + struct snd_timer_read *queue = NULL; + struct snd_timer_tread *tqueue = NULL; + + if (tu->tread) { + tqueue = kcalloc(size, sizeof(*tqueue), GFP_KERNEL); + if (!tqueue) + return -ENOMEM; + } else { + queue = kcalloc(size, sizeof(*queue), GFP_KERNEL); + if (!queue) + return -ENOMEM; + } + + spin_lock_irq(&tu->qlock); + kfree(tu->queue); + kfree(tu->tqueue); + tu->queue_size = size; + tu->queue = queue; + tu->tqueue = tqueue; + tu->qhead = tu->qtail = tu->qused = 0; + spin_unlock_irq(&tu->qlock); + + return 0; +} + static int snd_timer_user_open(struct inode *inode, struct file *file) { struct snd_timer_user *tu; @@ -1343,10 +1370,7 @@ static int snd_timer_user_open(struct inode *inode, struct file *file) init_waitqueue_head(&tu->qchange_sleep); mutex_init(&tu->ioctl_lock); tu->ticks = 1; - tu->queue_size = 128; - tu->queue = kmalloc(tu->queue_size * sizeof(struct snd_timer_read), - GFP_KERNEL); - if (tu->queue == NULL) { + if (realloc_user_queue(tu, 128) < 0) { kfree(tu); return -ENOMEM; } @@ -1618,34 +1642,12 @@ static int snd_timer_user_tselect(struct file *file, if (err < 0) goto __err;
- tu->qhead = tu->qtail = tu->qused = 0; - kfree(tu->queue); - tu->queue = NULL; - kfree(tu->tqueue); - tu->tqueue = NULL; - if (tu->tread) { - tu->tqueue = kmalloc(tu->queue_size * sizeof(struct snd_timer_tread), - GFP_KERNEL); - if (tu->tqueue == NULL) - err = -ENOMEM; - } else { - tu->queue = kmalloc(tu->queue_size * sizeof(struct snd_timer_read), - GFP_KERNEL); - if (tu->queue == NULL) - err = -ENOMEM; - } - - if (err < 0) { - snd_timer_close(tu->timeri); - tu->timeri = NULL; - } else { - tu->timeri->flags |= SNDRV_TIMER_IFLG_FAST; - tu->timeri->callback = tu->tread + tu->timeri->flags |= SNDRV_TIMER_IFLG_FAST; + tu->timeri->callback = tu->tread ? snd_timer_user_tinterrupt : snd_timer_user_interrupt; - tu->timeri->ccallback = snd_timer_user_ccallback; - tu->timeri->callback_data = (void *)tu; - tu->timeri->disconnect = snd_timer_user_disconnect; - } + tu->timeri->ccallback = snd_timer_user_ccallback; + tu->timeri->callback_data = (void *)tu; + tu->timeri->disconnect = snd_timer_user_disconnect;
__err: return err; @@ -1687,8 +1689,6 @@ static int snd_timer_user_params(struct file *file, struct snd_timer_user *tu; struct snd_timer_params params; struct snd_timer *t; - struct snd_timer_read *tr; - struct snd_timer_tread *ttr; int err;
tu = file->private_data; @@ -1751,23 +1751,9 @@ static int snd_timer_user_params(struct file *file, spin_unlock_irq(&t->lock); if (params.queue_size > 0 && (unsigned int)tu->queue_size != params.queue_size) { - if (tu->tread) { - ttr = kmalloc(params.queue_size * sizeof(*ttr), - GFP_KERNEL); - if (ttr) { - kfree(tu->tqueue); - tu->queue_size = params.queue_size; - tu->tqueue = ttr; - } - } else { - tr = kmalloc(params.queue_size * sizeof(*tr), - GFP_KERNEL); - if (tr) { - kfree(tu->queue); - tu->queue_size = params.queue_size; - tu->queue = tr; - } - } + err = realloc_user_queue(tu, params.queue_size); + if (err < 0) + goto _end; } tu->qhead = tu->qtail = tu->qused = 0; if (tu->timeri->flags & SNDRV_TIMER_IFLG_EARLY_EVENT) { @@ -1891,13 +1877,19 @@ static long __snd_timer_user_ioctl(struct file *file, unsigned int cmd, return snd_timer_user_next_device(argp); case SNDRV_TIMER_IOCTL_TREAD: { - int xarg; + int xarg, old_tread;
if (tu->timeri) /* too late */ return -EBUSY; if (get_user(xarg, p)) return -EFAULT; + old_tread = tu->tread; tu->tread = xarg ? 1 : 0; + if (tu->tread != old_tread && + realloc_user_queue(tu, tu->queue_size) < 0) { + tu->tread = old_tread; + return -ENOMEM; + } return 0; } case SNDRV_TIMER_IOCTL_GINFO:
For accessing the snd_timer_user queue indices, we take tu->qlock. But it's forgotten in a couple of places.
The one in snd_timer_user_params() should be safe without the spinlock as the timer is already stopped. But it's better for consistency.
The one in poll is just a read-out, so it's not inevitably needed, but it'd be good to make the result consistent, too.
Tested-by: Alexander Potapenko glider@google.com Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/timer.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/sound/core/timer.c b/sound/core/timer.c index 96cffb1be57f..148290ace756 100644 --- a/sound/core/timer.c +++ b/sound/core/timer.c @@ -1755,6 +1755,7 @@ static int snd_timer_user_params(struct file *file, if (err < 0) goto _end; } + spin_lock_irq(&tu->qlock); tu->qhead = tu->qtail = tu->qused = 0; if (tu->timeri->flags & SNDRV_TIMER_IFLG_EARLY_EVENT) { if (tu->tread) { @@ -1775,6 +1776,7 @@ static int snd_timer_user_params(struct file *file, } tu->filter = params.filter; tu->ticks = params.ticks; + spin_unlock_irq(&tu->qlock); err = 0; _end: if (copy_to_user(_params, ¶ms, sizeof(params))) @@ -2022,10 +2024,12 @@ static unsigned int snd_timer_user_poll(struct file *file, poll_table * wait) poll_wait(file, &tu->qchange_sleep, wait);
mask = 0; + spin_lock_irq(&tu->qlock); if (tu->qused) mask |= POLLIN | POLLRDNORM; if (tu->disconnected) mask |= POLLERR; + spin_unlock_irq(&tu->qlock);
return mask; }
participants (1)
-
Takashi Iwai