[alsa-devel] [PATCH 0/5] ALSA: seq: More cleanups and fixes
Hi,
this is a patchset for ALSA sequencer to clean up / minor improvement of some code as well as the fixes for remaining issues caught (spontaneously) by syzkaller.
Takashi
===
Takashi Iwai (5): ALSA: seq: Use kvmalloc() for cell pools ALSA: seq: Align temporary re-locking with irqsave version ALSA: seq: Remove superfluous irqsave flags ALSA: seq: Protect in-kernel ioctl calls with mutex ALSA: seq: Fix race of get-subscription call vs port-delete ioctls
sound/core/seq/seq_clientmgr.c | 38 +++++++++++++++++--------------------- sound/core/seq/seq_fifo.c | 14 ++++++-------- sound/core/seq/seq_memory.c | 30 ++++++++++++++---------------- sound/core/seq/seq_ports.c | 28 ++++++++++++++-------------- sound/core/seq/seq_ports.h | 5 +++-- 5 files changed, 54 insertions(+), 61 deletions(-)
Use kvmalloc() for allocating cell pools since the pool size can be relatively small that may be covered better by slab.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/seq/seq_memory.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/sound/core/seq/seq_memory.c b/sound/core/seq/seq_memory.c index 5b0388202bac..6ea4d8a5a71e 100644 --- a/sound/core/seq/seq_memory.c +++ b/sound/core/seq/seq_memory.c @@ -24,7 +24,7 @@ #include <linux/export.h> #include <linux/slab.h> #include <linux/sched/signal.h> -#include <linux/vmalloc.h> +#include <linux/mm.h> #include <sound/core.h>
#include <sound/seq_kernel.h> @@ -389,8 +389,8 @@ int snd_seq_pool_init(struct snd_seq_pool *pool) if (snd_BUG_ON(!pool)) return -EINVAL;
- cellptr = vmalloc(array_size(sizeof(struct snd_seq_event_cell), - pool->size)); + cellptr = kvmalloc_array(sizeof(struct snd_seq_event_cell), pool->size, + GFP_KERNEL); if (!cellptr) return -ENOMEM;
@@ -398,7 +398,7 @@ int snd_seq_pool_init(struct snd_seq_pool *pool) spin_lock_irqsave(&pool->lock, flags); if (pool->ptr) { spin_unlock_irqrestore(&pool->lock, flags); - vfree(cellptr); + kvfree(cellptr); return 0; }
@@ -456,7 +456,7 @@ int snd_seq_pool_done(struct snd_seq_pool *pool) pool->total_elements = 0; spin_unlock_irqrestore(&pool->lock, flags);
- vfree(ptr); + kvfree(ptr);
spin_lock_irqsave(&pool->lock, flags); pool->closing = 0;
In a few places in sequencer core, we temporarily unlock / re-lock the pool spin lock while waiting for the allocation in the blocking mode. There spin_unlock_irq() / spin_lock_irq() pairs are called while initially spin_lock_irqsave() is used (and spin_lock_irqrestore() at the end of the function again). This is likely OK for now, but it's a bit confusing and error-prone.
This patch replaces these temporary relocking lines with the irqsave variant to make the lock/unlock sequence more consistently.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/seq/seq_fifo.c | 4 ++-- sound/core/seq/seq_memory.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/core/seq/seq_fifo.c b/sound/core/seq/seq_fifo.c index 72c0302a55d2..613ae10d33b8 100644 --- a/sound/core/seq/seq_fifo.c +++ b/sound/core/seq/seq_fifo.c @@ -195,9 +195,9 @@ int snd_seq_fifo_cell_out(struct snd_seq_fifo *f, } set_current_state(TASK_INTERRUPTIBLE); add_wait_queue(&f->input_sleep, &wait); - spin_unlock_irq(&f->lock); + spin_unlock_irqrestore(&f->lock, flags); schedule(); - spin_lock_irq(&f->lock); + spin_lock_irqsave(&f->lock, flags); remove_wait_queue(&f->input_sleep, &wait); if (signal_pending(current)) { spin_unlock_irqrestore(&f->lock, flags); diff --git a/sound/core/seq/seq_memory.c b/sound/core/seq/seq_memory.c index 6ea4d8a5a71e..ae0b8971f6ce 100644 --- a/sound/core/seq/seq_memory.c +++ b/sound/core/seq/seq_memory.c @@ -244,13 +244,13 @@ 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); + spin_unlock_irqrestore(&pool->lock, flags); if (mutexp) mutex_unlock(mutexp); schedule(); if (mutexp) mutex_lock(mutexp); - spin_lock_irq(&pool->lock); + spin_lock_irqsave(&pool->lock, flags); remove_wait_queue(&pool->output_sleep, &wait); /* interrupted? */ if (signal_pending(current)) {
spin_lock_irqsave() is used unnecessarily in various places in sequencer core code although it's pretty obvious that the context is sleepable. Remove irqsave and use the plain spin_lock_irq() in such places for simplicity.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/seq/seq_clientmgr.c | 19 ++++++++----------- sound/core/seq/seq_fifo.c | 10 ++++------ sound/core/seq/seq_memory.c | 16 +++++++--------- sound/core/seq/seq_ports.c | 15 ++++++--------- 4 files changed, 25 insertions(+), 35 deletions(-)
diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 7d4640d1fe9f..933bde3843d9 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -203,7 +203,6 @@ int __init client_init_data(void)
static struct snd_seq_client *seq_create_client1(int client_index, int poolsize) { - unsigned long flags; int c; struct snd_seq_client *client;
@@ -224,7 +223,7 @@ static struct snd_seq_client *seq_create_client1(int client_index, int poolsize) mutex_init(&client->ioctl_mutex);
/* find free slot in the client table */ - spin_lock_irqsave(&clients_lock, flags); + spin_lock_irq(&clients_lock); if (client_index < 0) { for (c = SNDRV_SEQ_DYNAMIC_CLIENTS_BEGIN; c < SNDRV_SEQ_MAX_CLIENTS; @@ -232,17 +231,17 @@ static struct snd_seq_client *seq_create_client1(int client_index, int poolsize) if (clienttab[c] || clienttablock[c]) continue; clienttab[client->number = c] = client; - spin_unlock_irqrestore(&clients_lock, flags); + spin_unlock_irq(&clients_lock); return client; } } else { if (clienttab[client_index] == NULL && !clienttablock[client_index]) { clienttab[client->number = client_index] = client; - spin_unlock_irqrestore(&clients_lock, flags); + spin_unlock_irq(&clients_lock); return client; } } - spin_unlock_irqrestore(&clients_lock, flags); + spin_unlock_irq(&clients_lock); snd_seq_pool_delete(&client->pool); kfree(client); return NULL; /* no free slot found or busy, return failure code */ @@ -251,23 +250,21 @@ static struct snd_seq_client *seq_create_client1(int client_index, int poolsize)
static int seq_free_client1(struct snd_seq_client *client) { - unsigned long flags; - if (!client) return 0; - spin_lock_irqsave(&clients_lock, flags); + spin_lock_irq(&clients_lock); clienttablock[client->number] = 1; clienttab[client->number] = NULL; - spin_unlock_irqrestore(&clients_lock, flags); + spin_unlock_irq(&clients_lock); snd_seq_delete_all_ports(client); snd_seq_queue_client_leave(client->number); snd_use_lock_sync(&client->use_lock); snd_seq_queue_client_termination(client->number); if (client->pool) snd_seq_pool_delete(&client->pool); - spin_lock_irqsave(&clients_lock, flags); + spin_lock_irq(&clients_lock); clienttablock[client->number] = 0; - spin_unlock_irqrestore(&clients_lock, flags); + spin_unlock_irq(&clients_lock); return 0; }
diff --git a/sound/core/seq/seq_fifo.c b/sound/core/seq/seq_fifo.c index 613ae10d33b8..97ee89cb6426 100644 --- a/sound/core/seq/seq_fifo.c +++ b/sound/core/seq/seq_fifo.c @@ -98,18 +98,17 @@ static struct snd_seq_event_cell *fifo_cell_out(struct snd_seq_fifo *f); void snd_seq_fifo_clear(struct snd_seq_fifo *f) { struct snd_seq_event_cell *cell; - unsigned long flags;
/* clear overflow flag */ atomic_set(&f->overflow, 0);
snd_use_lock_sync(&f->use_lock); - spin_lock_irqsave(&f->lock, flags); + spin_lock_irq(&f->lock); /* drain the fifo */ while ((cell = fifo_cell_out(f)) != NULL) { snd_seq_cell_free(cell); } - spin_unlock_irqrestore(&f->lock, flags); + spin_unlock_irq(&f->lock); }
@@ -239,7 +238,6 @@ int snd_seq_fifo_poll_wait(struct snd_seq_fifo *f, struct file *file, /* change the size of pool; all old events are removed */ int snd_seq_fifo_resize(struct snd_seq_fifo *f, int poolsize) { - unsigned long flags; struct snd_seq_pool *newpool, *oldpool; struct snd_seq_event_cell *cell, *next, *oldhead;
@@ -255,7 +253,7 @@ int snd_seq_fifo_resize(struct snd_seq_fifo *f, int poolsize) return -ENOMEM; }
- spin_lock_irqsave(&f->lock, flags); + spin_lock_irq(&f->lock); /* remember old pool */ oldpool = f->pool; oldhead = f->head; @@ -265,7 +263,7 @@ int snd_seq_fifo_resize(struct snd_seq_fifo *f, int poolsize) f->tail = NULL; f->cells = 0; /* NOTE: overflow flag is not cleared */ - spin_unlock_irqrestore(&f->lock, flags); + spin_unlock_irq(&f->lock);
/* close the old pool and wait until all users are gone */ snd_seq_pool_mark_closing(oldpool); diff --git a/sound/core/seq/seq_memory.c b/sound/core/seq/seq_memory.c index ae0b8971f6ce..19b718e871c5 100644 --- a/sound/core/seq/seq_memory.c +++ b/sound/core/seq/seq_memory.c @@ -384,7 +384,6 @@ int snd_seq_pool_init(struct snd_seq_pool *pool) { int cell; struct snd_seq_event_cell *cellptr; - unsigned long flags;
if (snd_BUG_ON(!pool)) return -EINVAL; @@ -395,9 +394,9 @@ int snd_seq_pool_init(struct snd_seq_pool *pool) return -ENOMEM;
/* add new cells to the free cell list */ - spin_lock_irqsave(&pool->lock, flags); + spin_lock_irq(&pool->lock); if (pool->ptr) { - spin_unlock_irqrestore(&pool->lock, flags); + spin_unlock_irq(&pool->lock); kvfree(cellptr); return 0; } @@ -416,7 +415,7 @@ int snd_seq_pool_init(struct snd_seq_pool *pool) /* init statistics */ pool->max_used = 0; pool->total_elements = pool->size; - spin_unlock_irqrestore(&pool->lock, flags); + spin_unlock_irq(&pool->lock); return 0; }
@@ -435,7 +434,6 @@ void snd_seq_pool_mark_closing(struct snd_seq_pool *pool) /* remove events */ int snd_seq_pool_done(struct snd_seq_pool *pool) { - unsigned long flags; struct snd_seq_event_cell *ptr;
if (snd_BUG_ON(!pool)) @@ -449,18 +447,18 @@ int snd_seq_pool_done(struct snd_seq_pool *pool) schedule_timeout_uninterruptible(1); /* release all resources */ - spin_lock_irqsave(&pool->lock, flags); + spin_lock_irq(&pool->lock); ptr = pool->ptr; pool->ptr = NULL; pool->free = NULL; pool->total_elements = 0; - spin_unlock_irqrestore(&pool->lock, flags); + spin_unlock_irq(&pool->lock);
kvfree(ptr);
- spin_lock_irqsave(&pool->lock, flags); + spin_lock_irq(&pool->lock); pool->closing = 0; - spin_unlock_irqrestore(&pool->lock, flags); + spin_unlock_irq(&pool->lock);
return 0; } diff --git a/sound/core/seq/seq_ports.c b/sound/core/seq/seq_ports.c index 24d90abfc64d..1e2239240f21 100644 --- a/sound/core/seq/seq_ports.c +++ b/sound/core/seq/seq_ports.c @@ -128,7 +128,6 @@ static void port_subs_info_init(struct snd_seq_port_subs_info *grp) struct snd_seq_client_port *snd_seq_create_port(struct snd_seq_client *client, int port) { - unsigned long flags; struct snd_seq_client_port *new_port, *p; int num = -1; @@ -157,7 +156,7 @@ struct snd_seq_client_port *snd_seq_create_port(struct snd_seq_client *client,
num = port >= 0 ? port : 0; mutex_lock(&client->ports_mutex); - write_lock_irqsave(&client->ports_lock, flags); + write_lock_irq(&client->ports_lock); list_for_each_entry(p, &client->ports_list_head, list) { if (p->addr.port > num) break; @@ -169,7 +168,7 @@ struct snd_seq_client_port *snd_seq_create_port(struct snd_seq_client *client, client->num_ports++; new_port->addr.port = num; /* store the port number in the port */ sprintf(new_port->name, "port-%d", num); - write_unlock_irqrestore(&client->ports_lock, flags); + write_unlock_irq(&client->ports_lock); mutex_unlock(&client->ports_mutex);
return new_port; @@ -283,11 +282,10 @@ static int port_delete(struct snd_seq_client *client, /* delete a port with the given port id */ int snd_seq_delete_port(struct snd_seq_client *client, int port) { - unsigned long flags; struct snd_seq_client_port *found = NULL, *p;
mutex_lock(&client->ports_mutex); - write_lock_irqsave(&client->ports_lock, flags); + write_lock_irq(&client->ports_lock); list_for_each_entry(p, &client->ports_list_head, list) { if (p->addr.port == port) { /* ok found. delete from the list at first */ @@ -297,7 +295,7 @@ int snd_seq_delete_port(struct snd_seq_client *client, int port) break; } } - write_unlock_irqrestore(&client->ports_lock, flags); + write_unlock_irq(&client->ports_lock); mutex_unlock(&client->ports_mutex); if (found) return port_delete(client, found); @@ -308,7 +306,6 @@ int snd_seq_delete_port(struct snd_seq_client *client, int port) /* delete the all ports belonging to the given client */ int snd_seq_delete_all_ports(struct snd_seq_client *client) { - unsigned long flags; struct list_head deleted_list; struct snd_seq_client_port *port, *tmp; @@ -316,7 +313,7 @@ int snd_seq_delete_all_ports(struct snd_seq_client *client) * clear the port list in the client data. */ mutex_lock(&client->ports_mutex); - write_lock_irqsave(&client->ports_lock, flags); + write_lock_irq(&client->ports_lock); if (! list_empty(&client->ports_list_head)) { list_add(&deleted_list, &client->ports_list_head); list_del_init(&client->ports_list_head); @@ -324,7 +321,7 @@ int snd_seq_delete_all_ports(struct snd_seq_client *client) INIT_LIST_HEAD(&deleted_list); } client->num_ports = 0; - write_unlock_irqrestore(&client->ports_lock, flags); + write_unlock_irq(&client->ports_lock);
/* remove each port in deleted_list */ list_for_each_entry_safe(port, tmp, &deleted_list, list) {
ALSA OSS sequencer calls the ioctl function indirectly via snd_seq_kernel_client_ctl(). While we already applied the protection against races between the normal ioctls and writes via the client's ioctl_mutex, this code path was left untouched. And this seems to be the cause of still remaining some rare UAF as spontaneously triggered by syzkaller.
For the sake of robustness, wrap the ioctl_mutex also for the call via snd_seq_kernel_client_ctl(), too.
Reported-by: syzbot+e4c8abb920efa77bace9@syzkaller.appspotmail.com Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/seq/seq_clientmgr.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 933bde3843d9..976404691261 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -2340,14 +2340,19 @@ int snd_seq_kernel_client_ctl(int clientid, unsigned int cmd, void *arg) { const struct ioctl_handler *handler; struct snd_seq_client *client; + int err;
client = clientptr(clientid); if (client == NULL) return -ENXIO;
for (handler = ioctl_handlers; handler->cmd > 0; ++handler) { - if (handler->cmd == cmd) - return handler->func(client, arg); + if (handler->cmd == cmd) { + mutex_lock(&client->ioctl_mutex); + err = handler->func(client, arg); + mutex_unlock(&client->ioctl_mutex); + return err; + } }
pr_debug("ALSA: seq unknown ioctl() 0x%x (type='%c', number=0x%02x)\n",
The snd_seq_ioctl_get_subscription() retrieves the port subscriber information as a pointer, while the object isn't protected, hence it may be deleted before the actual reference. This race was spotted by syzkaller and may lead to a UAF.
The fix is simply copying the data in the lookup function that performs in the rwsem to protect against the deletion.
Reported-by: syzbot+9437020c82413d00222d@syzkaller.appspotmail.com Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/seq/seq_clientmgr.c | 10 ++-------- sound/core/seq/seq_ports.c | 13 ++++++++----- sound/core/seq/seq_ports.h | 5 +++-- 3 files changed, 13 insertions(+), 15 deletions(-)
diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 976404691261..f256704dc401 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -1897,20 +1897,14 @@ static int snd_seq_ioctl_get_subscription(struct snd_seq_client *client, int result; struct snd_seq_client *sender = NULL; struct snd_seq_client_port *sport = NULL; - struct snd_seq_subscribers *p;
result = -EINVAL; if ((sender = snd_seq_client_use_ptr(subs->sender.client)) == NULL) goto __end; if ((sport = snd_seq_port_use_ptr(sender, subs->sender.port)) == NULL) goto __end; - p = snd_seq_port_get_subscription(&sport->c_src, &subs->dest); - if (p) { - result = 0; - *subs = p->info; - } else - result = -ENOENT; - + result = snd_seq_port_get_subscription(&sport->c_src, &subs->dest, + subs); __end: if (sport) snd_seq_port_unlock(sport); diff --git a/sound/core/seq/seq_ports.c b/sound/core/seq/seq_ports.c index 1e2239240f21..d964d728681e 100644 --- a/sound/core/seq/seq_ports.c +++ b/sound/core/seq/seq_ports.c @@ -632,20 +632,23 @@ int snd_seq_port_disconnect(struct snd_seq_client *connector,
/* get matched subscriber */ -struct snd_seq_subscribers *snd_seq_port_get_subscription(struct snd_seq_port_subs_info *src_grp, - struct snd_seq_addr *dest_addr) +int snd_seq_port_get_subscription(struct snd_seq_port_subs_info *src_grp, + struct snd_seq_addr *dest_addr, + struct snd_seq_port_subscribe *subs) { - struct snd_seq_subscribers *s, *found = NULL; + struct snd_seq_subscribers *s; + int err = -ENOENT;
down_read(&src_grp->list_mutex); list_for_each_entry(s, &src_grp->list_head, src_list) { if (addr_match(dest_addr, &s->info.dest)) { - found = s; + *subs = s->info; + err = 0; break; } } up_read(&src_grp->list_mutex); - return found; + return err; }
/* diff --git a/sound/core/seq/seq_ports.h b/sound/core/seq/seq_ports.h index 26bd71f36c41..06003b36652e 100644 --- a/sound/core/seq/seq_ports.h +++ b/sound/core/seq/seq_ports.h @@ -135,7 +135,8 @@ int snd_seq_port_subscribe(struct snd_seq_client_port *port, struct snd_seq_port_subscribe *info);
/* get matched subscriber */ -struct snd_seq_subscribers *snd_seq_port_get_subscription(struct snd_seq_port_subs_info *src_grp, - struct snd_seq_addr *dest_addr); +int snd_seq_port_get_subscription(struct snd_seq_port_subs_info *src_grp, + struct snd_seq_addr *dest_addr, + struct snd_seq_port_subscribe *subs);
#endif
participants (1)
-
Takashi Iwai