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?
