* [PATCH] ALSA: seq: Fix use-after-free at creating a port
@ 2017-10-11 8:00 Takashi Iwai
2017-10-11 17:35 ` Linus Torvalds
0 siblings, 1 reply; 3+ messages in thread
From: Takashi Iwai @ 2017-10-11 8:00 UTC (permalink / raw)
To: alsa-devel; +Cc: Michael23 Yu, Linus Torvalds
There is a potential race window opened at creating and deleting a
port via ioctl, as spotted by fuzzing. snd_seq_create_port() creates
a port object and returns its pointer, but it doesn't take the
refcount, thus it can be deleted immediately by another thread.
Meanwhile, snd_seq_ioctl_create_port() still calls the function
snd_seq_system_client_ev_port_start() with the created port object
that is being deleted, and this triggers use-after-free like:
BUG: KASAN: use-after-free in snd_seq_ioctl_create_port+0x504/0x630 [snd_seq] at addr ffff8801f2241cb1
=============================================================================
BUG kmalloc-512 (Tainted: G B ): kasan: bad access detected
-----------------------------------------------------------------------------
INFO: Allocated in snd_seq_create_port+0x94/0x9b0 [snd_seq] age=1 cpu=3 pid=4511
___slab_alloc+0x425/0x460
__slab_alloc+0x20/0x40
kmem_cache_alloc_trace+0x150/0x190
snd_seq_create_port+0x94/0x9b0 [snd_seq]
snd_seq_ioctl_create_port+0xd1/0x630 [snd_seq]
snd_seq_do_ioctl+0x11c/0x190 [snd_seq]
snd_seq_ioctl+0x40/0x80 [snd_seq]
do_vfs_ioctl+0x54b/0xda0
SyS_ioctl+0x79/0x90
entry_SYSCALL_64_fastpath+0x16/0x75
INFO: Freed in port_delete+0x136/0x1a0 [snd_seq] age=1 cpu=2 pid=4717
__slab_free+0x204/0x310
kfree+0x15f/0x180
port_delete+0x136/0x1a0 [snd_seq]
snd_seq_delete_port+0x235/0x350 [snd_seq]
snd_seq_ioctl_delete_port+0xc8/0x180 [snd_seq]
snd_seq_do_ioctl+0x11c/0x190 [snd_seq]
snd_seq_ioctl+0x40/0x80 [snd_seq]
do_vfs_ioctl+0x54b/0xda0
SyS_ioctl+0x79/0x90
entry_SYSCALL_64_fastpath+0x16/0x75
Call Trace:
[<ffffffff81b03781>] dump_stack+0x63/0x82
[<ffffffff81531b3b>] print_trailer+0xfb/0x160
[<ffffffff81536db4>] object_err+0x34/0x40
[<ffffffff815392d3>] kasan_report.part.2+0x223/0x520
[<ffffffffa07aadf4>] ? snd_seq_ioctl_create_port+0x504/0x630 [snd_seq]
[<ffffffff815395fe>] __asan_report_load1_noabort+0x2e/0x30
[<ffffffffa07aadf4>] snd_seq_ioctl_create_port+0x504/0x630 [snd_seq]
[<ffffffffa07aa8f0>] ? snd_seq_ioctl_delete_port+0x180/0x180 [snd_seq]
[<ffffffff8136be50>] ? taskstats_exit+0xbc0/0xbc0
[<ffffffffa07abc5c>] snd_seq_do_ioctl+0x11c/0x190 [snd_seq]
[<ffffffffa07abd10>] snd_seq_ioctl+0x40/0x80 [snd_seq]
[<ffffffff8136d433>] ? acct_account_cputime+0x63/0x80
[<ffffffff815b515b>] do_vfs_ioctl+0x54b/0xda0
.....
We may fix this in a few different ways, and in this patch, it's fixed
simply by taking the refcount properly at snd_seq_create_port() and
letting the caller unref the object after use. Also, there is another
potential use-after-free by sprintf() call in snd_seq_create_port(),
and this is moved inside the lock.
This fix covers CVE-2017-15265.
Reported-and-tested-by: Michael23 Yu <ycqzsy@gmail.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
sound/core/seq/seq_clientmgr.c | 6 +++++-
sound/core/seq/seq_ports.c | 7 +++++--
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c
index ea2d0ae85bd3..6c9cba2166d9 100644
--- a/sound/core/seq/seq_clientmgr.c
+++ b/sound/core/seq/seq_clientmgr.c
@@ -1259,6 +1259,7 @@ static int snd_seq_ioctl_create_port(struct snd_seq_client *client, void *arg)
struct snd_seq_port_info *info = arg;
struct snd_seq_client_port *port;
struct snd_seq_port_callback *callback;
+ int port_idx;
/* it is not allowed to create the port for an another client */
if (info->addr.client != client->number)
@@ -1269,7 +1270,9 @@ static int snd_seq_ioctl_create_port(struct snd_seq_client *client, void *arg)
return -ENOMEM;
if (client->type == USER_CLIENT && info->kernel) {
- snd_seq_delete_port(client, port->addr.port);
+ port_idx = port->addr.port;
+ snd_seq_port_unlock(port);
+ snd_seq_delete_port(client, port_idx);
return -EINVAL;
}
if (client->type == KERNEL_CLIENT) {
@@ -1290,6 +1293,7 @@ static int snd_seq_ioctl_create_port(struct snd_seq_client *client, void *arg)
snd_seq_set_port_info(port, info);
snd_seq_system_client_ev_port_start(port->addr.client, port->addr.port);
+ snd_seq_port_unlock(port);
return 0;
}
diff --git a/sound/core/seq/seq_ports.c b/sound/core/seq/seq_ports.c
index 0a7020c82bfc..d21ece9f8d73 100644
--- a/sound/core/seq/seq_ports.c
+++ b/sound/core/seq/seq_ports.c
@@ -122,7 +122,9 @@ static void port_subs_info_init(struct snd_seq_port_subs_info *grp)
}
-/* create a port, port number is returned (-1 on failure) */
+/* create a port, port number is returned (-1 on failure);
+ * the caller needs to unref the port via snd_seq_port_unlock() appropriately
+ */
struct snd_seq_client_port *snd_seq_create_port(struct snd_seq_client *client,
int port)
{
@@ -151,6 +153,7 @@ struct snd_seq_client_port *snd_seq_create_port(struct snd_seq_client *client,
snd_use_lock_init(&new_port->use_lock);
port_subs_info_init(&new_port->c_src);
port_subs_info_init(&new_port->c_dest);
+ snd_use_lock_use(&new_port->use_lock);
num = port >= 0 ? port : 0;
mutex_lock(&client->ports_mutex);
@@ -165,9 +168,9 @@ struct snd_seq_client_port *snd_seq_create_port(struct snd_seq_client *client,
list_add_tail(&new_port->list, &p->list);
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);
mutex_unlock(&client->ports_mutex);
- sprintf(new_port->name, "port-%d", num);
return new_port;
}
--
2.14.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ALSA: seq: Fix use-after-free at creating a port
2017-10-11 8:00 [PATCH] ALSA: seq: Fix use-after-free at creating a port Takashi Iwai
@ 2017-10-11 17:35 ` Linus Torvalds
2017-10-11 18:05 ` Takashi Iwai
0 siblings, 1 reply; 3+ messages in thread
From: Linus Torvalds @ 2017-10-11 17:35 UTC (permalink / raw)
To: Takashi Iwai
Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
Michael23 Yu
On Wed, Oct 11, 2017 at 1:00 AM, Takashi Iwai <tiwai@suse.de> wrote:
> There is a potential race window opened at creating and deleting a
> port via ioctl, as spotted by fuzzing. [..]
I'm assuming I'll just get this through the normal sound fixes pull request?
Also, just for future issues, I really wish people would edit the call
traces to be a bit more legible. You did remove some of the excessive
KASAN noise, but the stack trace hex numbers are a bit distracting
too.
Of course, the only reason those hex numbers are there in the first
place was that the oops was generated with an older kernel - we don't
even print them out any more.
Not a big deal, but I thought I'd mention it just because I generally
encourage people to try to edit down oopses etc to the really relevant
parts (the kernel often spits out a lot of stuff that isn't really
relevant for the particular bug, because it *might* be)
Thanks,
Linus
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ALSA: seq: Fix use-after-free at creating a port
2017-10-11 17:35 ` Linus Torvalds
@ 2017-10-11 18:05 ` Takashi Iwai
0 siblings, 0 replies; 3+ messages in thread
From: Takashi Iwai @ 2017-10-11 18:05 UTC (permalink / raw)
To: Linus Torvalds
Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
Michael23 Yu
On Wed, 11 Oct 2017 19:35:46 +0200,
Linus Torvalds wrote:
>
> On Wed, Oct 11, 2017 at 1:00 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > There is a potential race window opened at creating and deleting a
> > port via ioctl, as spotted by fuzzing. [..]
>
> I'm assuming I'll just get this through the normal sound fixes pull request?
Yes, I'll send you a pull request tomorrow together with other fixes.
> Also, just for future issues, I really wish people would edit the call
> traces to be a bit more legible. You did remove some of the excessive
> KASAN noise, but the stack trace hex numbers are a bit distracting
> too.
>
> Of course, the only reason those hex numbers are there in the first
> place was that the oops was generated with an older kernel - we don't
> even print them out any more.
>
> Not a big deal, but I thought I'd mention it just because I generally
> encourage people to try to edit down oopses etc to the really relevant
> parts (the kernel often spits out a lot of stuff that isn't really
> relevant for the particular bug, because it *might* be)
OK, noted.
Now after seeing lots of commits with stack trace messages in this
week (I warn you beforehand!), I share the same feeling.
Will tidy up harder at the next time.
Thanks!
Takashi
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-10-11 18:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-11 8:00 [PATCH] ALSA: seq: Fix use-after-free at creating a port Takashi Iwai
2017-10-11 17:35 ` Linus Torvalds
2017-10-11 18:05 ` Takashi Iwai
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.