[alsa-devel] [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 at 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 at 126.com>
> > Reported-by: Nicolai Stange <nstange at suse.de>
> > Cc: <stable at vger.kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai at 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