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 <ti...@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 <butterflyhuan...@gmail.com>
Cc: <sta...@vger.kernel.org>
Link: 
https://lore.kernel.org/r/CAFcO6XMWpUVK_yzzCpp8_XP7+=oupqvubecbmffedkpe8jw...@mail.gmail.com
Signed-off-by: Takashi Iwai <ti...@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?