* [PATCH 0/2] More hardening for ALSA sequencer write/ioctl races @ 2018-03-08 7:18 Takashi Iwai 2018-03-08 7:18 ` [PATCH 1/2] ALSA: seq: Don't allow resizing pool in use Takashi Iwai 2018-03-08 7:18 ` [PATCH 2/2] ALSA: seq: More protection for concurrent write and ioctl races Takashi Iwai 0 siblings, 2 replies; 6+ messages in thread From: Takashi Iwai @ 2018-03-08 7:18 UTC (permalink / raw) To: alsa-devel; +Cc: 范龙飞, Nicolai Stange 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(-) -- 2.16.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] ALSA: seq: Don't allow resizing pool in use 2018-03-08 7:18 [PATCH 0/2] More hardening for ALSA sequencer write/ioctl races Takashi Iwai @ 2018-03-08 7:18 ` Takashi Iwai 2018-03-08 10:44 ` Nicolai Stange 2018-03-08 7:18 ` [PATCH 2/2] ALSA: seq: More protection for concurrent write and ioctl races Takashi Iwai 1 sibling, 1 reply; 6+ messages in thread From: Takashi Iwai @ 2018-03-08 7:18 UTC (permalink / raw) To: alsa-devel; +Cc: 范龙飞, Nicolai Stange 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); -- 2.16.2 _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ALSA: seq: Don't allow resizing pool in use 2018-03-08 7:18 ` [PATCH 1/2] ALSA: seq: Don't allow resizing pool in use Takashi Iwai @ 2018-03-08 10:44 ` Nicolai Stange 2018-03-08 10:56 ` Takashi Iwai 0 siblings, 1 reply; 6+ messages in thread From: Nicolai Stange @ 2018-03-08 10:44 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, Miroslav Benes, 范龙飞, Nicolai Stange 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 -- SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ALSA: seq: Don't allow resizing pool in use 2018-03-08 10:44 ` Nicolai Stange @ 2018-03-08 10:56 ` Takashi Iwai 0 siblings, 0 replies; 6+ messages in thread From: Takashi Iwai @ 2018-03-08 10:56 UTC (permalink / raw) To: Nicolai Stange; +Cc: alsa-devel, Miroslav Benes, 范龙飞 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 _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] ALSA: seq: More protection for concurrent write and ioctl races 2018-03-08 7:18 [PATCH 0/2] More hardening for ALSA sequencer write/ioctl races Takashi Iwai 2018-03-08 7:18 ` [PATCH 1/2] ALSA: seq: Don't allow resizing pool in use Takashi Iwai @ 2018-03-08 7:18 ` Takashi Iwai 2018-03-08 10:38 ` Nicolai Stange 1 sibling, 1 reply; 6+ messages in thread From: Takashi Iwai @ 2018-03-08 7:18 UTC (permalink / raw) To: alsa-devel; +Cc: 范龙飞, Nicolai Stange 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) -- 2.16.2 _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ALSA: seq: More protection for concurrent write and ioctl races 2018-03-08 7:18 ` [PATCH 2/2] ALSA: seq: More protection for concurrent write and ioctl races Takashi Iwai @ 2018-03-08 10:38 ` Nicolai Stange 0 siblings, 0 replies; 6+ messages in thread From: Nicolai Stange @ 2018-03-08 10:38 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, Miroslav Benes, 范龙飞, Nicolai Stange 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> -- SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-08 10:56 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-03-08 7:18 [PATCH 0/2] More hardening for ALSA sequencer write/ioctl races Takashi Iwai 2018-03-08 7:18 ` [PATCH 1/2] ALSA: seq: Don't allow resizing pool in use Takashi Iwai 2018-03-08 10:44 ` Nicolai Stange 2018-03-08 10:56 ` Takashi Iwai 2018-03-08 7:18 ` [PATCH 2/2] ALSA: seq: More protection for concurrent write and ioctl races Takashi Iwai 2018-03-08 10:38 ` Nicolai Stange
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.