[v2] ALSA: seq: resize buffer for overflow
diff mbox

Message ID 20171006171731.88889-1-salyzyn@android.com
State New
Headers show

Commit Message

Mark Salyzyn Oct. 6, 2017, 5:17 p.m. UTC
Can not replicate, issue discovered in fuzzing. Stack trace below.
No functional or performance testing done regarding the fix.

Trap at (reformatted):

snd_seq_oss_readq_puts(struct seq_oss_readq *q, int dev,
                       unsigned char *data, int len)
{
    union evrec rec;
    int result;

    memset(&rec, 0, sizeof(rec));
    rec.c[0] = SEQ_MIDIPUTC;
    rec.c[2] = dev;

    while (len-- > 0) {
        rec.c[1] = *data++; // data is RBX  HERE

'data' pointer just passed a page boundary, so the buffer supplied
was short.  Caller must have been handed an ev->type equal to
SNDDRV_SEQ_EVENT_SYSEX, which resulted in handing off
ev->data.ext.ptr[ev->data.ext.len] buffer.  Intuited that the source
of the event and buffer was referenced in
snd_midi_event_encode_byte() passing a larger length than the
allocated buffer.

BUG: unable to handle kernel paging request at ffffc900008ab000
IP: [<ffffffff82e2fc05>] snd_seq_oss_readq_puts+0xd5/0x170 sound/core/seq/oss/seq_oss_readq.c:112
PGD 1da091067 PUD 1da092067
Oops: 0000 [#1] PREEMPT SMP KASAN
Dumping ftrace buffer:
   (ftrace buffer empty)
Modules linked in:
CPU: 1 PID: 3264 Comm: XXXXXXXX
Hardware name: XXXXXXXXXX
task: ffff8801cdd9e000 task.stack: ffff8801ce648000
RIP: 0010:[<ffffffff82e2fc05>]  [<ffffffff82e2fc05>] snd_seq_oss_readq_puts+0xd5/0x170 sound/core/seq/oss/seq_oss_readq.c:112
RSP: 0018:ffff8801ce64f1c0  EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffffc900008ab000 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffffffff858e5780
RBP: ffff8801ce64f260 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 1ffff10039cc9df2 R12: 000000003fffffa4
R13: dffffc0000000000 R14: ffff8801ce64f238 R15: ffffc900008ab001
FS:  00007fe3d3d9e700(0000) GS:ffff8801db300000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffc900008ab000 CR3: 00000001d19b7000 CR4: 00000000001406e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Stack:
 1ffff10039cc9e3b ffff8801ce64f1f8 ffff8801d0b9aa00 0000000041b58ab3
 ffffffff841daf3c ffffffff82e2fb30 0000000000000286 0000000000000005
 ffffffff838aa5d5 ffffffff861962c0 dffffc0000000000 ffff8801ce64f260
Call Trace:
 [<ffffffff82e2ebbe>] send_midi_event sound/core/seq/oss/seq_oss_midi.c:616 [inline]
 [<ffffffff82e2ebbe>] snd_seq_oss_midi_input+0x8ce/0xa70 sound/core/seq/oss/seq_oss_midi.c:535
 [<ffffffff82e2758d>] snd_seq_oss_event_input+0x15d/0x220 sound/core/seq/oss/seq_oss_event.c:439
 [<ffffffff82e0b650>] snd_seq_deliver_single_event.constprop.11+0x310/0x7c0 sound/core/seq/seq_clientmgr.c:621
 [<ffffffff82e0be16>] deliver_to_subscribers sound/core/seq/seq_clientmgr.c:676 [inline]
 [<ffffffff82e0be16>] snd_seq_deliver_event+0x316/0x740 sound/core/seq/seq_clientmgr.c:807
 [<ffffffff82e0cf9e>] snd_seq_kernel_client_dispatch+0x11e/0x150 sound/core/seq/seq_clientmgr.c:2314
 [<ffffffff82e31775>] dummy_input+0x235/0x320 sound/core/seq/seq_dummy.c:104
 [<ffffffff82e0b650>] snd_seq_deliver_single_event.constprop.11+0x310/0x7c0 sound/core/seq/seq_clientmgr.c:621
 [<ffffffff82e0bc2d>] snd_seq_deliver_event+0x12d/0x740 sound/core/seq/seq_clientmgr.c:818
 [<ffffffff82e0d0ed>] snd_seq_dispatch_event+0x11d/0x520 sound/core/seq/seq_clientmgr.c:892
 [<ffffffff82e1117e>] snd_seq_check_queue.part.3+0x38e/0x510 sound/core/seq/seq_queue.c:285
 [<ffffffff82e11d8d>] snd_seq_check_queue sound/core/seq/seq_queue.c:357 [inline]
 [<ffffffff82e11d8d>] snd_seq_enqueue_event+0x32d/0x3d0 sound/core/seq/seq_queue.c:363
 [<ffffffff82e0c444>] snd_seq_client_enqueue_event+0x204/0x3e0 sound/core/seq/seq_clientmgr.c:951
 [<ffffffff82e0cc55>] kernel_client_enqueue.part.10+0xb5/0xd0 sound/core/seq/seq_clientmgr.c:2251
 [<ffffffff82e0cd3f>] kernel_client_enqueue sound/core/seq/seq_clientmgr.c:2241 [inline]
 [<ffffffff82e0cd3f>] snd_seq_kernel_client_enqueue_blocking+0xcf/0x110 sound/core/seq/seq_clientmgr.c:2279
 [<ffffffff82e27f68>] insert_queue sound/core/seq/oss/seq_oss_rw.c:189 [inline]
 [<ffffffff82e27f68>] snd_seq_oss_write+0x538/0x850 sound/core/seq/oss/seq_oss_rw.c:148
 [<ffffffff82e1fab4>] odev_write+0x64/0x90 sound/core/seq/oss/seq_oss.c:177
 [<ffffffff8156ab43>] __vfs_write+0x103/0x680 fs/read_write.c:510
 [<ffffffff8156ec70>] vfs_write+0x170/0x4e0 fs/read_write.c:560
 [<ffffffff81572669>] SYSC_write fs/read_write.c:607 [inline]
 [<ffffffff81572669>] SyS_write+0xd9/0x1b0 fs/read_write.c:599
 [<ffffffff838aad85>] entry_SYSCALL_64_fastpath+0x23/0xc6
Code: 4d 9a eb 4e e8 2d aa 53 fe 4c 8d 7b 01 48 89 d8 48 89 d9 48 c1 e8 03 83 e1 07 42 0f b6 04 28 38 c8 7f 08 84 c0 0f 85 80 00 00 00 <41> 0f b6 47 ff 41 83 ec 01 48 8b b5 68 ff ff ff 48 8b bd 70 ff
RIP  [<ffffffff82e2fc05>] snd_seq_oss_readq_puts+0xd5/0x170 sound/core/seq/oss/seq_oss_readq.c:112
 RSP <ffff8801ce64f1c0>
CR2: ffffc900008ab000

Signed-off-by: Mark Salyzyn <salyzyn@android.com>

v2: removed nested locks in snd_midi_event_resize_buffer
---
 sound/core/seq/seq_midi_event.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Mark Salyzyn Oct. 9, 2017, 2:51 p.m. UTC | #1
On 10/07/2017 02:39 AM, Takashi Iwai wrote:
> I doubt it came from snd_midi_event_encode_byte().
> Judging from the call trace below, the event originated from the OSS
> sequencer write, i.e. it received an OSS event packet, and it was
> delivered again to another OSS sequencer port back via dummy client.
>
> If so, it should have received some EV_SYSEX packet, and it was
> processed via snd_seq_oss_synth_sysex(), and the encoded event was
> delivered.
>
> Now the question is how it triggers this Oops.  I couldn't find any
> obvious cause, but one thing I noticed is a possible race when writing
> to OSS sequencer concurrently.  Something wrong might happen.

Concurrent writing, thanks, I will switch gears and see if that 
represents the replication path!
> BTW, about your patch is buggy regarding the call kmalloc() with
> GFP_KERNEL inside spinlock.

<urrrrk> yup, withdraw this patch, and please erase it from my permanent 
record ;->

Thanks for the review, it was immensely helpful!

-- Mark

Patch
diff mbox

diff --git a/sound/core/seq/seq_midi_event.c b/sound/core/seq/seq_midi_event.c
index 90bbbdbeba03..ed3206ef0cd4 100644
--- a/sound/core/seq/seq_midi_event.c
+++ b/sound/core/seq/seq_midi_event.c
@@ -192,8 +192,7 @@  EXPORT_SYMBOL(snd_midi_event_no_status);
 /*
  * resize buffer
  */
-#if 0
-int snd_midi_event_resize_buffer(struct snd_midi_event *dev, int bufsize)
+static int snd_midi_event_resize_buffer(struct snd_midi_event *dev, int bufsize)
 {
 	unsigned char *new_buf, *old_buf;
 	unsigned long flags;
@@ -203,16 +202,13 @@  int snd_midi_event_resize_buffer(struct snd_midi_event *dev, int bufsize)
 	new_buf = kmalloc(bufsize, GFP_KERNEL);
 	if (new_buf == NULL)
 		return -ENOMEM;
-	spin_lock_irqsave(&dev->lock, flags);
 	old_buf = dev->buf;
 	dev->buf = new_buf;
 	dev->bufsize = bufsize;
 	reset_encode(dev);
-	spin_unlock_irqrestore(&dev->lock, flags);
 	kfree(old_buf);
 	return 0;
 }
-#endif  /*  0  */
 
 /*
  *  read bytes and encode to sequencer event if finished
@@ -297,6 +293,8 @@  int snd_midi_event_encode_byte(struct snd_midi_event *dev, int c,
 	} else 	if (dev->type == ST_SYSEX) {
 		if (c == MIDI_CMD_COMMON_SYSEX_END ||
 		    dev->read >= dev->bufsize) {
+			if (dev->read > dev->bufsize)
+				snd_midi_event_resize_buffer(dev, dev->read);
 			ev->flags &= ~SNDRV_SEQ_EVENT_LENGTH_MASK;
 			ev->flags |= SNDRV_SEQ_EVENT_LENGTH_VARIABLE;
 			ev->type = SNDRV_SEQ_EVENT_SYSEX;