[PATCH 0/4][T] CVE-2018-7566, CVE-2018-1000004: Multiple issues in ALSA

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/4][T] CVE-2018-7566, CVE-2018-1000004: Multiple issues in ALSA

Tyler Hicks-2
https://people.canonical.com/~ubuntu-security/cve/2018/CVE-2018-1000004.html

 In the Linux kernel 4.12, 3.10, 2.6 and possibly earlier versions a race
 condition vulnerability exists in the sound system, this can lead to a
 deadlock and denial of service condition.

https://people.canonical.com/~ubuntu-security/cve/2018/CVE-2018-7566.html

 The Linux kernel 4.15 has a Buffer Overflow via an
 SNDRV_SEQ_IOCTL_SET_CLIENT_POOL ioctl write operation to /dev/snd/seq
 by a local user.

I've tested these changes by ensuring that audio still works in a
desktop VM. These issues only affect Trusty.

Tyler


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/4] ALSA: seq: Make ioctls race-free

Tyler Hicks-2
From: Takashi Iwai <[hidden email]>

The ALSA sequencer ioctls have no protection against racy calls while
the concurrent operations may lead to interfere with each other.  As
reported recently, for example, the concurrent calls of setting client
pool with a combination of write calls may lead to either the
unkillable dead-lock or UAF.

As a slightly big hammer solution, this patch introduces the mutex to
make each ioctl exclusive.  Although this may reduce performance via
parallel ioctl calls, usually it's not demanded for sequencer usages,
hence it should be negligible.

Reported-by: Luo Quan <[hidden email]>
Reviewed-by: Kees Cook <[hidden email]>
Reviewed-by: Greg Kroah-Hartman <[hidden email]>
Cc: <[hidden email]>
Signed-off-by: Takashi Iwai <[hidden email]>

CVE-2018-1000004

(backported from commit b3defb791b26ea0683a93a4f49c77ec45ec96f10)
Signed-off-by: Tyler Hicks <[hidden email]>
---
 sound/core/seq/seq_clientmgr.c | 12 +++++++++---
 sound/core/seq/seq_clientmgr.h |  1 +
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c
index 750e4a975379..cd539ac0e236 100644
--- a/sound/core/seq/seq_clientmgr.c
+++ b/sound/core/seq/seq_clientmgr.c
@@ -236,6 +236,7 @@ static struct snd_seq_client *seq_create_client1(int client_index, int poolsize)
  rwlock_init(&client->ports_lock);
  mutex_init(&client->ports_mutex);
  INIT_LIST_HEAD(&client->ports_list_head);
+ mutex_init(&client->ioctl_mutex);
 
  /* find free slot in the client table */
  spin_lock_irqsave(&clients_lock, flags);
@@ -2200,15 +2201,20 @@ static int snd_seq_do_ioctl(struct snd_seq_client *client, unsigned int cmd,
  if (! arg)
  return -EFAULT;
  for (p = ioctl_tables; p->cmd; p++) {
- if (p->cmd == cmd)
- return p->func(client, arg);
+ if (p->cmd == cmd) {
+ int err;
+
+ mutex_lock(&client->ioctl_mutex);
+ err = p->func(client, arg);
+ mutex_unlock(&client->ioctl_mutex);
+ return err;
+ }
  }
  snd_printd("seq unknown ioctl() 0x%x (type='%c', number=0x%02x)\n",
    cmd, _IOC_TYPE(cmd), _IOC_NR(cmd));
  return -ENOTTY;
 }
 
-
 static long snd_seq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
  struct snd_seq_client *client = file->private_data;
diff --git a/sound/core/seq/seq_clientmgr.h b/sound/core/seq/seq_clientmgr.h
index 20f0a725ec7d..91f8f165bfdc 100644
--- a/sound/core/seq/seq_clientmgr.h
+++ b/sound/core/seq/seq_clientmgr.h
@@ -59,6 +59,7 @@ struct snd_seq_client {
  struct list_head ports_list_head;
  rwlock_t ports_lock;
  struct mutex ports_mutex;
+ struct mutex ioctl_mutex;
  int convert32; /* convert 32->64bit */
 
  /* output pool */
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/4] ALSA: seq: Fix racy pool initializations

Tyler Hicks-2
In reply to this post by Tyler Hicks-2
From: Takashi Iwai <[hidden email]>

ALSA sequencer core initializes the event pool on demand by invoking
snd_seq_pool_init() when the first write happens and the pool is
empty.  Meanwhile user can reset the pool size manually via ioctl
concurrently, and this may lead to UAF or out-of-bound accesses since
the function tries to vmalloc / vfree the buffer.

