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@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? Erm no, this is obviously wrong. The error path needs re-lock. Will respin the fix. thanks, Takashi
