Date: Sat, 12 Aug 2017 08:43:44 +0200
Message-ID: <s5h4ltdxp3z.wl-tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: Daniel Mentz <danielmentz@google.com>
Cc: alsa-devel@alsa-project.org, lkml <linux-kernel@vger.kernel.org>,
        Jaroslav Kysela <perex@perex.cz>, stable@vger.kernel.org
Subject: Re: [PATCH] ALSA: seq: 2nd attempt at fixing race creating a q
In-Reply-To: <CAE2F3rDKTv+98V7yZ3Hen=Zi5_HVnwW_zHMUDAcSO8SwB9K-GA@mail.gmail.com>
References: <20170811030734.37741-1-danielmentz@google.com>
        <s5h1soidv2u.wl-tiwai@suse.de>
        <CAE2F3rDKTv+98V7yZ3Hen=Zi5_HVnwW_zHMUDAcSO8SwB9K-GA@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.2
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3003
Lines: 68

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,
> > > 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