A simple fix is to just wrap the snd_seq_pool_init() call with the
recently introduced client->ioctl_mutex; as the calls for
snd_seq_pool_init() from other side are always protected with this
mutex, we can avoid the race.

Reported-by: 范龙飞 <[hidden email]>
Cc: <[hidden email]>
Signed-off-by: Takashi Iwai <[hidden email]>

CVE-2018-7566

(cherry picked from commit d15d662e89fc667b90cd294b0eb45694e33144da)
Signed-off-by: Tyler Hicks <[hidden email]>
---
 sound/core/seq/seq_clientmgr.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c
index cd539ac0e236..275bf2fd2905 100644
--- a/sound/core/seq/seq_clientmgr.c
+++ b/sound/core/seq/seq_clientmgr.c
@@ -1000,7 +1000,7 @@ static ssize_t snd_seq_write(struct file *file, const char __user *buf,
 {
  struct snd_seq_client *client = file->private_data;
  int written = 0, len;
- int err = -EINVAL;
+ int err;
  struct snd_seq_event event;
 
  if (!(snd_seq_file_flags(file) & SNDRV_SEQ_LFLG_OUTPUT))
@@ -1015,11 +1015,15 @@ static ssize_t snd_seq_write(struct file *file, const char __user *buf,
 
  /* allocate the pool now if the pool is not allocated yet */
  if (client->pool->size > 0 && !snd_seq_write_pool_allocated(client)) {
- if (snd_seq_pool_init(client->pool) < 0)
+ mutex_lock(&client->ioctl_mutex);
+ err = snd_seq_pool_init(client->pool);
+ mutex_unlock(&client->ioctl_mutex);
+ if (err < 0)
  return -ENOMEM;
  }
 
  /* only process whole events */
+ err = -EINVAL;
  while (count >= sizeof(struct snd_seq_event)) {
  /* Read in the event header from the user */
  len = sizeof(event);
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/4] ALSA: seq: Don't allow resizing pool in use

Tyler Hicks-2
In reply to this post by Tyler Hicks-2
From: Takashi Iwai <[hidden email]>

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: 范龙飞 <[hidden email]>
Reported-by: Nicolai Stange <[hidden email]>
Cc: <[hidden email]>
Signed-off-by: Takashi Iwai <[hidden email]>

CVE-2018-7566

(cherry picked from commit d85739367c6d56e475c281945c68fdb05ca74b4c)
Signed-off-by: Tyler Hicks <[hidden email]>
---
 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 275bf2fd2905..7dd3939ba096 100644
--- a/sound/core/seq/seq_clientmgr.c
+++ b/sound/core/seq/seq_clientmgr.c
@@ -1917,6 +1917,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_queue_client_leave_cells(client->number);
  snd_seq_pool_done(client->pool);
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Tyler Hicks-2
In reply to this post by Tyler Hicks-2
From: Takashi Iwai <[hidden email]>

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: 范龙飞 <[hidden email]>
Reported-by: Nicolai Stange <[hidden email]>
Reviewed-and-tested-by: Nicolai Stange <[hidden email]>
Cc: <[hidden email]>
Signed-off-by: Takashi Iwai <[hidden email]>

CVE-2018-7566

(cherry picked from commit 7bd80091567789f1c0cb70eb4737aac8bcd2b6b9)
Signed-off-by: Tyler Hicks <[hidden email]>
---
 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 7dd3939ba096..b110d352019d 100644
--- a/sound/core/seq/seq_clientmgr.c
+++ b/sound/core/seq/seq_clientmgr.c
@@ -907,7 +907,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;
@@ -945,7 +946,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;
 
@@ -1014,12 +1016,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 */
@@ -1070,7 +1071,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;
 
@@ -1081,6 +1082,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;
 }
 
@@ -2343,7 +2346,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 42c9bd8c3f42..5ef7e9d9b728 100644
--- a/sound/core/seq/seq_fifo.c
+++ b/sound/core/seq/seq_fifo.c
@@ -120,7 +120,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)
  atomic_inc(&f->overflow);
diff --git a/sound/core/seq/seq_memory.c b/sound/core/seq/seq_memory.c
index 36c46e661bac..aaa5fb2232b2 100644
--- a/sound/core/seq/seq_memory.c
+++ b/sound/core/seq/seq_memory.c
@@ -221,7 +221,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;
@@ -245,7 +246,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? */
@@ -288,7 +293,7 @@ __error:
  */
 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;
@@ -305,7 +310,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;
 
@@ -331,7 +336,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 4a2ec779b8a7..1cc822dd35c7 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.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team