From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758364AbeD0Pwr (ORCPT ); Fri, 27 Apr 2018 11:52:47 -0400 Received: from mx2.suse.de ([195.135.220.15]:40671 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757560AbeD0Pwp (ORCPT ); Fri, 27 Apr 2018 11:52:45 -0400 Date: Fri, 27 Apr 2018 17:52:43 +0200 Message-ID: From: Takashi Iwai To: DaeRyong Jeong Cc: perex@perex.cz, alsa-devel@alsa-project.org, kt0755@gmail.com, bammanag@purdue.edu, byoungyoung@purdue.edu, linux-kernel@vger.kernel.org Subject: Re: Unable to handle kernel paging request in snd_seq_oss_readq_puts In-Reply-To: <20180427014858.GA23026@dragonet.kaist.ac.kr> References: <20180426045223.GA15307@dragonet.kaist.ac.kr> <20180427014858.GA23026@dragonet.kaist.ac.kr> 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 27 Apr 2018 03:48:59 +0200, DaeRyong Jeong wrote: > > On Thu, Apr 26, 2018 at 09:17:45AM +0200, Takashi Iwai wrote: > > On Thu, 26 Apr 2018 06:52:27 +0200, > > DaeRyong Jeong wrote: > > > > > > We report the crash: > > > unable to handle kernel paging request in snd_seq_oss_readq_puts > > > > > > This crash has been found in v4.16 using RaceFuzzer (a modified > > > version of Syzkaller), which we describe more at the end of this > > > report. Our analysis shows that the race occurs when invoking two > > > syscalls concurrently, write$eventfd and write$sndseq. > > > > > > Analysis: > > > We think the concurrent execution of snd_virmidi_output_trigger() and > > > snd_midi_event_encode_byte() causes the crash. Since the first call site > > > of snd_seq_kernel_client_dispatch() in snd_virmidi_output_trigger() is not > > > protected by substream->runtime->lock, it is possible that > > > snd_seq_oss_readq_puts() and snd_midi_event_encode_byte() are executed > > > concurrently in the call sequences as below, and snd_seq_oss_readq_puts() > > > accesses ev->data.ex.ptr before it is initialized. > > > > Thanks for the bug report and analysis. > > > > I guess that it's not about initialization but rather other way > > round. The first task sends the pending event with SYSEX that > > contains the buffer pointer in the event packet. Meanwhile, the > > second task now starts processing the MIDI stream and overwrites this > > event packet. So the data address that is being processed is > > overwritten, and it leads to the crash. > > > > Below is the fix patch. It's totally untested, and I'd love to hear > > if this really works. Could you give it a try? > > > > > > thanks, > > > > Takashi > > > > -- 8< -- > > From: Takashi Iwai > > Subject: [PATCH] ALSA: seq: Fix races at MIDI encoding in > > snd_virmidi_output_trigger() > > > > The sequencer virmidi code has an open race at its output trigger > > callback: namely, virmidi keeps only one event packet for processing > > while it doesn't protect for concurrent output trigger calls. > > > > snd_virmidi_output_trigger() tries to process the previously > > unfinished event before starting encoding the given MIDI stream, but > > this is done without any lock. Meanwhile, if another rawmidi stream > > starts the output trigger, this proceeds further, and overwrites the > > event package that is being processed in another thread. This > > eventually corrupts and may lead to the invalid memory access if the > > event type is like SYSEX. > > > > The fix is just to move the spinlock to cover both the pending event > > and the new stream. > > > > The bug was spotted by a new fuzzer, RaceFuzzer. > > > > BugLink: http://lkml.kernel.org/r/20180426045223.GA15307@dragonet.kaist.ac.kr > > Reported-by: DaeRyong Jeong > > Cc: > > Signed-off-by: Takashi Iwai > > --- > > sound/core/seq/seq_virmidi.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/sound/core/seq/seq_virmidi.c b/sound/core/seq/seq_virmidi.c > > index f48a4cd24ffc..289ae6bb81d9 100644 > > --- a/sound/core/seq/seq_virmidi.c > > +++ b/sound/core/seq/seq_virmidi.c > > @@ -174,12 +174,12 @@ static void snd_virmidi_output_trigger(struct snd_rawmidi_substream *substream, > > } > > return; > > } > > + spin_lock_irqsave(&substream->runtime->lock, flags); > > if (vmidi->event.type != SNDRV_SEQ_EVENT_NONE) { > > if (snd_seq_kernel_client_dispatch(vmidi->client, &vmidi->event, in_atomic(), 0) < 0) > > - return; > > + goto out; > > vmidi->event.type = SNDRV_SEQ_EVENT_NONE; > > } > > - spin_lock_irqsave(&substream->runtime->lock, flags); > > while (1) { > > count = __snd_rawmidi_transmit_peek(substream, buf, sizeof(buf)); > > if (count <= 0) > > -- > > 2.16.3 > > > > I'm really sorry to say this. Since we are implementing our fuzzer and > our reproducer is not complete, we don't have a reproducer for this bug. > We manually analyzed the crash using the crash log to spot where the race > occurs. > The patch looks good for me who don't understand the ALSA subsystem, but > we can't test the patch. > > I'm sorry. No need for sorry, it means positive, just that it's hard to trigger :) In anyway, I queued the fix patch now. Thanks! Takashi