UBSAN bug in hda_generic.c

Commit Message

Bob Copeland June 25, 2016, 11:08 a.m. UTC

Hi,

I have UBSAN reporting an out-of-bounds array access (see below) on my
machine.  The following patch fixes the warning for me, but not sure if
that is just papering over some other bug.  Thanks in advance for looking!

From 551e904f7a7aea9e9c03c439e554100643239c5c Mon Sep 17 00:00:00 2001
From: Bob Copeland <[e-mail]>
Date: Sat, 25 Jun 2016 06:53:44 -0400
Subject: [PATCH] ALSA: hda - fix read before array start

UBSAN reports the following warning from accessing path->path[-1]
in set_path_power():

[   16.078040] ================================================================================
[   16.078124] UBSAN: Undefined behaviour in sound/pci/hda/hda_generic.c:3981:17
[   16.078198] index -1 is out of range for type 'hda_nid_t [10]'
[   16.078270] CPU: 2 PID: 1738 Comm: modprobe Not tainted 4.7.0-rc1-wt+ #47
[   16.078274] Hardware name: LENOVO 3443CTO/3443CTO, BIOS G6ET23WW (1.02 ) 08/14/2012
[   16.078278]  ffff8800cb246000 ffff8800cb3638b8 ffffffff815c4fe3 0000000000000032
[   16.078286]  ffff8800cb3638e0 ffffffffffffffff ffff8800cb3638d0 ffffffff8162443d
[   16.078294]  ffffffffa0894200 ffff8800cb363920 ffffffff81624af7 0000000000000292
[   16.078302] Call Trace:
[   16.078311]  [<ffffffff815c4fe3>] dump_stack+0x86/0xd3
[   16.078317]  [<ffffffff8162443d>] ubsan_epilogue+0xd/0x40
[   16.078324]  [<ffffffff81624af7>] __ubsan_handle_out_of_bounds+0x67/0x70
[   16.078335]  [<ffffffffa087665f>] set_path_power+0x1bf/0x230 [snd_hda_codec_generic]
[   16.078344]  [<ffffffffa087880d>] add_pin_power_ctls+0x8d/0xc0 [snd_hda_codec_generic]
[   16.078352]  [<ffffffffa087f190>] ? pin_power_down_callback+0x20/0x20 [snd_hda_codec_generic]
[   16.078360]  [<ffffffffa0878947>] add_all_pin_power_ctls+0x107/0x150 [snd_hda_codec_generic]
[   16.078370]  [<ffffffffa08842b3>] snd_hda_gen_parse_auto_config+0x2d73/0x49e0 [snd_hda_codec_generic]
[   16.078376]  [<ffffffff81173360>] ? trace_hardirqs_on_caller+0x1b0/0x2c0
[   16.078390]  [<ffffffffa089df27>] alc_parse_auto_config+0x147/0x310 [snd_hda_codec_realtek]
[   16.078402]  [<ffffffffa08a332a>] patch_alc269+0x23a/0x560 [snd_hda_codec_realtek]
[   16.078417]  [<ffffffffa0838644>] hda_codec_driver_probe+0xa4/0x1a0 [snd_hda_codec]
[   16.078424]  [<ffffffff817bbac1>] driver_probe_device+0x101/0x380
[   16.078430]  [<ffffffff817bbdf9>] __driver_attach+0xb9/0x100
[   16.078438]  [<ffffffff817bbd40>] ? driver_probe_device+0x380/0x380
[   16.078444]  [<ffffffff817b8d20>] bus_for_each_dev+0x70/0xc0
[   16.078449]  [<ffffffff817bb087>] driver_attach+0x27/0x50
[   16.078454]  [<ffffffff817ba956>] bus_add_driver+0x166/0x2c0
[   16.078460]  [<ffffffffa0369000>] ? 0xffffffffa0369000
[   16.078465]  [<ffffffff817bd13d>] driver_register+0x7d/0x130
[   16.078477]  [<ffffffffa083816f>] __hda_codec_driver_register+0x6f/0x90 [snd_hda_codec]
[   16.078488]  [<ffffffffa036901e>] realtek_driver_init+0x1e/0x1000 [snd_hda_codec_realtek]
[   16.078493]  [<ffffffff8100215e>] do_one_initcall+0x4e/0x1d0
[   16.078499]  [<ffffffff8119f54d>] ? rcu_read_lock_sched_held+0x6d/0x80
[   16.078504]  [<ffffffff813701b1>] ? kmem_cache_alloc_trace+0x391/0x560
[   16.078510]  [<ffffffff812bb314>] ? do_init_module+0x28/0x273
[   16.078515]  [<ffffffff812bb387>] do_init_module+0x9b/0x273
[   16.078522]  [<ffffffff811e3782>] load_module+0x20b2/0x3410
[   16.078527]  [<ffffffff811df140>] ? m_show+0x210/0x210
[   16.078533]  [<ffffffff813b2b26>] ? kernel_read+0x66/0xe0
[   16.078541]  [<ffffffff811e4cfa>] SYSC_finit_module+0xba/0xc0
[   16.078547]  [<ffffffff811e4d1e>] SyS_finit_module+0xe/0x10
[   16.078552]  [<ffffffff81a860fc>] entry_SYSCALL_64_fastpath+0x1f/0xbd
[   16.078556] ================================================================================

Fix by checking path->depth before use.

Signed-off-by: Bob Copeland <[e-mail]>
---
 sound/pci/hda/hda_generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Bob Copeland June 25, 2016, 11:52 a.m. UTC | #1

On Sat, Jun 25, 2016 at 01:28:00PM +0200, Takashi Iwai wrote:
> On Sat, 25 Jun 2016 13:08:02 +0200,
> Bob Copeland wrote:
> > 
> > Hi,
> > 
> > I have UBSAN reporting an out-of-bounds array access (see below) on my
> > machine.  The following patch fixes the warning for me, but not sure if
> > that is just papering over some other bug.  Thanks in advance for looking!
> 
> A better check would be to put
> 	if (!path->depth)
> 		continue;
> before both path->path[0] and path->path[path->depth - 1]
> evaluations.  path->depth=1 cannot exist, but path->depth=0 might be.
> 
> Could you resubmit with that fix?

Sure, patch to follow in a separate mail so that patchwork can pick it up.

Patch

diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index 320445f3bf73..bc23a9d8e7b3 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -3978,7 +3978,7 @@  static hda_nid_t set_path_power(struct hda_codec *codec, hda_nid_t nid,
 	for (n = 0; n < spec->paths.used; n++) {
 		path = snd_array_elem(&spec->paths, n);
 		if (path->path[0] == nid ||
-		    path->path[path->depth - 1] == nid) {
+		    (path->depth > 0 && path->path[path->depth - 1] == nid)) {
 			bool pin_old = path->pin_enabled;
 			bool stream_old = path->stream_enabled;