LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Takashi Iwai <tiwai@suse.de>
To: DaeRyong Jeong <threeearcat@gmail.com>
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
Date: Fri, 27 Apr 2018 17:52:43 +0200	[thread overview]
Message-ID: <s5hd0ykfxys.wl-tiwai@suse.de> (raw)
In-Reply-To: <20180427014858.GA23026@dragonet.kaist.ac.kr>

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 <tiwai@suse.de>
> > 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 <threeearcat@gmail.com>
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  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

      reply	other threads:[~2018-04-27 15:52 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-26  4:52 DaeRyong Jeong
2018-04-26  7:17 ` Takashi Iwai
2018-04-27  1:48   ` DaeRyong Jeong
2018-04-27 15:52     ` Takashi Iwai [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=s5hd0ykfxys.wl-tiwai@suse.de \
    --to=tiwai@suse.de \
    --cc=alsa-devel@alsa-project.org \
    --cc=bammanag@purdue.edu \
    --cc=byoungyoung@purdue.edu \
    --cc=kt0755@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=perex@perex.cz \
    --cc=threeearcat@gmail.com \
    --subject='Re: Unable to handle kernel paging request in snd_seq_oss_readq_puts' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).