Re: [alsa-devel] BUG: unable to handle kernel paging request in snd_seq_oss_readq_puts
On Wed, Nov 1, 2017 at 9:38 PM, syzbot bot+31681772ec7a18dde8d3f8caf475f361a89b9514@syzkaller.appspotmail.com wrote:
This also happened on more recent commits, including upstream 9c323bff13f92832e03657cabdd70d731408d621 (Oct 20):
device gre0 entered promiscuous mode BUG: unable to handle kernel paging request at ffffc90001688000 IP: snd_seq_oss_readq_puts+0x146/0x210 sound/core/seq/oss/seq_oss_readq.c:112 PGD 1dac75067 P4D 1dac75067 PUD 1dac76067 PMD 1c1dd3067 PTE 0 Oops: 0000 [#1] SMP KASAN Dumping ftrace buffer: (ftrace buffer empty) Modules linked in: CPU: 1 PID: 18947 Comm: syz-executor2 Not tainted 4.14.0-rc5+ #51 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 task: ffff8801d92c8140 task.stack: ffff8801c2ef0000 RIP: 0010:snd_seq_oss_readq_puts+0x146/0x210 sound/core/seq/oss/seq_oss_readq.c:112 RSP: 0018:ffff8801c2ef6db0 EFLAGS: 00010246 RAX: ffffed00385dedbf RBX: ffffc90001688000 RCX: ffff8801c2ef6df9 RDX: 000000000000554e RSI: ffffffff83d4c556 RDI: 0000000000000282 RBP: ffff8801c2ef6e60 R08: 0000000000000001 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000000 R12: 00000000400000a0 R13: ffff8801c2ef6e38 R14: ffffc90001688001 R15: dffffc0000000000 FS: 0000000000000000(0000) GS:ffff8801db300000(0063) knlGS:00000000f76ffb40 CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033 CR2: ffffc90001688000 CR3: 00000001d0650000 CR4: 00000000001406e0 DR0: 0000000020000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000600 Call Trace: send_midi_event sound/core/seq/oss/seq_oss_midi.c:616 [inline] snd_seq_oss_midi_input+0xd39/0x1040 sound/core/seq/oss/seq_oss_midi.c:535 snd_seq_oss_event_input+0x15d/0x220 sound/core/seq/oss/seq_oss_event.c:439 snd_seq_deliver_single_event.constprop.11+0x2fb/0x940 sound/core/seq/seq_clientmgr.c:621 deliver_to_subscribers sound/core/seq/seq_clientmgr.c:676 [inline] snd_seq_deliver_event+0x318/0x790 sound/core/seq/seq_clientmgr.c:807 snd_seq_kernel_client_dispatch+0x11e/0x150 sound/core/seq/seq_clientmgr.c:2313 dummy_input+0x2c4/0x400 sound/core/seq/seq_dummy.c:104 snd_seq_deliver_single_event.constprop.11+0x2fb/0x940 sound/core/seq/seq_clientmgr.c:621 snd_seq_deliver_event+0x12c/0x790 sound/core/seq/seq_clientmgr.c:818 snd_seq_dispatch_event+0x105/0x5b0 sound/core/seq/seq_clientmgr.c:892 snd_seq_check_queue.part.3+0x38e/0x510 sound/core/seq/seq_queue.c:285 snd_seq_check_queue sound/core/seq/seq_queue.c:357 [inline] snd_seq_enqueue_event+0x32d/0x3d0 sound/core/seq/seq_queue.c:363 snd_seq_client_enqueue_event+0x21b/0x420 sound/core/seq/seq_clientmgr.c:951 kernel_client_enqueue.part.10+0xb5/0xd0 sound/core/seq/seq_clientmgr.c:2252 kernel_client_enqueue sound/core/seq/seq_clientmgr.c:2242 [inline] snd_seq_kernel_client_enqueue_blocking+0xcf/0x110 sound/core/seq/seq_clientmgr.c:2279 insert_queue sound/core/seq/oss/seq_oss_rw.c:189 [inline] snd_seq_oss_write+0x5fe/0xa80 sound/core/seq/oss/seq_oss_rw.c:148 odev_write+0x64/0x90 sound/core/seq/oss/seq_oss.c:177 __vfs_write+0xef/0x970 fs/read_write.c:479 __kernel_write+0xf8/0x350 fs/read_write.c:500 write_pipe_buf+0x175/0x220 fs/splice.c:797 splice_from_pipe_feed fs/splice.c:502 [inline] __splice_from_pipe+0x328/0x730 fs/splice.c:626 splice_from_pipe+0x1e9/0x330 fs/splice.c:661 default_file_splice_write+0x40/0x90 fs/splice.c:809 do_splice_from fs/splice.c:851 [inline] do_splice fs/splice.c:1147 [inline] SYSC_splice fs/splice.c:1402 [inline] SyS_splice+0x7b7/0x1610 fs/splice.c:1382 do_syscall_32_irqs_on arch/x86/entry/common.c:329 [inline] do_fast_syscall_32+0x3f2/0xf05 arch/x86/entry/common.c:391 entry_SYSENTER_compat+0x51/0x60 arch/x86/entry/entry_64_compat.S:124 RIP: 0023:0xf7f03c79 RSP: 002b:00000000f76ff01c EFLAGS: 00000296 ORIG_RAX: 0000000000000139 RAX: ffffffffffffffda RBX: 0000000000000014 RCX: 0000000000000000 RDX: 0000000000000013 RSI: 0000000000000000 RDI: 000000005813dd57 RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 Code: d9 4c 8d 73 01 48 c1 e8 03 83 e1 07 42 0f b6 04 38 38 c8 7f 08 84 c0 0f 85 bf 00 00 00 48 8b 85 58 ff ff ff 48 8b 8d 68 ff ff ff <41> 0f b6 5e ff 0f b6 00 83 e1 07 38 c8 7f 08 84 c0 0f 85 89 00 RIP: snd_seq_oss_readq_puts+0x146/0x210 sound/core/seq/oss/seq_oss_readq.c:112 RSP: ffff8801c2ef6db0 CR2: ffffc90001688000 ---[ end trace c7c98a599517e76d ]---
On Wed, 01 Nov 2017 19:39:46 +0100, Dmitry Vyukov wrote:
Could you try the patch below with CONFIG_SND_DEBUG=y and see whether it catches any bad calls? It's already in for-next branch for 4.15.
thanks,
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: seq: Add sanity check for user-space pointer delivery
The sequencer event may contain a user-space pointer with its SNDRV_SEQ_EXT_USRPTR bit, and we assure that its delivery is limited with non-atomic mode. Otherwise the copy_from_user() may hit the fault and cause a problem. Although the core code doesn't set such a flag (only set at snd_seq_write()), any wild driver may set it mistakenly and lead to an unexpected crash.
This patch adds a sanity check of such events at the delivery core code to filter out the invalid invocation in the atomic mode.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/seq/seq_clientmgr.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index ea2d0ae85bd3..f2343f63ba26 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -802,6 +802,10 @@ static int snd_seq_deliver_event(struct snd_seq_client *client, struct snd_seq_e return -EMLINK; }
+ if (snd_seq_ev_is_variable(event) && + snd_BUG_ON(atomic && (event->data.ext.len & SNDRV_SEQ_EXT_USRPTR))) + return -EINVAL; + if (event->queue == SNDRV_SEQ_ADDRESS_SUBSCRIBERS || event->dest.client == SNDRV_SEQ_ADDRESS_SUBSCRIBERS) result = deliver_to_subscribers(client, event, atomic, hop);
On Wed, 01 Nov 2017 20:49:10 +0100, Takashi Iwai wrote:
And now I remember that it's the exactly same bug Mark Salyzyn reported sometime ago, msgid 20171006171731.88889-1-salyzyn@android.com
So this is likely hitting an invalid page, if Mark's analysis is correct.
thanks,
Takashi
On Thu, 02 Nov 2017 09:58:04 +0100, Takashi Iwai wrote:
I figured out that it's the badly handled SYSEX event delivery in OSS sequencer code. The fix patch is below.
#syz fix: ALSA: seq: Fix OSS sysex delivery in OSS emulation
thanks,
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: seq: Fix OSS sysex delivery in OSS emulation
The SYSEX event delivery in OSS sequencer emulation assumed that the event is encoded in the variable-length data with the straight buffering. This was the normal behavior in the past, but during the development, the chained buffers were introduced for carrying more data, while the OSS code was left intact. As a result, when a SYSEX event with the chained buffer data is passed to OSS sequencer port, it may end up with the wrong memory access, as if it were having a too large buffer.
This patch addresses the bug, by applying the buffer data expansion by the generic snd_seq_dump_var_event() helper function.
Reported-by: syzbot syzkaller@googlegroups.com Reported-by: Mark Salyzyn salyzyn@android.com Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/seq/oss/seq_oss_midi.c | 4 +--- sound/core/seq/oss/seq_oss_readq.c | 29 +++++++++++++++++++++++++++++ sound/core/seq/oss/seq_oss_readq.h | 2 ++ 3 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/sound/core/seq/oss/seq_oss_midi.c b/sound/core/seq/oss/seq_oss_midi.c index aaff9ee32695..b30b2139e3f0 100644 --- a/sound/core/seq/oss/seq_oss_midi.c +++ b/sound/core/seq/oss/seq_oss_midi.c @@ -612,9 +612,7 @@ send_midi_event(struct seq_oss_devinfo *dp, struct snd_seq_event *ev, struct seq if (!dp->timer->running) len = snd_seq_oss_timer_start(dp->timer); if (ev->type == SNDRV_SEQ_EVENT_SYSEX) { - if ((ev->flags & SNDRV_SEQ_EVENT_LENGTH_MASK) == SNDRV_SEQ_EVENT_LENGTH_VARIABLE) - snd_seq_oss_readq_puts(dp->readq, mdev->seq_device, - ev->data.ext.ptr, ev->data.ext.len); + snd_seq_oss_readq_sysex(dp->readq, mdev->seq_device, ev); } else { len = snd_midi_event_decode(mdev->coder, msg, sizeof(msg), ev); if (len > 0) diff --git a/sound/core/seq/oss/seq_oss_readq.c b/sound/core/seq/oss/seq_oss_readq.c index 046cb586fb2f..06b21226b4e7 100644 --- a/sound/core/seq/oss/seq_oss_readq.c +++ b/sound/core/seq/oss/seq_oss_readq.c @@ -117,6 +117,35 @@ snd_seq_oss_readq_puts(struct seq_oss_readq *q, int dev, unsigned char *data, in return 0; }
+/* + * put MIDI sysex bytes; the event buffer may be chained, thus it has + * to be expanded via snd_seq_dump_var_event(). + */ +struct readq_sysex_ctx { + struct seq_oss_readq *readq; + int dev; +}; + +static int readq_dump_sysex(void *ptr, void *buf, int count) +{ + struct readq_sysex_ctx *ctx = ptr; + + return snd_seq_oss_readq_puts(ctx->readq, ctx->dev, buf, count); +} + +int snd_seq_oss_readq_sysex(struct seq_oss_readq *q, int dev, + struct snd_seq_event *ev) +{ + struct readq_sysex_ctx ctx = { + .readq = q, + .dev = dev + }; + + if ((ev->flags & SNDRV_SEQ_EVENT_LENGTH_MASK) != SNDRV_SEQ_EVENT_LENGTH_VARIABLE) + return 0; + return snd_seq_dump_var_event(ev, readq_dump_sysex, &ctx); +} + /* * copy an event to input queue: * return zero if enqueued diff --git a/sound/core/seq/oss/seq_oss_readq.h b/sound/core/seq/oss/seq_oss_readq.h index f1463f1f449e..8d033ca2d23f 100644 --- a/sound/core/seq/oss/seq_oss_readq.h +++ b/sound/core/seq/oss/seq_oss_readq.h @@ -44,6 +44,8 @@ void snd_seq_oss_readq_delete(struct seq_oss_readq *q); void snd_seq_oss_readq_clear(struct seq_oss_readq *readq); unsigned int snd_seq_oss_readq_poll(struct seq_oss_readq *readq, struct file *file, poll_table *wait); int snd_seq_oss_readq_puts(struct seq_oss_readq *readq, int dev, unsigned char *data, int len); +int snd_seq_oss_readq_sysex(struct seq_oss_readq *q, int dev, + struct snd_seq_event *ev); int snd_seq_oss_readq_put_event(struct seq_oss_readq *readq, union evrec *ev); int snd_seq_oss_readq_put_timestamp(struct seq_oss_readq *readq, unsigned long curt, int seq_mode); int snd_seq_oss_readq_pick(struct seq_oss_readq *q, union evrec *rec);
On Wed, Nov 1, 2017 at 8:49 PM, Takashi Iwai tiwai@suse.de wrote:
Hi Takashi,
Unfortunately it's not possible to test custom patches in syzbot infrastructure. We've experimented with applying a bunch of custom patches in the past and it lead to unrecoverable mess. We were not able to communicate precise state of code with reports, we were not able to provide meaningful report with line numbers that matter (not possible to understand what exactly line caused a bug), developers could (rightfully) suspect that some bugs might be caused the unknown set of private patches, random subset of patches won't apply and that set changes over time and depends on order in which we apply patches, etc. It's also not possible to dedicate a syzkaller instance with a bunch of attached machines for this. First, it will require lots of resources (your request is not unique). Then, whenever we test kernel we get dozens of bugs. What should we do with these bugs? We don't know which are related to your patch and which are not. We can't report them upstream (see above). Basically you would need to go through these dozens of bugs after testing and do something with each of them, but I don't think you want to.
But we are happy to test whatever is in upstream tree (this patch already is).
Re CONFIG_SND_DEBUG=y, should we enable it permanently in syzbot configs?
From our point of view, the more debug configs are enabled, the more
bugs we find, the better. There just must be somebody who will then fix problems uncovered by the config (either bugs of config false positives). If you will take a look on the config attached to the first mail, do you see anything else to fix there re sound? Maybe turn off some deprecated configs that nobody uses for a long time? Or enable some new configs?
Thanks
On Mon, Nov 20, 2017 at 1:47 PM, Dmitry Vyukov dvyukov@google.com wrote:
FYI, we are also rolling out syzbot feature that allows testing of custom patches (but only on the exact reproducer, not general fuzzing):
https://github.com/google/syzkaller/blob/master/docs/syzbot.md#communication...
As far as I understand this was not applicable in this particular case, but if you need it in future, give it a try.
On Mon, 20 Nov 2017 13:47:28 +0100, Dmitry Vyukov wrote:
The bug turned out to be a different issue as the suggested patch may fix, and I believe we already address it by the upstream commit 132d358b183ac6ad8b3fea32ad5e0663456d18d1 ALSA: seq: Fix OSS sysex delivery in OSS emulation
I thought I submitted the fix line, but it might be to another syzbot report or I did wrong.
Re CONFIG_SND_DEBUG=y, should we enable it permanently in syzbot configs?
Yes, it's worth to have CONFIG_SND_DEBUG=y in fuzzer.
In the case of CONFIG_SND_DEBUG, it gives more debug hints by adding WARN_ON(), but the code behavior is almost same.
CONFIG_SND_DYNAMIC_MINORS=y is recommended for modern systems, too.
Thanks!
Takashi
On Mon, Nov 20, 2017 at 2:21 PM, Takashi Iwai tiwai@suse.de wrote:
Yes, you submitted the fix line above. syzbot already detected that the fix reached all of tested branches for this bug.
I've enabled CONFIG_SND_DEBUG and CONFIG_SND_DYNAMIC_MINORS in syzbot configs.
participants (2)
-
Dmitry Vyukov
-
Takashi Iwai