Commit 530372de authored by Takashi Iwai's avatar Takashi Iwai Committed by Luis Henriques

ALSA: rawmidi: Make snd_rawmidi_transmit() race-free

commit 06ab3003 upstream.

A kernel WARNING in snd_rawmidi_transmit_ack() is triggered by
syzkaller fuzzer:
  WARNING: CPU: 1 PID: 20739 at sound/core/rawmidi.c:1136
Call Trace:
 [<     inline     >] __dump_stack lib/dump_stack.c:15
 [<ffffffff82999e2d>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
 [<ffffffff81352089>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:482
 [<ffffffff813522b9>] warn_slowpath_null+0x29/0x30 kernel/panic.c:515
 [<ffffffff84f80bd5>] snd_rawmidi_transmit_ack+0x275/0x400 sound/core/rawmidi.c:1136
 [<ffffffff84fdb3c1>] snd_virmidi_output_trigger+0x4b1/0x5a0 sound/core/seq/seq_virmidi.c:163
 [<     inline     >] snd_rawmidi_output_trigger sound/core/rawmidi.c:150
 [<ffffffff84f87ed9>] snd_rawmidi_kernel_write1+0x549/0x780 sound/core/rawmidi.c:1223
 [<ffffffff84f89fd3>] snd_rawmidi_write+0x543/0xb30 sound/core/rawmidi.c:1273
 [<ffffffff817b0323>] __vfs_write+0x113/0x480 fs/read_write.c:528
 [<ffffffff817b1db7>] vfs_write+0x167/0x4a0 fs/read_write.c:577
 [<     inline     >] SYSC_write fs/read_write.c:624
 [<ffffffff817b50a1>] SyS_write+0x111/0x220 fs/read_write.c:616
 [<ffffffff86336c36>] entry_SYSCALL_64_fastpath+0x16/0x7a arch/x86/entry/entry_64.S:185

Also a similar warning is found but in another path:
Call Trace:
 [<     inline     >] __dump_stack lib/dump_stack.c:15
 [<ffffffff82be2c0d>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
 [<ffffffff81355139>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:482
 [<ffffffff81355369>] warn_slowpath_null+0x29/0x30 kernel/panic.c:515
 [<ffffffff8527e69a>] rawmidi_transmit_ack+0x24a/0x3b0 sound/core/rawmidi.c:1133
 [<ffffffff8527e851>] snd_rawmidi_transmit_ack+0x51/0x80 sound/core/rawmidi.c:1163
 [<ffffffff852d9046>] snd_virmidi_output_trigger+0x2b6/0x570 sound/core/seq/seq_virmidi.c:185
 [<     inline     >] snd_rawmidi_output_trigger sound/core/rawmidi.c:150
 [<ffffffff85285a0b>] snd_rawmidi_kernel_write1+0x4bb/0x760 sound/core/rawmidi.c:1252
 [<ffffffff85287b73>] snd_rawmidi_write+0x543/0xb30 sound/core/rawmidi.c:1302
 [<ffffffff817ba5f3>] __vfs_write+0x113/0x480 fs/read_write.c:528
 [<ffffffff817bc087>] vfs_write+0x167/0x4a0 fs/read_write.c:577
 [<     inline     >] SYSC_write fs/read_write.c:624
 [<ffffffff817bf371>] SyS_write+0x111/0x220 fs/read_write.c:616
 [<ffffffff86660276>] entry_SYSCALL_64_fastpath+0x16/0x7a arch/x86/entry/entry_64.S:185

In the former case, the reason is that virmidi has an open code
calling snd_rawmidi_transmit_ack() with the value calculated outside
the spinlock.   We may use snd_rawmidi_transmit() in a loop just for
consuming the input data, but even there, there is a race between
snd_rawmidi_transmit_peek() and snd_rawmidi_tranmit_ack().

Similarly in the latter case, it calls snd_rawmidi_transmit_peek() and
snd_rawmidi_tranmit_ack() separately without protection, so they are
racy as well.

The patch tries to address these issues by the following ways:
- Introduce the unlocked versions of snd_rawmidi_transmit_peek() and
  snd_rawmidi_transmit_ack() to be called inside the explicit lock.
- Rewrite snd_rawmidi_transmit() to be race-free (the former case).
- Make the split calls (the latter case) protected in the rawmidi spin
  lock.

BugLink: http://lkml.kernel.org/r/CACT4Y+YPq1+cYLkadwjWa5XjzF1_Vki1eHnVn-Lm0hzhSpu5PA@mail.gmail.com
BugLink: http://lkml.kernel.org/r/CACT4Y+acG4iyphdOZx47Nyq_VHGbpJQK-6xNpiqUjaZYqsXOGw@mail.gmail.comReported-by: default avatarDmitry Vyukov <dvyukov@google.com>
Tested-by: default avatarDmitry Vyukov <dvyukov@google.com>
Signed-off-by: default avatarTakashi Iwai <tiwai@suse.de>
Signed-off-by: default avatarLuis Henriques <luis.henriques@canonical.com>
parent 0d0254d7
...@@ -165,6 +165,10 @@ int snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream, ...@@ -165,6 +165,10 @@ int snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream,
int snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, int count); int snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, int count);
int snd_rawmidi_transmit(struct snd_rawmidi_substream *substream, int snd_rawmidi_transmit(struct snd_rawmidi_substream *substream,
unsigned char *buffer, int count); unsigned char *buffer, int count);
int __snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream,
unsigned char *buffer, int count);
int __snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream,
int count);
/* main midi functions */ /* main midi functions */
......
...@@ -1065,23 +1065,16 @@ int snd_rawmidi_transmit_empty(struct snd_rawmidi_substream *substream) ...@@ -1065,23 +1065,16 @@ int snd_rawmidi_transmit_empty(struct snd_rawmidi_substream *substream)
EXPORT_SYMBOL(snd_rawmidi_transmit_empty); EXPORT_SYMBOL(snd_rawmidi_transmit_empty);
/** /**
* snd_rawmidi_transmit_peek - copy data from the internal buffer * __snd_rawmidi_transmit_peek - copy data from the internal buffer
* @substream: the rawmidi substream * @substream: the rawmidi substream
* @buffer: the buffer pointer * @buffer: the buffer pointer
* @count: data size to transfer * @count: data size to transfer
* *
* Copies data from the internal output buffer to the given buffer. * This is a variant of snd_rawmidi_transmit_peek() without spinlock.
*
* Call this in the interrupt handler when the midi output is ready,
* and call snd_rawmidi_transmit_ack() after the transmission is
* finished.
*
* Return: The size of copied data, or a negative error code on failure.
*/ */
int snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream, int __snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream,
unsigned char *buffer, int count) unsigned char *buffer, int count)
{ {
unsigned long flags;
int result, count1; int result, count1;
struct snd_rawmidi_runtime *runtime = substream->runtime; struct snd_rawmidi_runtime *runtime = substream->runtime;
...@@ -1091,7 +1084,6 @@ int snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream, ...@@ -1091,7 +1084,6 @@ int snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream,
return -EINVAL; return -EINVAL;
} }
result = 0; result = 0;
spin_lock_irqsave(&runtime->lock, flags);
if (runtime->avail >= runtime->buffer_size) { if (runtime->avail >= runtime->buffer_size) {
/* warning: lowlevel layer MUST trigger down the hardware */ /* warning: lowlevel layer MUST trigger down the hardware */
goto __skip; goto __skip;
...@@ -1116,25 +1108,47 @@ int snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream, ...@@ -1116,25 +1108,47 @@ int snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream,
} }
} }
__skip: __skip:
return result;
}
EXPORT_SYMBOL(__snd_rawmidi_transmit_peek);
/**
* snd_rawmidi_transmit_peek - copy data from the internal buffer
* @substream: the rawmidi substream
* @buffer: the buffer pointer
* @count: data size to transfer
*
* Copies data from the internal output buffer to the given buffer.
*
* Call this in the interrupt handler when the midi output is ready,
* and call snd_rawmidi_transmit_ack() after the transmission is
* finished.
*
* Return: The size of copied data, or a negative error code on failure.
*/
int snd_rawmidi_transmit_peek(struct snd_rawmidi_substream *substream,
unsigned char *buffer, int count)
{
struct snd_rawmidi_runtime *runtime = substream->runtime;
int result;
unsigned long flags;
spin_lock_irqsave(&runtime->lock, flags);
result = __snd_rawmidi_transmit_peek(substream, buffer, count);
spin_unlock_irqrestore(&runtime->lock, flags); spin_unlock_irqrestore(&runtime->lock, flags);
return result; return result;
} }
EXPORT_SYMBOL(snd_rawmidi_transmit_peek); EXPORT_SYMBOL(snd_rawmidi_transmit_peek);
/** /**
* snd_rawmidi_transmit_ack - acknowledge the transmission * __snd_rawmidi_transmit_ack - acknowledge the transmission
* @substream: the rawmidi substream * @substream: the rawmidi substream
* @count: the transferred count * @count: the transferred count
* *
* Advances the hardware pointer for the internal output buffer with * This is a variant of __snd_rawmidi_transmit_ack() without spinlock.
* the given size and updates the condition.
* Call after the transmission is finished.
*
* Return: The advanced size if successful, or a negative error code on failure.
*/ */
int snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, int count) int __snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, int count)
{ {
unsigned long flags;
struct snd_rawmidi_runtime *runtime = substream->runtime; struct snd_rawmidi_runtime *runtime = substream->runtime;
if (runtime->buffer == NULL) { if (runtime->buffer == NULL) {
...@@ -1142,7 +1156,6 @@ int snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, int count) ...@@ -1142,7 +1156,6 @@ int snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, int count)
"snd_rawmidi_transmit_ack: output is not active!!!\n"); "snd_rawmidi_transmit_ack: output is not active!!!\n");
return -EINVAL; return -EINVAL;
} }
spin_lock_irqsave(&runtime->lock, flags);
snd_BUG_ON(runtime->avail + count > runtime->buffer_size); snd_BUG_ON(runtime->avail + count > runtime->buffer_size);
runtime->hw_ptr += count; runtime->hw_ptr += count;
runtime->hw_ptr %= runtime->buffer_size; runtime->hw_ptr %= runtime->buffer_size;
...@@ -1152,9 +1165,32 @@ int snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, int count) ...@@ -1152,9 +1165,32 @@ int snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, int count)
if (runtime->drain || snd_rawmidi_ready(substream)) if (runtime->drain || snd_rawmidi_ready(substream))
wake_up(&runtime->sleep); wake_up(&runtime->sleep);
} }
spin_unlock_irqrestore(&runtime->lock, flags);
return count; return count;
} }
EXPORT_SYMBOL(__snd_rawmidi_transmit_ack);
/**
* snd_rawmidi_transmit_ack - acknowledge the transmission
* @substream: the rawmidi substream
* @count: the transferred count
*
* Advances the hardware pointer for the internal output buffer with
* the given size and updates the condition.
* Call after the transmission is finished.
*
* Return: The advanced size if successful, or a negative error code on failure.
*/
int snd_rawmidi_transmit_ack(struct snd_rawmidi_substream *substream, int count)
{
struct snd_rawmidi_runtime *runtime = substream->runtime;
int result;
unsigned long flags;
spin_lock_irqsave(&runtime->lock, flags);
result = __snd_rawmidi_transmit_ack(substream, count);
spin_unlock_irqrestore(&runtime->lock, flags);
return result;
}
EXPORT_SYMBOL(snd_rawmidi_transmit_ack); EXPORT_SYMBOL(snd_rawmidi_transmit_ack);
/** /**
...@@ -1170,12 +1206,22 @@ EXPORT_SYMBOL(snd_rawmidi_transmit_ack); ...@@ -1170,12 +1206,22 @@ EXPORT_SYMBOL(snd_rawmidi_transmit_ack);
int snd_rawmidi_transmit(struct snd_rawmidi_substream *substream, int snd_rawmidi_transmit(struct snd_rawmidi_substream *substream,
unsigned char *buffer, int count) unsigned char *buffer, int count)
{ {
struct snd_rawmidi_runtime *runtime = substream->runtime;
int result;
unsigned long flags;
spin_lock_irqsave(&runtime->lock, flags);
if (!substream->opened) if (!substream->opened)
return -EBADFD; result = -EBADFD;
count = snd_rawmidi_transmit_peek(substream, buffer, count); else {
if (count < 0) count = __snd_rawmidi_transmit_peek(substream, buffer, count);
return count; if (count <= 0)
return snd_rawmidi_transmit_ack(substream, count); result = count;
else
result = __snd_rawmidi_transmit_ack(substream, count);
}
spin_unlock_irqrestore(&runtime->lock, flags);
return result;
} }
EXPORT_SYMBOL(snd_rawmidi_transmit); EXPORT_SYMBOL(snd_rawmidi_transmit);
......
...@@ -155,21 +155,26 @@ static void snd_virmidi_output_trigger(struct snd_rawmidi_substream *substream, ...@@ -155,21 +155,26 @@ static void snd_virmidi_output_trigger(struct snd_rawmidi_substream *substream,
struct snd_virmidi *vmidi = substream->runtime->private_data; struct snd_virmidi *vmidi = substream->runtime->private_data;
int count, res; int count, res;
unsigned char buf[32], *pbuf; unsigned char buf[32], *pbuf;
unsigned long flags;
if (up) { if (up) {
vmidi->trigger = 1; vmidi->trigger = 1;
if (vmidi->seq_mode == SNDRV_VIRMIDI_SEQ_DISPATCH && if (vmidi->seq_mode == SNDRV_VIRMIDI_SEQ_DISPATCH &&
!(vmidi->rdev->flags & SNDRV_VIRMIDI_SUBSCRIBE)) { !(vmidi->rdev->flags & SNDRV_VIRMIDI_SUBSCRIBE)) {
snd_rawmidi_transmit_ack(substream, substream->runtime->buffer_size - substream->runtime->avail); while (snd_rawmidi_transmit(substream, buf,
return; /* ignored */ sizeof(buf)) > 0) {
/* ignored */
}
return;
} }
if (vmidi->event.type != SNDRV_SEQ_EVENT_NONE) { if (vmidi->event.type != SNDRV_SEQ_EVENT_NONE) {
if (snd_seq_kernel_client_dispatch(vmidi->client, &vmidi->event, in_atomic(), 0) < 0) if (snd_seq_kernel_client_dispatch(vmidi->client, &vmidi->event, in_atomic(), 0) < 0)
return; return;
vmidi->event.type = SNDRV_SEQ_EVENT_NONE; vmidi->event.type = SNDRV_SEQ_EVENT_NONE;
} }
spin_lock_irqsave(&substream->runtime->lock, flags);
while (1) { while (1) {
count = snd_rawmidi_transmit_peek(substream, buf, sizeof(buf)); count = __snd_rawmidi_transmit_peek(substream, buf, sizeof(buf));
if (count <= 0) if (count <= 0)
break; break;
pbuf = buf; pbuf = buf;
...@@ -179,16 +184,18 @@ static void snd_virmidi_output_trigger(struct snd_rawmidi_substream *substream, ...@@ -179,16 +184,18 @@ static void snd_virmidi_output_trigger(struct snd_rawmidi_substream *substream,
snd_midi_event_reset_encode(vmidi->parser); snd_midi_event_reset_encode(vmidi->parser);
continue; continue;
} }
snd_rawmidi_transmit_ack(substream, res); __snd_rawmidi_transmit_ack(substream, res);
pbuf += res; pbuf += res;
count -= res; count -= res;
if (vmidi->event.type != SNDRV_SEQ_EVENT_NONE) { if (vmidi->event.type != SNDRV_SEQ_EVENT_NONE) {
if (snd_seq_kernel_client_dispatch(vmidi->client, &vmidi->event, in_atomic(), 0) < 0) if (snd_seq_kernel_client_dispatch(vmidi->client, &vmidi->event, in_atomic(), 0) < 0)
return; goto out;
vmidi->event.type = SNDRV_SEQ_EVENT_NONE; vmidi->event.type = SNDRV_SEQ_EVENT_NONE;
} }
} }
} }
out:
spin_unlock_irqrestore(&substream->runtime->lock, flags);
} else { } else {
vmidi->trigger = 0; vmidi->trigger = 0;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment