lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sat, 12 Aug 2017 08:43:44 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Daniel Mentz <danielmentz@...gle.com>
Cc:     alsa-devel@...a-project.org, lkml <linux-kernel@...r.kernel.org>,
        Jaroslav Kysela <perex@...ex.cz>, stable@...r.kernel.org
Subject: Re: [PATCH] ALSA: seq: 2nd attempt at fixing race creating a q

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

Powered by blists - more mailing lists