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