Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 07 May 2020 12:36:14 +0200
Message-ID: <s5hpnbg3uch.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Amadeusz SX2awiX4ski <amadeuszx.slawinski@linux.intel.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        security@kernel.org, alsa-devel@alsa-project.org,
        butt3rflyh4ck <butterflyhuangxx@gmail.com>, tiwai@suse.com,
        linux-kernel@vger.kernel.org,
        syzkaller <syzkaller@googlegroups.com>
Subject: Re: KASAN: use-after-free Write in snd_rawmidi_kernel_write1
In-Reply-To: <ecc5f76f-2e87-f634-b98c-9fbcad177d72@linux.intel.com>
References: <CAFcO6XMGT42wFBxEa01Ee5Msuecm+WiXnn4rc-VWkC4vTzycPg@mail.gmail.com>
        <20200507082302.GF1024567@kroah.com>
        <s5h8si45ard.wl-tiwai@suse.de>
        <ecc5f76f-2e87-f634-b98c-9fbcad177d72@linux.intel.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.3
 (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
Precedence: bulk

On Thu, 07 May 2020 12:27:41 +0200,
Amadeusz SX2awiX4ski wrote:
> 
> 
> 
> On 5/7/2020 11:56 AM, 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
> 
> (...)
> 
> > @@ -1021,6 +1039,7 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream,
> >   	unsigned long appl_ptr;
> >     	spin_lock_irqsave(&runtime->lock, flags);
> > +	snd_rawmidi_buffer_ref(runtime);
> >   	while (count > 0 && runtime->avail) {
> >   		count1 = runtime->buffer_size - runtime->appl_ptr;
> >   		if (count1 > count)
> > @@ -1040,13 +1059,17 @@ static long snd_rawmidi_kernel_read1(struct snd_rawmidi_substream *substream,
> >   			spin_unlock_irqrestore(&runtime->lock, flags);
> First unlock
> >   			if (copy_to_user(userbuf + result,
> >   					 runtime->buffer + appl_ptr, count1)) {
> > -				return result > 0 ? result : -EFAULT;
> > +				if (!result)
> > +					result = -EFAULT;
> > +				goto out;
> 
> goto -> Second unlock
> >   			}
> >   			spin_lock_irqsave(&runtime->lock, flags);
> >   		}
> >   		result += count1;
> >   		count -= count1;
> >   	}
> > + out:
> > +	snd_rawmidi_buffer_unref(runtime);
> >   	spin_unlock_irqrestore(&runtime->lock, flags);
> Second unlock
> >   	return result;
> >   }
> 
> So if I follow this correctly, you call spin_unlock_irqrestore twice
> in case of error?

Erm no, this is obviously wrong.  The error path needs re-lock.
Will respin the fix.


thanks,

Takashi