* [PATCH v2] ALSA: seq: resize buffer for overflow
@ 2017-10-06 17:17 Mark Salyzyn
2017-10-07 9:39 ` Takashi Iwai
0 siblings, 1 reply; 3+ messages in thread
From: Mark Salyzyn @ 2017-10-06 17:17 UTC (permalink / raw)
To: linux-kernel
Cc: bunk, bunk, Mark Salyzyn, Jaroslav Kysela, Takashi Iwai, alsa-devel
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(-)
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;
--
2.14.2.920.gcf0c67979c-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] ALSA: seq: resize buffer for overflow
2017-10-06 17:17 [PATCH v2] ALSA: seq: resize buffer for overflow Mark Salyzyn
@ 2017-10-07 9:39 ` Takashi Iwai
2017-10-09 14:51 ` Mark Salyzyn
0 siblings, 1 reply; 3+ messages in thread
From: Takashi Iwai @ 2017-10-07 9:39 UTC (permalink / raw)
To: Mark Salyzyn; +Cc: linux-kernel, alsa-devel, bunk, Jaroslav Kysela, bunk
On Fri, 06 Oct 2017 19:17:27 +0200,
Mark Salyzyn wrote:
>
> 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.
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.
BTW, about your patch is buggy regarding the call kmalloc() with
GFP_KERNEL inside spinlock.
thanks,
Takashi
> 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(-)
>
> 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;
> --
> 2.14.2.920.gcf0c67979c-goog
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] ALSA: seq: resize buffer for overflow
2017-10-07 9:39 ` Takashi Iwai
@ 2017-10-09 14:51 ` Mark Salyzyn
0 siblings, 0 replies; 3+ messages in thread
From: Mark Salyzyn @ 2017-10-09 14:51 UTC (permalink / raw)
To: Takashi Iwai; +Cc: linux-kernel, alsa-devel, bunk, Jaroslav Kysela, bunk
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-10-09 14:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-06 17:17 [PATCH v2] ALSA: seq: resize buffer for overflow Mark Salyzyn
2017-10-07 9:39 ` Takashi Iwai
2017-10-09 14:51 ` Mark Salyzyn
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).