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

[Amadeusz SÅawiÅski]

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@xxxxxxx>
> 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@xxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> Link: https://lore.kernel.org/r/CAFcO6XMWpUVK_yzzCpp8_XP7+=oUpQvuBeCbMffEDkpe8jWrfg@xxxxxxxxxxxxxx
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> ---
>   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?