Re: KASAN: use-after-free Write in snd_rawmidi_kernel_write1

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, May 07, 2020 at 12:19:18PM +0200, Takashi Iwai wrote:
> On Thu, 07 May 2020 12:13:10 +0200,
> Greg Kroah-Hartman wrote:
> > 
> > On Thu, May 07, 2020 at 11:56:22AM +0200, Takashi Iwai wrote:
> > > On Thu, 07 May 2020 10:23:02 +0200,
> > > Greg Kroah-Hartman wrote:
> > > > 
> > > > On Thu, May 07, 2020 at 04:04:25PM +0800, butt3rflyh4ck wrote:
> > > > > I report a bug (in linux-5.7-rc1) found by syzkaller.
> > > > > 
> > > > > kernel config: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/v5.7.0-rc1.config
> > > > > reproducer: https://github.com/butterflyhack/syzkaller-fuzz/blob/master/repro.cprog
> > > > > 
> > > > > I test the reproducer in linux-5.7-rc4 and crash too.
> > > > 
> > > > Great, care to create a fix for this and send it to the proper
> > > > maintainers?  That's the best way to get it fixed, otherwise it just
> > > > goes in the file with the rest of the syzbot reports we are burried
> > > > under.
> > > 
> > > Don't worry, I already prepared a fix patch below :)
> > > 
> > > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > -- 8< --
> > > From: Takashi Iwai <tiwai@suse.de>
> > > Subject: [PATCH] ALSA: rawmidi: Fix racy buffer resize under concurrent
> > >  accesses
> > > 
> > > The rawmidi core allows user to resize the runtime buffer via ioctl,
> > > and this may lead to UAF when performed during concurrent reads or
> > > writes.
> > > 
> > > This patch fixes the race by introducing a reference counter for the
> > > runtime buffer access and returns -EBUSY error when the resize is
> > > performed concurrently.
> > > 
> > > Reported-by: butt3rflyh4ck <butterflyhuangxx@gmail.com>
> > > Cc: <stable@vger.kernel.org>
> > > Link: https://lore.kernel.org/r/CAFcO6XMWpUVK_yzzCpp8_XP7+=oUpQvuBeCbMffEDkpe8jWrfg@mail.gmail.com
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > >  include/sound/rawmidi.h |  1 +
> > >  sound/core/rawmidi.c    | 29 ++++++++++++++++++++++++++++-
> > >  2 files changed, 29 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/include/sound/rawmidi.h b/include/sound/rawmidi.h
> > > index a36b7227a15a..334842daa904 100644
> > > --- a/include/sound/rawmidi.h
> > > +++ b/include/sound/rawmidi.h
> > > @@ -61,6 +61,7 @@ struct snd_rawmidi_runtime {
> > >  	size_t avail_min;	/* min avail for wakeup */
> > >  	size_t avail;		/* max used buffer for wakeup */
> > >  	size_t xruns;		/* over/underruns counter */
> > > +	int buffer_ref;		/* buffer reference count */
> > >  	/* misc */
> > >  	spinlock_t lock;
> > >  	wait_queue_head_t sleep;
> > > diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
> > > index 20dd08e1f675..4185d9e81e3c 100644
> > > --- a/sound/core/rawmidi.c
> > > +++ b/sound/core/rawmidi.c
> > > @@ -120,6 +120,17 @@ static void snd_rawmidi_input_event_work(struct work_struct *work)
> > >  		runtime->event(runtime->substream);
> > >  }
> > >  
> > > +/* buffer refcount management: call with runtime->lock held */
> > > +static inline void snd_rawmidi_buffer_ref(struct snd_rawmidi_runtime *runtime)
> > > +{
> > > +	runtime->buffer_ref++;
> > > +}
> > > +
> > > +static inline void snd_rawmidi_buffer_unref(struct snd_rawmidi_runtime *runtime)
> > > +{
> > > +	runtime->buffer_ref--;
> > > +}
> > 
> > Why not use the reference count structure?
> 
> The context accessing the buffer is always with the spinlock, so we
> don't need expensive atomic ops there.
> 
> Usually this kind of check can be a simple boolean flag, but in this
> case, there is one place that goes out of lock due to
> copy_from/to_user, so a refcount is used in this patch instead.

Ah, ok, thanks for the explanation.

greg k-h