[alsa-devel] [PATCH 0/2] More hardening for ALSA sequencer write/ioctl races
Hi,
here is two patches to paper over the still remaining races in ALSA sequencer write and ioctl that have been leaked in the previous fix for CVE-2018-1000004.
The reports came up totally individually, so both people are put as Reported-by tag here.
thanks,
Takashi
===
Takashi Iwai (2): ALSA: seq: Don't allow resizing pool in use ALSA: seq: More protection for concurrent write and ioctl races
sound/core/seq/seq_clientmgr.c | 21 ++++++++++++++------- sound/core/seq/seq_fifo.c | 2 +- sound/core/seq/seq_memory.c | 14 ++++++++++---- sound/core/seq/seq_memory.h | 3 ++- 4 files changed, 27 insertions(+), 13 deletions(-)
This is a fix for a (sort of) fallout in the recent commit d15d662e89fc ("ALSA: seq: Fix racy pool initializations") for CVE-2018-1000004. As the pool resize deletes the existing cells, it may lead to a race when another thread is writing concurrently, eventually resulting a UAF.
A simple workaround is not to allow the pool resizing when the pool is in use. It's an invalid behavior in anyway.
Fixes: d15d662e89fc ("ALSA: seq: Fix racy pool initializations") Reported-by: 范龙飞 long7573@126.com Reported-by: Nicolai Stange nstange@suse.de Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/seq/seq_clientmgr.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 04d4db44fae5..d41ce3ed62ca 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -1838,6 +1838,9 @@ static int snd_seq_ioctl_set_client_pool(struct snd_seq_client *client, (! snd_seq_write_pool_allocated(client) || info->output_pool != client->pool->size)) { if (snd_seq_write_pool_allocated(client)) { + /* is the pool in use? */ + if (atomic_read(&client->pool->counter)) + return -EBUSY; /* remove all existing cells */ snd_seq_pool_mark_closing(client->pool); snd_seq_queue_client_leave_cells(client->number);
Takashi Iwai tiwai@suse.de writes:
This is a fix for a (sort of) fallout in the recent commit d15d662e89fc ("ALSA: seq: Fix racy pool initializations") for CVE-2018-1000004. As the pool resize deletes the existing cells, it may lead to a race when another thread is writing concurrently, eventually resulting a UAF.
A simple workaround is not to allow the pool resizing when the pool is in use. It's an invalid behavior in anyway.
Fixes: d15d662e89fc ("ALSA: seq: Fix racy pool initializations") Reported-by: 范龙飞 long7573@126.com Reported-by: Nicolai Stange nstange@suse.de Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de
sound/core/seq/seq_clientmgr.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 04d4db44fae5..d41ce3ed62ca 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -1838,6 +1838,9 @@ static int snd_seq_ioctl_set_client_pool(struct snd_seq_client *client, (! snd_seq_write_pool_allocated(client) || info->output_pool != client->pool->size)) { if (snd_seq_write_pool_allocated(client)) {
Maybe I'm missing something, but doesn't this
/* is the pool in use? */if (atomic_read(&client->pool->counter))return -EBUSY;
/* remove all existing cells */ snd_seq_pool_mark_closing(client->pool);
render this
snd_seq_queue_client_leave_cells(client->number);
useless (assuming the presence of [2/2] ("ALSA: seq: More protection for concurrent write and ioctl races"))?
Thanks,
Nicolai
On Thu, 08 Mar 2018 11:44:47 +0100, Nicolai Stange wrote:
Takashi Iwai tiwai@suse.de writes:
This is a fix for a (sort of) fallout in the recent commit d15d662e89fc ("ALSA: seq: Fix racy pool initializations") for CVE-2018-1000004. As the pool resize deletes the existing cells, it may lead to a race when another thread is writing concurrently, eventually resulting a UAF.
A simple workaround is not to allow the pool resizing when the pool is in use. It's an invalid behavior in anyway.
Fixes: d15d662e89fc ("ALSA: seq: Fix racy pool initializations") Reported-by: 范龙飞 long7573@126.com Reported-by: Nicolai Stange nstange@suse.de Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de
sound/core/seq/seq_clientmgr.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 04d4db44fae5..d41ce3ed62ca 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -1838,6 +1838,9 @@ static int snd_seq_ioctl_set_client_pool(struct snd_seq_client *client, (! snd_seq_write_pool_allocated(client) || info->output_pool != client->pool->size)) { if (snd_seq_write_pool_allocated(client)) {
Maybe I'm missing something, but doesn't this
/* is the pool in use? */if (atomic_read(&client->pool->counter))return -EBUSY;/* remove all existing cells */ snd_seq_pool_mark_closing(client->pool);render this
snd_seq_queue_client_leave_cells(client->number);useless (assuming the presence of [2/2] ("ALSA: seq: More protection for concurrent write and ioctl races"))?
Right, after the second patch, this call can be removed, indeed. I'll cook up the third patch to remove the superfluous call.
thanks,
Takashi
This patch is an attempt for further hardening against races between the concurrent write and ioctls. The previous fix d15d662e89fc ("ALSA: seq: Fix racy pool initializations") covered the race of the pool initialization at writer and the pool resize ioctl by the client->ioctl_mutex (CVE-2018-1000004). However, basically this mutex should be applied more widely to the whole write operation for avoiding the unexpected pool operations by another thread.
The only change outside snd_seq_write() is the additional mutex argument to helper functions, so that we can unlock / relock the given mutex temporarily during schedule() call for blocking write.
Fixes: d15d662e89fc ("ALSA: seq: Fix racy pool initializations") Reported-by: 范龙飞 long7573@126.com Reported-by: Nicolai Stange nstange@suse.de Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/seq/seq_clientmgr.c | 18 +++++++++++------- sound/core/seq/seq_fifo.c | 2 +- sound/core/seq/seq_memory.c | 14 ++++++++++---- sound/core/seq/seq_memory.h | 3 ++- 4 files changed, 24 insertions(+), 13 deletions(-)
diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index d41ce3ed62ca..1b62421dadd1 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -910,7 +910,8 @@ int snd_seq_dispatch_event(struct snd_seq_event_cell *cell, int atomic, int hop) static int snd_seq_client_enqueue_event(struct snd_seq_client *client, struct snd_seq_event *event, struct file *file, int blocking, - int atomic, int hop) + int atomic, int hop, + struct mutex *mutexp) { struct snd_seq_event_cell *cell; int err; @@ -948,7 +949,8 @@ static int snd_seq_client_enqueue_event(struct snd_seq_client *client, return -ENXIO; /* queue is not allocated */
/* allocate an event cell */ - err = snd_seq_event_dup(client->pool, event, &cell, !blocking || atomic, file); + err = snd_seq_event_dup(client->pool, event, &cell, !blocking || atomic, + file, mutexp); if (err < 0) return err;
@@ -1017,12 +1019,11 @@ static ssize_t snd_seq_write(struct file *file, const char __user *buf, return -ENXIO;
/* allocate the pool now if the pool is not allocated yet */ + mutex_lock(&client->ioctl_mutex); if (client->pool->size > 0 && !snd_seq_write_pool_allocated(client)) { - mutex_lock(&client->ioctl_mutex); err = snd_seq_pool_init(client->pool); - mutex_unlock(&client->ioctl_mutex); if (err < 0) - return -ENOMEM; + goto out; }
/* only process whole events */ @@ -1073,7 +1074,7 @@ static ssize_t snd_seq_write(struct file *file, const char __user *buf, /* ok, enqueue it */ err = snd_seq_client_enqueue_event(client, &event, file, !(file->f_flags & O_NONBLOCK), - 0, 0); + 0, 0, &client->ioctl_mutex); if (err < 0) break;
@@ -1084,6 +1085,8 @@ static ssize_t snd_seq_write(struct file *file, const char __user *buf, written += len; }
+ out: + mutex_unlock(&client->ioctl_mutex); return written ? written : err; }
@@ -2263,7 +2266,8 @@ static int kernel_client_enqueue(int client, struct snd_seq_event *ev, if (! cptr->accept_output) result = -EPERM; else /* send it */ - result = snd_seq_client_enqueue_event(cptr, ev, file, blocking, atomic, hop); + result = snd_seq_client_enqueue_event(cptr, ev, file, blocking, + atomic, hop, NULL);
snd_seq_client_unlock(cptr); return result; diff --git a/sound/core/seq/seq_fifo.c b/sound/core/seq/seq_fifo.c index a8c2822e0198..72c0302a55d2 100644 --- a/sound/core/seq/seq_fifo.c +++ b/sound/core/seq/seq_fifo.c @@ -125,7 +125,7 @@ int snd_seq_fifo_event_in(struct snd_seq_fifo *f, return -EINVAL;
snd_use_lock_use(&f->use_lock); - err = snd_seq_event_dup(f->pool, event, &cell, 1, NULL); /* always non-blocking */ + err = snd_seq_event_dup(f->pool, event, &cell, 1, NULL, NULL); /* always non-blocking */ if (err < 0) { if ((err == -ENOMEM) || (err == -EAGAIN)) atomic_inc(&f->overflow); diff --git a/sound/core/seq/seq_memory.c b/sound/core/seq/seq_memory.c index f763682584a8..ab1112e90f88 100644 --- a/sound/core/seq/seq_memory.c +++ b/sound/core/seq/seq_memory.c @@ -220,7 +220,8 @@ void snd_seq_cell_free(struct snd_seq_event_cell * cell) */ static int snd_seq_cell_alloc(struct snd_seq_pool *pool, struct snd_seq_event_cell **cellp, - int nonblock, struct file *file) + int nonblock, struct file *file, + struct mutex *mutexp) { struct snd_seq_event_cell *cell; unsigned long flags; @@ -244,7 +245,11 @@ static int snd_seq_cell_alloc(struct snd_seq_pool *pool, set_current_state(TASK_INTERRUPTIBLE); add_wait_queue(&pool->output_sleep, &wait); spin_unlock_irq(&pool->lock); + if (mutexp) + mutex_unlock(mutexp); schedule(); + if (mutexp) + mutex_lock(mutexp); spin_lock_irq(&pool->lock); remove_wait_queue(&pool->output_sleep, &wait); /* interrupted? */ @@ -287,7 +292,7 @@ static int snd_seq_cell_alloc(struct snd_seq_pool *pool, */ int snd_seq_event_dup(struct snd_seq_pool *pool, struct snd_seq_event *event, struct snd_seq_event_cell **cellp, int nonblock, - struct file *file) + struct file *file, struct mutex *mutexp) { int ncells, err; unsigned int extlen; @@ -304,7 +309,7 @@ int snd_seq_event_dup(struct snd_seq_pool *pool, struct snd_seq_event *event, if (ncells >= pool->total_elements) return -ENOMEM;
- err = snd_seq_cell_alloc(pool, &cell, nonblock, file); + err = snd_seq_cell_alloc(pool, &cell, nonblock, file, mutexp); if (err < 0) return err;
@@ -330,7 +335,8 @@ int snd_seq_event_dup(struct snd_seq_pool *pool, struct snd_seq_event *event, int size = sizeof(struct snd_seq_event); if (len < size) size = len; - err = snd_seq_cell_alloc(pool, &tmp, nonblock, file); + err = snd_seq_cell_alloc(pool, &tmp, nonblock, file, + mutexp); if (err < 0) goto __error; if (cell->event.data.ext.ptr == NULL) diff --git a/sound/core/seq/seq_memory.h b/sound/core/seq/seq_memory.h index 32f959c17786..3abe306c394a 100644 --- a/sound/core/seq/seq_memory.h +++ b/sound/core/seq/seq_memory.h @@ -66,7 +66,8 @@ struct snd_seq_pool { void snd_seq_cell_free(struct snd_seq_event_cell *cell);
int snd_seq_event_dup(struct snd_seq_pool *pool, struct snd_seq_event *event, - struct snd_seq_event_cell **cellp, int nonblock, struct file *file); + struct snd_seq_event_cell **cellp, int nonblock, + struct file *file, struct mutex *mutexp);
/* return number of unused (free) cells */ static inline int snd_seq_unused_cells(struct snd_seq_pool *pool)
Takashi Iwai tiwai@suse.de writes:
This patch is an attempt for further hardening against races between the concurrent write and ioctls. The previous fix d15d662e89fc ("ALSA: seq: Fix racy pool initializations") covered the race of the pool initialization at writer and the pool resize ioctl by the client->ioctl_mutex (CVE-2018-1000004). However, basically this mutex should be applied more widely to the whole write operation for avoiding the unexpected pool operations by another thread.
The only change outside snd_seq_write() is the additional mutex argument to helper functions, so that we can unlock / relock the given mutex temporarily during schedule() call for blocking write.
Fixes: d15d662e89fc ("ALSA: seq: Fix racy pool initializations") Reported-by: 范龙飞 long7573@126.com Reported-by: Nicolai Stange nstange@suse.de Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de
Reviewed-and-tested-by: Nicolai Stange nstange@suse.de
participants (2)
-
Nicolai Stange -
Takashi Iwai