Re: [PATCH] ALSA: seq: 2nd attempt at fixing race creating a q

[Takashi Iwai]

On Fri, 11 Aug 2017 23:23:42 +0200,
Daniel Mentz wrote:
> 
> On Fri, Aug 11, 2017 at 7:42 AM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> >
> > On Fri, 11 Aug 2017 05:07:34 +0200,
> > Daniel Mentz wrote:
> > >
> > > commit 4842e98f26dd80be3623c4714a244ba52ea096a8 ("ALSA: seq: Fix race at
> > > creating a queue") attempted to fix a race reported by syzkaller. That
> > > fix has been described as follows:
> > >
> > > "
> > > When a sequencer queue is created in snd_seq_queue_alloc(),it adds the
> > > new queue element to the public list before referencing it.  Thus the
> > > queue might be deleted before the call of snd_seq_queue_use(), and it
> > > results in the use-after-free error, as spotted by syzkaller.
> > >
> > > The fix is to reference the queue object at the right time.
> > > "
> > >
> > > The last phrase in that commit message refers to calling queue_use(q, client,
> > > 1) which increments queue->clients, but that does not prevent the
> > > DELETE_QUEUE ioctl() and queue_delete() from kfree()ing the queue.
> > > Hence, the commit is not effective at preventing the race.
> >
> > kfree() is performed only after snd_use_lock_sync(), thus by acquiring
> > snd_use_lock() it doesn't race.  If it were already deleted,
> > queue_use() returns NULL so it's also fine.
> 
> queue_use() is defined to return void. I am assuming you're referring
> to queueptr().
> 
> Where do you acquire the snd_use_lock i.e. where do you call
> snd_use_lock_use()? My understanding is that it's called from
> queueptr(). With that said, there is a tiny gap between the new queue
> being added to the list in queue_list_add() (from
> snd_seq_queue_alloc()) and the call to queueptr() in
> snd_seq_ioctl_create_queue(). syzkaller specifically points to the
> last instruction "return q->queue;" in snd_seq_queue_alloc(). This is
> *after* q has been added to the list (and is therefore visible to
> other threads) but *before* it has been locked through
> snd_seq_ioctl_create_queue()->queueptr()->snd_use_lock_use(). Hence,
> there is a possibility that the pointer q is no longer valid.
> You could capture the return value from queue_list_add() which is
> identical to q->queue and then return that. However, the other issue
> is that queueptr() indeed returns NULL if the queue with that index
> has been deleted, but it's theoretically possible that the queue has
> been deleted and a *different* been created that now occupies the same
> spot in the queue_list.

OK, now I understood.  It's a small race window between
queue_list_add() and queueptr().  This should be clarified better in
the description.  No too much details are needed.  The only point is 
where the race window is opened against which.

> > Or do you actually see any crash or the wild behavior?
> 
> syzkaller reported a crash:

Then it's a real bug, so need to be fixed quickly.
Could you rephrase your changelog text including the crash information 
briefly, and resubmit?  The code change itself looks good.


thanks,

Takashi