On Fri, 11 Aug 2017 23:23:42 +0200, Daniel Mentz wrote:
On Fri, Aug 11, 2017 at 7:42 AM, Takashi Iwai tiwai@suse.de 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,
- 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