| Message ID | 20180308071856.12149-2-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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);
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(+)