[PATCH 0/2] More hardening for ALSA sequencer write/ioctl races

[PATCH 0/2] More hardening for ALSA sequencer write/ioctl races

[Takashi Iwai]

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

[PATCH 1/2] ALSA: seq: Don't allow resizing pool in use

[Takashi Iwai]

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

[PATCH 2/2] ALSA: seq: More protection for concurrent write and ioctl races

[Takashi Iwai]

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

Re: [PATCH 2/2] ALSA: seq: More protection for concurrent write and ioctl races

[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

Re: [PATCH 1/2] ALSA: seq: Don't allow resizing pool in use

[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

Re: [PATCH 1/2] ALSA: seq: Don't allow resizing pool in use

[Takashi Iwai]

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