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: Thu, 26 Apr 2018 09:17:45 +0200	[thread overview]
Message-ID: <s5h1sf2v3l2.wl-tiwai@suse.de> (raw)
In-Reply-To: <20180426045223.GA15307@dragonet.kaist.ac.kr>

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

  reply	other threads:[~2018-04-26  7:17 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 [this message]
2018-04-27  1:48   ` DaeRyong Jeong
2018-04-27 15:52     ` Takashi Iwai

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=s5h1sf2v3l2.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).