All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Fix an OOB bug in parse_audio_mixer_unit
@ 2019-08-14  2:36 Hui Peng
  2019-08-14  2:44 ` 彭辉
  2019-08-14  6:36   ` Takashi Iwai
  0 siblings, 2 replies; 11+ messages in thread
From: Hui Peng @ 2019-08-14  2:36 UTC (permalink / raw)
  To: security
  Cc: Hui Peng, Mathias Payer, Jaroslav Kysela, Takashi Iwai,
	Thomas Gleixner, Allison Randal, YueHaibing, Wenwen Wang,
	alsa-devel, linux-kernel

The `uac_mixer_unit_descriptor` shown as below is read from the
device side. In `parse_audio_mixer_unit`, `baSourceID` field is
accessed from index 0 to `bNrInPins` - 1, the current implementation
assumes that descriptor is always valid (the length  of descriptor
is no shorter than 5 + `bNrInPins`). If a descriptor read from
the device side is invalid, it may trigger out-of-bound memory
access.

```
struct uac_mixer_unit_descriptor {
	__u8 bLength;
	__u8 bDescriptorType;
	__u8 bDescriptorSubtype;
	__u8 bUnitID;
	__u8 bNrInPins;
	__u8 baSourceID[];
}
```

This patch fixes the bug by add a sanity check on the length of
the descriptor.

Signed-off-by: Hui Peng <benquike@gmail.com>
Reported-by: Hui Peng <benquike@gmail.com>
Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
---
 sound/usb/mixer.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 7498b5191b68..38202ce67237 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -2091,6 +2091,15 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
 	struct usb_audio_term iterm;
 	int input_pins, num_ins, num_outs;
 	int pin, ich, err;
+	int desc_len = (int) ((unsigned long) state->buffer +
+			state->buflen - (unsigned long) raw_desc);
+
+	if (desc_len < sizeof(*desc) + desc->bNrInPins) {
+		usb_audio_err(state->chip,
+			      "descriptor %d too short\n",
+			      unitid);
+		return -EINVAL;
+	}
 
 	err = uac_mixer_unit_get_channels(state, desc);
 	if (err < 0) {
-- 
2.22.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit
  2019-08-14  2:36 [PATCH] Fix an OOB bug in parse_audio_mixer_unit Hui Peng
@ 2019-08-14  2:44 ` 彭辉
  2019-08-14  6:36   ` Takashi Iwai
  1 sibling, 0 replies; 11+ messages in thread
From: 彭辉 @ 2019-08-14  2:44 UTC (permalink / raw)
  To: security
  Cc: Mathias Payer, alsa-devel, linux-kernel, YueHaibing,
	Takashi Iwai, Thomas Gleixner, Allison Randal

[-- Attachment #1: Type: text/plain, Size: 1967 bytes --]

The attached file is the KASAN report.

On Tue, Aug 13, 2019 at 10:37 PM Hui Peng <benquike@gmail.com> wrote:

> The `uac_mixer_unit_descriptor` shown as below is read from the
> device side. In `parse_audio_mixer_unit`, `baSourceID` field is
> accessed from index 0 to `bNrInPins` - 1, the current implementation
> assumes that descriptor is always valid (the length  of descriptor
> is no shorter than 5 + `bNrInPins`). If a descriptor read from
> the device side is invalid, it may trigger out-of-bound memory
> access.
>
> ```
> struct uac_mixer_unit_descriptor {
>         __u8 bLength;
>         __u8 bDescriptorType;
>         __u8 bDescriptorSubtype;
>         __u8 bUnitID;
>         __u8 bNrInPins;
>         __u8 baSourceID[];
> }
> ```
>
> This patch fixes the bug by add a sanity check on the length of
> the descriptor.
>
> Signed-off-by: Hui Peng <benquike@gmail.com>
> Reported-by: Hui Peng <benquike@gmail.com>
> Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> ---
>  sound/usb/mixer.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 7498b5191b68..38202ce67237 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -2091,6 +2091,15 @@ static int parse_audio_mixer_unit(struct
> mixer_build *state, int unitid,
>         struct usb_audio_term iterm;
>         int input_pins, num_ins, num_outs;
>         int pin, ich, err;
> +       int desc_len = (int) ((unsigned long) state->buffer +
> +                       state->buflen - (unsigned long) raw_desc);
> +
> +       if (desc_len < sizeof(*desc) + desc->bNrInPins) {
> +               usb_audio_err(state->chip,
> +                             "descriptor %d too short\n",
> +                             unitid);
> +               return -EINVAL;
> +       }
>
>         err = uac_mixer_unit_get_channels(state, desc);
>         if (err < 0) {
> --
> 2.22.1
>
>

-- 
May the *Lord* Richly Bless you and yours !

[-- Attachment #2: bug-0813.kasan.report --]
[-- Type: application/octet-stream, Size: 11237 bytes --]

[   14.533959] usb 1-1: new high-speed USB device number 2 using ehci-pci
[   14.663607] usb 1-1: Using ep0 maxpacket: 16
[   14.690028] usb 1-1: string descriptor 0 read error: -22
[   14.691647] usb 1-1: New USB device found, idVendor=046d, idProduct=0a44, bcdDevice= 1.27
[   14.694133] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[   14.703190] usb 1-1: current rate 16732531 is different from the runtime rate 48000
[   14.708416] usb 1-1: current rate 11254477 is different from the runtime rate 48000
[   14.710736] usb 1-1: unit 243 not found!
[   14.721047] usb 1-1: unit 3 not found!
[   14.721572] usb 1-1: unit 0 not found!
[   14.722061] usb 1-1: unit 10: unexpected type 0x03
[   14.722651] usb 1-1: unit 36 not found!
[   14.723135] usb 1-1: unit 7 not found!
[   14.723627] usb 1-1: unit 5 not found!
[   14.724100] usb 1-1: unit 4 not found!
[   14.724566] usb 1-1: unit 16 not found!
[   14.725047] usb 1-1: unit 64 not found!
[   14.725521] usb 1-1: unit 31 not found!
[   14.726003] usb 1-1: unit 128 not found!
[   14.726519] usb 1-1: unit 187 not found!
[   14.727013] usb 1-1: unit 192 not found!
[   14.727499] usb 1-1: unit 37 not found!
[   14.727998] usb 1-1: unit 130 not found!
[   14.728536] usb 1-1: unit 96 not found!
[   14.729079] usb 1-1: unit 33 not found!
[   14.729556] usb 1-1: unit 34 not found!
[   14.730046] usb 1-1: unit 43 not found!
[   14.730522] usb 1-1: unit 135 not found!
[   14.731042] ==================================================================
[   14.731930] BUG: KASAN: slab-out-of-bounds in parse_audio_mixer_unit+0xfb/0x5d0 sound/usb/mixer.c:2120
[   14.732814] Read of size 1 at addr ffff888159209d7d by task kworker/0:2/2408
[   14.733701]
[   14.733881] CPU: 0 PID: 2408 Comm: kworker/0:2 Not tainted 5.3.0-rc4+ #4
[   14.734636] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.11.0-0-g63451fca13-prebuilt.qemu-project.org 04/01/2014
[   14.735939] Workqueue: usb_hub_wq hub_event
[   14.736415] Call Trace:
[   14.736701]  dump_stack+0x5b/0x8b lib/dump_stack.c:115
[   14.737081]  ? parse_audio_mixer_unit+0xfb/0x5d0 sound/usb/mixer.c:2120
[   14.737605]  print_address_description+0x6e/0x390 mm/kasan/report.c:352
[   14.738167]  ? parse_audio_mixer_unit+0xfb/0x5d0 sound/usb/mixer.c:2120
[   14.738689]  ? parse_audio_mixer_unit+0xfb/0x5d0 sound/usb/mixer.c:2120
[   14.739211]  __kasan_report+0x149/0x18d mm/kasan/report.c:483
[   14.739647]  ? parse_audio_mixer_unit+0xfb/0x5d0 sound/usb/mixer.c:2120
[   14.740169]  kasan_report+0xe/0x20 mm/kasan/common.c:612
[   14.740557]  parse_audio_mixer_unit+0xfb/0x5d0 sound/usb/mixer.c:2120
[   14.741060]  ? snd_usb_mixer_controls+0x4a0/0x4a0 sound/usb/mixer.c:2089
[   14.741591]  ? find_audio_control_unit+0x74/0x90 sound/usb/mixer.c:176
[   14.742139]  parse_audio_feature_unit+0x118/0x530 sound/usb/mixer.c:1889
[   14.742671]  ? really_probe+0x2a3/0x3c0 drivers/base/dd.c:549
[   14.743134]  ? driver_probe_device+0x70/0x130 drivers/base/dd.c:709
[   14.743628]  ? bus_probe_device+0xe5/0x110 drivers/base/bus.c:514
[   14.744093]  ? snd_usb_find_desc+0x62/0x90 sound/usb/helper.c:43
[   14.744557]  ? parse_audio_mixer_unit+0x5d0/0x5d0 sound/usb/mixer.c:1820
[   14.745088]  ? find_audio_control_unit+0x74/0x90 sound/usb/mixer.c:176
[   14.745610]  snd_usb_mixer_controls+0x1b8/0x4a0 sound/usb/mixer.c:3129
[   14.746127]  ? parse_audio_selector_unit+0x7c0/0x7c0 sound/usb/mixer.c:3092
[   14.746688]  ? kasan_unpoison_shadow+0x30/0x40 mm/kasan/common.c:162
[   14.747190]  snd_usb_create_mixer+0x1b4/0xb30 sound/usb/mixer.c:3483
[   14.747686]  ? kvm_clock_get_cycles+0xd/0x10 arch/x86/kernel/kvmclock.c:98
[   14.748173]  ? ktime_get_mono_fast_ns+0xd6/0x110 kernel/time/timekeeping.c:457
[   14.748695]  ? snd_usb_mixer_interrupt+0x2e0/0x2e0 sound/usb/mixer.c:3441
[   14.749260]  ? usb_driver_claim_interface+0x10d/0x1c0 drivers/usb/core/driver.c:542
[   14.749829]  ? snd_usb_create_stream+0x176/0x240 sound/usb/card.c:201
[   14.750355]  usb_audio_probe+0xaa5/0x1040 sound/usb/card.c:654
[   14.750810]  ? usb_audio_resume+0x10/0x10 sound/usb/card.c:557
[   14.751265]  ? _raw_write_lock+0xd0/0xd0 kernel/locking/spinlock.c:150
[   14.751711]  ? pm_runtime_enable+0x41/0x140 drivers/base/power/runtime.c:1377
[   14.752185]  ? pvclock_clocksource_read+0xd1/0x180 arch/x86/include/asm/pvclock.h:35 (discriminator 1)
[   14.752726]  ? kvm_clock_get_cycles+0xd/0x10 arch/x86/kernel/kvmclock.c:98
[   14.753210]  ? ktime_get_mono_fast_ns+0xd6/0x110 kernel/time/timekeeping.c:457
[   14.753733]  usb_probe_interface+0x161/0x3d0 drivers/usb/core/driver.c:362
[   14.754220]  really_probe+0x2a3/0x3c0 drivers/base/dd.c:549
[   14.754638]  ? driver_allows_async_probing+0x70/0x70 drivers/base/dd.c:790
[   14.755198]  driver_probe_device+0x70/0x130 drivers/base/dd.c:709
[   14.755673]  ? driver_allows_async_probing+0x70/0x70 drivers/base/dd.c:790
[   14.756266]  bus_for_each_drv+0xd3/0x130 drivers/base/bus.c:454
[   14.756713]  ? bus_rescan_devices+0x10/0x10 drivers/base/bus.c:443
[   14.757187]  __device_attach+0x15e/0x1e0 drivers/base/dd.c:884
[   14.757634]  ? device_bind_driver+0x70/0x70 drivers/base/dd.c:856
[   14.758113]  ? kfree+0x95/0x200 mm/slub.c:1474
[   14.758474]  ? kobject_uevent_env+0x178/0x8b0 lib/kobject_uevent.c:626
[   14.758967]  bus_probe_device+0xe5/0x110 drivers/base/bus.c:514
[   14.759413]  device_add+0x617/0xa10 drivers/base/core.c:2166
[   14.759811]  ? get_device_parent.isra.30+0x2a0/0x2a0
[   14.762597]  ? __kasan_slab_free+0x143/0x180 mm/kasan/common.c:452
[   14.763136]  ? usb_enable_lpm+0x33/0x1f0 drivers/usb/core/hub.c:4207 (discriminator 2)
[   14.763629]  usb_set_configuration+0x671/0xc40 drivers/usb/core/message.c:2024
[   14.764191]  generic_probe+0x32/0x80 drivers/usb/core/generic.c:211
[   14.764600]  really_probe+0x2a3/0x3c0 drivers/base/dd.c:549
[   14.765018]  ? driver_allows_async_probing+0x70/0x70 drivers/base/dd.c:790
[   14.765579]  driver_probe_device+0x70/0x130 drivers/base/dd.c:709
[   14.766058]  ? driver_allows_async_probing+0x70/0x70 drivers/base/dd.c:790
[   14.766619]  bus_for_each_drv+0xd3/0x130 drivers/base/bus.c:454
[   14.767066]  ? bus_rescan_devices+0x10/0x10 drivers/base/bus.c:443
[   14.767541]  __device_attach+0x15e/0x1e0 drivers/base/dd.c:884
[   14.767988]  ? device_bind_driver+0x70/0x70 drivers/base/dd.c:856
[   14.768463]  ? kfree+0x95/0x200 mm/slub.c:1474
[   14.768825]  ? kobject_uevent_env+0x178/0x8b0 lib/kobject_uevent.c:626
[   14.769319]  bus_probe_device+0xe5/0x110 drivers/base/bus.c:514
[   14.769766]  device_add+0x617/0xa10 drivers/base/core.c:2166
[   14.770169]  ? get_device_parent.isra.30+0x2a0/0x2a0
[   14.770730]  usb_new_device+0x3e4/0x740 drivers/usb/core/hub.c:2537
[   14.771168]  hub_event+0x109d/0x1d90 drivers/usb/core/hub.c:5099
[   14.771577]  ? hub_port_debounce+0x180/0x180 drivers/usb/core/hub.c:5363
[   14.772116]  ? rpm_suspend+0x286/0x920 drivers/base/power/runtime.c:679
[   14.772544]  ? pm_runtime_get_if_in_use+0x160/0x160 drivers/base/power/runtime.c:515
[   14.773140]  ? __switch_to_asm+0x40/0x70 arch/x86/entry/entry_64.S:312
[   14.773588]  ? apic_timer_interrupt+0xa/0x20 arch/x86/entry/entry_64.S:830
[   14.774076]  ? _raw_spin_unlock_irqrestore+0xd/0x20 include/linux/compiler.h:223
[   14.774629]  ? _raw_spin_lock_irq+0x76/0xd0 arch/x86/include/asm/atomic.h:200
[   14.775108]  ? read_word_at_a_time+0xe/0x20 include/linux/compiler.h:281
[   14.775585]  ? strscpy+0xbf/0x1e0 lib/string.c:211
[   14.775967]  process_one_work+0x466/0x7f0 kernel/workqueue.c:2269
[   14.776426]  worker_thread+0x64/0x6a0 include/linux/compiler.h:199
[   14.776847]  ? process_one_work+0x7f0/0x7f0 kernel/workqueue.c:2358
[   14.777324]  kthread+0x1b6/0x1e0 kernel/kthread.c:255
[   14.777696]  ? kthread_create_on_node+0xc0/0xc0 kernel/kthread.c:215
[   14.778214]  ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:358
[   14.778624]
[   14.778804] Allocated by task 2408:
[   14.779204]  save_stack+0x19/0x80 mm/kasan/common.c:55
[   14.779586]  __kasan_kmalloc.constprop.4+0xa0/0xd0
[   14.780128]  usb_get_configuration+0x22c/0x21f0 drivers/usb/core/config.c:858
[   14.780643]  usb_new_device+0x4d2/0x740 drivers/usb/core/hub.c:2370
[   14.781080]  hub_event+0x109d/0x1d90 drivers/usb/core/hub.c:5099
[   14.781490]  process_one_work+0x466/0x7f0 kernel/workqueue.c:2269
[   14.781947]  worker_thread+0x64/0x6a0 include/linux/compiler.h:199
[   14.782369]  kthread+0x1b6/0x1e0 kernel/kthread.c:255
[   14.782741]  ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:358
[   14.783174]
[   14.783353] Freed by task 1:
[   14.783686]  save_stack+0x19/0x80 mm/kasan/common.c:55
[   14.784067]  __kasan_slab_free+0x12e/0x180 mm/kasan/common.c:77
[   14.784534]  kfree+0x95/0x200 mm/slub.c:1474
[   14.784878]  krealloc+0x77/0xc0 mm/slab_common.c:1657
[   14.785240]  add_sysfs_param.isra.10+0x61/0x330
[   14.785754]  param_sysfs_init+0x1ca/0x24b kernel/params.c:783
[   14.787941]  do_one_initcall+0x86/0x29f init/main.c:939
[   14.788429]  kernel_init_freeable+0x247/0x2dd init/main.c:1007
[   14.788973]  kernel_init+0xa/0x120 init/main.c:1112
[   14.789363]  ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:358
[   14.789771]
[   14.789951] The buggy address belongs to the object at ffff888159209c80
[   14.789951]  which belongs to the cache kmalloc-256 of size 256
[   14.791348] The buggy address is located 253 bytes inside of
[   14.791348]  256-byte region [ffff888159209c80, ffff888159209d80)
[   14.792654] The buggy address belongs to the page:
[   14.793197] page:ffffea0005648240 refcount:1 mapcount:0 mapping:ffff88815a8013c0 index:0x0
[   14.794146] flags: 0x200000000000200(slab)
[   14.794613] raw: 0200000000000200 dead000000000100 dead000000000122 ffff88815a8013c0
[   14.795480] raw: 0000000000000000 00000000000c000c 00000001ffffffff 0000000000000000
[   14.796346] page dumped because: kasan: bad access detected
[   14.796973]
[   14.797151] Memory state around the buggy address:
[   14.797693]  ffff888159209c00: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
[   14.798506]  ffff888159209c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[   14.799368] >ffff888159209d00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 05
[   14.800178]                                                                 ^
[   14.800978]  ffff888159209d80: fc fc fc fc fc fc fc fc 00 00 00 00 00 00 00 00
[   14.801788]  ffff888159209e00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[   14.802629] ==================================================================
[   14.803438] Disabling lock debugging due to kernel taint
[   14.804073] usb 1-1: unit 104 not found!
[   14.804546] usb 1-1: unit 223 not found!
[   14.804996] usb 1-1: unit 212 not found!
[   14.805451] usb 1-1: unit 183 not found!
[   14.831530] usb 1-1: Warning! Unlikely big volume range (=4294967295), cval->res is probably wrong.
[   14.832618] usb 1-1: [1] FU [Speaker Playback Volume] ch = 2, val = -26754/318/-27029
[   14.844521] hid-generic 0003:046D:0A44.0001: unknown main item tag 0x4
[   14.845354] hid-generic 0003:046D:0A44.0001: item fetching failed at offset 1441790347
[   14.853488] hid-generic: probe of 0003:046D:0A44.0001 failed with error -22

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit
  2019-08-14  2:36 [PATCH] Fix an OOB bug in parse_audio_mixer_unit Hui Peng
@ 2019-08-14  6:36   ` Takashi Iwai
  2019-08-14  6:36   ` Takashi Iwai
  1 sibling, 0 replies; 11+ messages in thread
From: Takashi Iwai @ 2019-08-14  6:36 UTC (permalink / raw)
  To: Hui Peng
  Cc: security, alsa-devel, YueHaibing, Thomas Gleixner,
	Allison Randal, Mathias Payer, Jaroslav Kysela, Takashi Iwai,
	Wenwen Wang, linux-kernel

On Wed, 14 Aug 2019 04:36:24 +0200,
Hui Peng wrote:
> 
> The `uac_mixer_unit_descriptor` shown as below is read from the
> device side. In `parse_audio_mixer_unit`, `baSourceID` field is
> accessed from index 0 to `bNrInPins` - 1, the current implementation
> assumes that descriptor is always valid (the length  of descriptor
> is no shorter than 5 + `bNrInPins`). If a descriptor read from
> the device side is invalid, it may trigger out-of-bound memory
> access.
> 
> ```
> struct uac_mixer_unit_descriptor {
> 	__u8 bLength;
> 	__u8 bDescriptorType;
> 	__u8 bDescriptorSubtype;
> 	__u8 bUnitID;
> 	__u8 bNrInPins;
> 	__u8 baSourceID[];
> }
> ```
> 
> This patch fixes the bug by add a sanity check on the length of
> the descriptor.
> 
> Signed-off-by: Hui Peng <benquike@gmail.com>
> Reported-by: Hui Peng <benquike@gmail.com>
> Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> ---
>  sound/usb/mixer.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 7498b5191b68..38202ce67237 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -2091,6 +2091,15 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>  	struct usb_audio_term iterm;
>  	int input_pins, num_ins, num_outs;
>  	int pin, ich, err;
> +	int desc_len = (int) ((unsigned long) state->buffer +
> +			state->buflen - (unsigned long) raw_desc);
> +
> +	if (desc_len < sizeof(*desc) + desc->bNrInPins) {
> +		usb_audio_err(state->chip,
> +			      "descriptor %d too short\n",
> +			      unitid);
> +		return -EINVAL;
> +	}
>  
>  	err = uac_mixer_unit_get_channels(state, desc);
>  	if (err < 0) {

Hm, what is the desc->bLength value in the error case?

Basically the buffer boundary is already checked against bLength in
snd_usb_find_desc() which is called from obtaining the raw_desc in the
caller of this function (parse_audio_unit()).

So, if any, we need to check bLength for the possible overflow like
below.


thanks,

Takashi

--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -744,6 +744,8 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state,
 		return -EINVAL;
 	if (!desc->bNrInPins)
 		return -EINVAL;
+	if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
+		return -EINVAL;
 
 	switch (state->mixer->protocol) {
 	case UAC_VERSION_1:

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit
@ 2019-08-14  6:36   ` Takashi Iwai
  0 siblings, 0 replies; 11+ messages in thread
From: Takashi Iwai @ 2019-08-14  6:36 UTC (permalink / raw)
  To: Hui Peng
  Cc: security, alsa-devel, YueHaibing, Thomas Gleixner,
	Allison Randal, Mathias Payer, Jaroslav Kysela, Takashi Iwai,
	Wenwen Wang, linux-kernel

On Wed, 14 Aug 2019 04:36:24 +0200,
Hui Peng wrote:
> 
> The `uac_mixer_unit_descriptor` shown as below is read from the
> device side. In `parse_audio_mixer_unit`, `baSourceID` field is
> accessed from index 0 to `bNrInPins` - 1, the current implementation
> assumes that descriptor is always valid (the length  of descriptor
> is no shorter than 5 + `bNrInPins`). If a descriptor read from
> the device side is invalid, it may trigger out-of-bound memory
> access.
> 
> ```
> struct uac_mixer_unit_descriptor {
> 	__u8 bLength;
> 	__u8 bDescriptorType;
> 	__u8 bDescriptorSubtype;
> 	__u8 bUnitID;
> 	__u8 bNrInPins;
> 	__u8 baSourceID[];
> }
> ```
> 
> This patch fixes the bug by add a sanity check on the length of
> the descriptor.
> 
> Signed-off-by: Hui Peng <benquike@gmail.com>
> Reported-by: Hui Peng <benquike@gmail.com>
> Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> ---
>  sound/usb/mixer.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> index 7498b5191b68..38202ce67237 100644
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -2091,6 +2091,15 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>  	struct usb_audio_term iterm;
>  	int input_pins, num_ins, num_outs;
>  	int pin, ich, err;
> +	int desc_len = (int) ((unsigned long) state->buffer +
> +			state->buflen - (unsigned long) raw_desc);
> +
> +	if (desc_len < sizeof(*desc) + desc->bNrInPins) {
> +		usb_audio_err(state->chip,
> +			      "descriptor %d too short\n",
> +			      unitid);
> +		return -EINVAL;
> +	}
>  
>  	err = uac_mixer_unit_get_channels(state, desc);
>  	if (err < 0) {

Hm, what is the desc->bLength value in the error case?

Basically the buffer boundary is already checked against bLength in
snd_usb_find_desc() which is called from obtaining the raw_desc in the
caller of this function (parse_audio_unit()).

So, if any, we need to check bLength for the possible overflow like
below.


thanks,

Takashi

--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -744,6 +744,8 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state,
 		return -EINVAL;
 	if (!desc->bNrInPins)
 		return -EINVAL;
+	if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
+		return -EINVAL;
 
 	switch (state->mixer->protocol) {
 	case UAC_VERSION_1:

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit
  2019-08-14  6:36   ` Takashi Iwai
@ 2019-08-14  9:09     ` Dan Carpenter
  -1 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2019-08-14  9:09 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Hui Peng, security, alsa-devel, YueHaibing, Thomas Gleixner,
	Allison Randal, Mathias Payer, Jaroslav Kysela, Takashi Iwai,
	Wenwen Wang, linux-kernel

On Wed, Aug 14, 2019 at 08:36:42AM +0200, Takashi Iwai wrote:
> On Wed, 14 Aug 2019 04:36:24 +0200,
> Hui Peng wrote:
> > 
> > The `uac_mixer_unit_descriptor` shown as below is read from the
> > device side. In `parse_audio_mixer_unit`, `baSourceID` field is
> > accessed from index 0 to `bNrInPins` - 1, the current implementation
> > assumes that descriptor is always valid (the length  of descriptor
> > is no shorter than 5 + `bNrInPins`). If a descriptor read from
> > the device side is invalid, it may trigger out-of-bound memory
> > access.
> > 
> > ```
> > struct uac_mixer_unit_descriptor {
> > 	__u8 bLength;
> > 	__u8 bDescriptorType;
> > 	__u8 bDescriptorSubtype;
> > 	__u8 bUnitID;
> > 	__u8 bNrInPins;
> > 	__u8 baSourceID[];
> > }
> > ```
> > 
> > This patch fixes the bug by add a sanity check on the length of
> > the descriptor.
> > 
> > Signed-off-by: Hui Peng <benquike@gmail.com>
> > Reported-by: Hui Peng <benquike@gmail.com>
> > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> > ---
> >  sound/usb/mixer.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > index 7498b5191b68..38202ce67237 100644
> > --- a/sound/usb/mixer.c
> > +++ b/sound/usb/mixer.c
> > @@ -2091,6 +2091,15 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
> >  	struct usb_audio_term iterm;
> >  	int input_pins, num_ins, num_outs;
> >  	int pin, ich, err;
> > +	int desc_len = (int) ((unsigned long) state->buffer +
> > +			state->buflen - (unsigned long) raw_desc);
> > +
> > +	if (desc_len < sizeof(*desc) + desc->bNrInPins) {
> > +		usb_audio_err(state->chip,
> > +			      "descriptor %d too short\n",
> > +			      unitid);
> > +		return -EINVAL;
> > +	}
> >  
> >  	err = uac_mixer_unit_get_channels(state, desc);
> >  	if (err < 0) {
> 
> Hm, what is the desc->bLength value in the error case?
> 
> Basically the buffer boundary is already checked against bLength in
> snd_usb_find_desc() which is called from obtaining the raw_desc in the
> caller of this function (parse_audio_unit()).
> 
> So, if any, we need to check bLength for the possible overflow like
> below.
> 
> 
> thanks,
> 
> Takashi
> 
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -744,6 +744,8 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state,
>  		return -EINVAL;
>  	if (!desc->bNrInPins)
>  		return -EINVAL;
> +	if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> +		return -EINVAL;

VERSION 1 and 2 already have a different check:

	if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
		return 0; /* no bmControls -> skip */

So something is possibly off by one.  It's just version 3 which doesn't
have a check.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit
@ 2019-08-14  9:09     ` Dan Carpenter
  0 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2019-08-14  9:09 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Mathias Payer, security, Wenwen Wang, linux-kernel, Takashi Iwai,
	YueHaibing, alsa-devel, Thomas Gleixner, Hui Peng,
	Allison Randal

On Wed, Aug 14, 2019 at 08:36:42AM +0200, Takashi Iwai wrote:
> On Wed, 14 Aug 2019 04:36:24 +0200,
> Hui Peng wrote:
> > 
> > The `uac_mixer_unit_descriptor` shown as below is read from the
> > device side. In `parse_audio_mixer_unit`, `baSourceID` field is
> > accessed from index 0 to `bNrInPins` - 1, the current implementation
> > assumes that descriptor is always valid (the length  of descriptor
> > is no shorter than 5 + `bNrInPins`). If a descriptor read from
> > the device side is invalid, it may trigger out-of-bound memory
> > access.
> > 
> > ```
> > struct uac_mixer_unit_descriptor {
> > 	__u8 bLength;
> > 	__u8 bDescriptorType;
> > 	__u8 bDescriptorSubtype;
> > 	__u8 bUnitID;
> > 	__u8 bNrInPins;
> > 	__u8 baSourceID[];
> > }
> > ```
> > 
> > This patch fixes the bug by add a sanity check on the length of
> > the descriptor.
> > 
> > Signed-off-by: Hui Peng <benquike@gmail.com>
> > Reported-by: Hui Peng <benquike@gmail.com>
> > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> > ---
> >  sound/usb/mixer.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > index 7498b5191b68..38202ce67237 100644
> > --- a/sound/usb/mixer.c
> > +++ b/sound/usb/mixer.c
> > @@ -2091,6 +2091,15 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
> >  	struct usb_audio_term iterm;
> >  	int input_pins, num_ins, num_outs;
> >  	int pin, ich, err;
> > +	int desc_len = (int) ((unsigned long) state->buffer +
> > +			state->buflen - (unsigned long) raw_desc);
> > +
> > +	if (desc_len < sizeof(*desc) + desc->bNrInPins) {
> > +		usb_audio_err(state->chip,
> > +			      "descriptor %d too short\n",
> > +			      unitid);
> > +		return -EINVAL;
> > +	}
> >  
> >  	err = uac_mixer_unit_get_channels(state, desc);
> >  	if (err < 0) {
> 
> Hm, what is the desc->bLength value in the error case?
> 
> Basically the buffer boundary is already checked against bLength in
> snd_usb_find_desc() which is called from obtaining the raw_desc in the
> caller of this function (parse_audio_unit()).
> 
> So, if any, we need to check bLength for the possible overflow like
> below.
> 
> 
> thanks,
> 
> Takashi
> 
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -744,6 +744,8 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state,
>  		return -EINVAL;
>  	if (!desc->bNrInPins)
>  		return -EINVAL;
> +	if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> +		return -EINVAL;

VERSION 1 and 2 already have a different check:

	if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
		return 0; /* no bmControls -> skip */

So something is possibly off by one.  It's just version 3 which doesn't
have a check.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit
  2019-08-14  9:09     ` Dan Carpenter
  (?)
@ 2019-08-14 15:14     ` Takashi Iwai
  -1 siblings, 0 replies; 11+ messages in thread
From: Takashi Iwai @ 2019-08-14 15:14 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Hui Peng, security, alsa-devel, YueHaibing, Thomas Gleixner,
	Allison Randal, Mathias Payer, Jaroslav Kysela, Takashi Iwai,
	Wenwen Wang, linux-kernel

On Wed, 14 Aug 2019 11:09:21 +0200,
Dan Carpenter wrote:
> 
> On Wed, Aug 14, 2019 at 08:36:42AM +0200, Takashi Iwai wrote:
> > On Wed, 14 Aug 2019 04:36:24 +0200,
> > Hui Peng wrote:
> > > 
> > > The `uac_mixer_unit_descriptor` shown as below is read from the
> > > device side. In `parse_audio_mixer_unit`, `baSourceID` field is
> > > accessed from index 0 to `bNrInPins` - 1, the current implementation
> > > assumes that descriptor is always valid (the length  of descriptor
> > > is no shorter than 5 + `bNrInPins`). If a descriptor read from
> > > the device side is invalid, it may trigger out-of-bound memory
> > > access.
> > > 
> > > ```
> > > struct uac_mixer_unit_descriptor {
> > > 	__u8 bLength;
> > > 	__u8 bDescriptorType;
> > > 	__u8 bDescriptorSubtype;
> > > 	__u8 bUnitID;
> > > 	__u8 bNrInPins;
> > > 	__u8 baSourceID[];
> > > }
> > > ```
> > > 
> > > This patch fixes the bug by add a sanity check on the length of
> > > the descriptor.
> > > 
> > > Signed-off-by: Hui Peng <benquike@gmail.com>
> > > Reported-by: Hui Peng <benquike@gmail.com>
> > > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> > > ---
> > >  sound/usb/mixer.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > > index 7498b5191b68..38202ce67237 100644
> > > --- a/sound/usb/mixer.c
> > > +++ b/sound/usb/mixer.c
> > > @@ -2091,6 +2091,15 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
> > >  	struct usb_audio_term iterm;
> > >  	int input_pins, num_ins, num_outs;
> > >  	int pin, ich, err;
> > > +	int desc_len = (int) ((unsigned long) state->buffer +
> > > +			state->buflen - (unsigned long) raw_desc);
> > > +
> > > +	if (desc_len < sizeof(*desc) + desc->bNrInPins) {
> > > +		usb_audio_err(state->chip,
> > > +			      "descriptor %d too short\n",
> > > +			      unitid);
> > > +		return -EINVAL;
> > > +	}
> > >  
> > >  	err = uac_mixer_unit_get_channels(state, desc);
> > >  	if (err < 0) {
> > 
> > Hm, what is the desc->bLength value in the error case?
> > 
> > Basically the buffer boundary is already checked against bLength in
> > snd_usb_find_desc() which is called from obtaining the raw_desc in the
> > caller of this function (parse_audio_unit()).
> > 
> > So, if any, we need to check bLength for the possible overflow like
> > below.
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > --- a/sound/usb/mixer.c
> > +++ b/sound/usb/mixer.c
> > @@ -744,6 +744,8 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state,
> >  		return -EINVAL;
> >  	if (!desc->bNrInPins)
> >  		return -EINVAL;
> > +	if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> > +		return -EINVAL;
> 
> VERSION 1 and 2 already have a different check:
> 
> 	if (desc->bLength < sizeof(*desc) + desc->bNrInPins + 1)
> 		return 0; /* no bmControls -> skip */
>
> So something is possibly off by one.  It's just version 3 which doesn't
> have a check.
> 

No, both are sensible checks.  The first check is about the minimal
size that doesn't contain bmControls bitmap which is optional on some
devices, while the latter checks about the presence of bmControls
field.  Note that the latter returns zero, which means no error, while
the former returns an error.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit
  2019-08-14  6:36   ` Takashi Iwai
  (?)
  (?)
@ 2019-08-14 16:28   ` 彭辉
  2019-08-14 16:33     ` Takashi Iwai
  -1 siblings, 1 reply; 11+ messages in thread
From: 彭辉 @ 2019-08-14 16:28 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Mathias Payer, security, Wenwen Wang, linux-kernel, Takashi Iwai,
	YueHaibing, alsa-devel, Thomas Gleixner, Allison Randal

Hi, Takashi:
Here the problem is that `desc->bLength` is controlled by the device side,
so  `desc->bLength` may not represent the real length of the descriptor.
That is why I use pointer arithmetic operations to derive the real size of
the buffer
in my patch.

On Wed, Aug 14, 2019 at 2:36 AM Takashi Iwai <tiwai@suse.de> wrote:

> On Wed, 14 Aug 2019 04:36:24 +0200,
> Hui Peng wrote:
> >
> > The `uac_mixer_unit_descriptor` shown as below is read from the
> > device side. In `parse_audio_mixer_unit`, `baSourceID` field is
> > accessed from index 0 to `bNrInPins` - 1, the current implementation
> > assumes that descriptor is always valid (the length  of descriptor
> > is no shorter than 5 + `bNrInPins`). If a descriptor read from
> > the device side is invalid, it may trigger out-of-bound memory
> > access.
> >
> > ```
> > struct uac_mixer_unit_descriptor {
> >       __u8 bLength;
> >       __u8 bDescriptorType;
> >       __u8 bDescriptorSubtype;
> >       __u8 bUnitID;
> >       __u8 bNrInPins;
> >       __u8 baSourceID[];
> > }
> > ```
> >
> > This patch fixes the bug by add a sanity check on the length of
> > the descriptor.
> >
> > Signed-off-by: Hui Peng <benquike@gmail.com>
> > Reported-by: Hui Peng <benquike@gmail.com>
> > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> > ---
> >  sound/usb/mixer.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> > index 7498b5191b68..38202ce67237 100644
> > --- a/sound/usb/mixer.c
> > +++ b/sound/usb/mixer.c
> > @@ -2091,6 +2091,15 @@ static int parse_audio_mixer_unit(struct
> mixer_build *state, int unitid,
> >       struct usb_audio_term iterm;
> >       int input_pins, num_ins, num_outs;
> >       int pin, ich, err;
> > +     int desc_len = (int) ((unsigned long) state->buffer +
> > +                     state->buflen - (unsigned long) raw_desc);
> > +
> > +     if (desc_len < sizeof(*desc) + desc->bNrInPins) {
> > +             usb_audio_err(state->chip,
> > +                           "descriptor %d too short\n",
> > +                           unitid);
> > +             return -EINVAL;
> > +     }
> >
> >       err = uac_mixer_unit_get_channels(state, desc);
> >       if (err < 0) {
>
> Hm, what is the desc->bLength value in the error case?
>
> Basically the buffer boundary is already checked against bLength in
> snd_usb_find_desc() which is called from obtaining the raw_desc in the
> caller of this function (parse_audio_unit()).
>
> So, if any, we need to check bLength for the possible overflow like
> below.
>
>
> thanks,
>
> Takashi
>
> --- a/sound/usb/mixer.c
> +++ b/sound/usb/mixer.c
> @@ -744,6 +744,8 @@ static int uac_mixer_unit_get_channels(struct
> mixer_build *state,
>                 return -EINVAL;
>         if (!desc->bNrInPins)
>                 return -EINVAL;
> +       if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> +               return -EINVAL;
>
>         switch (state->mixer->protocol) {
>         case UAC_VERSION_1:
>


-- 
May the *Lord* Richly Bless you and yours !

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit
  2019-08-14 16:28   ` 彭辉
@ 2019-08-14 16:33     ` Takashi Iwai
  2019-08-14 16:52       ` 彭辉
  0 siblings, 1 reply; 11+ messages in thread
From: Takashi Iwai @ 2019-08-14 16:33 UTC (permalink / raw)
  To: 彭辉
  Cc: security, alsa-devel, YueHaibing, Thomas Gleixner,
	Allison Randal, Mathias Payer, Jaroslav Kysela, Takashi Iwai,
	Wenwen Wang, linux-kernel

On Wed, 14 Aug 2019 18:28:39 +0200,
彭辉 wrote:
> 
> Hi, Takashi:
> Here the problem is that `desc->bLength` is controlled by the device side,
> so  `desc->bLength` may not represent the real length of the descriptor.
> That is why I use pointer arithmetic operations to derive the real size of the
> buffer
> in my patch.

But bLength is checked before calling this, i.e. it's already assured
that bLength fits within the buffer limit.  So, the result calls don't
have to care about the buffer limit itself, and they can just
concentrate on overflow over bLength.


thanks,

Takashi

> 
> On Wed, Aug 14, 2019 at 2:36 AM Takashi Iwai <tiwai@suse.de> wrote:
> 
>     On Wed, 14 Aug 2019 04:36:24 +0200,
>     Hui Peng wrote:
>     >
>     > The `uac_mixer_unit_descriptor` shown as below is read from the
>     > device side. In `parse_audio_mixer_unit`, `baSourceID` field is
>     > accessed from index 0 to `bNrInPins` - 1, the current implementation
>     > assumes that descriptor is always valid (the length  of descriptor
>     > is no shorter than 5 + `bNrInPins`). If a descriptor read from
>     > the device side is invalid, it may trigger out-of-bound memory
>     > access.
>     >
>     > ```
>     > struct uac_mixer_unit_descriptor {
>     >       __u8 bLength;
>     >       __u8 bDescriptorType;
>     >       __u8 bDescriptorSubtype;
>     >       __u8 bUnitID;
>     >       __u8 bNrInPins;
>     >       __u8 baSourceID[];
>     > }
>     > ```
>     >
>     > This patch fixes the bug by add a sanity check on the length of
>     > the descriptor.
>     >
>     > Signed-off-by: Hui Peng <benquike@gmail.com>
>     > Reported-by: Hui Peng <benquike@gmail.com>
>     > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
>     > ---
>     >  sound/usb/mixer.c | 9 +++++++++
>     >  1 file changed, 9 insertions(+)
>     >
>     > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>     > index 7498b5191b68..38202ce67237 100644
>     > --- a/sound/usb/mixer.c
>     > +++ b/sound/usb/mixer.c
>     > @@ -2091,6 +2091,15 @@ static int parse_audio_mixer_unit(struct
>     mixer_build *state, int unitid,
>     >       struct usb_audio_term iterm;
>     >       int input_pins, num_ins, num_outs;
>     >       int pin, ich, err;
>     > +     int desc_len = (int) ((unsigned long) state->buffer +
>     > +                     state->buflen - (unsigned long) raw_desc);
>     > +
>     > +     if (desc_len < sizeof(*desc) + desc->bNrInPins) {
>     > +             usb_audio_err(state->chip,
>     > +                           "descriptor %d too short\n",
>     > +                           unitid);
>     > +             return -EINVAL;
>     > +     }
>     > 
>     >       err = uac_mixer_unit_get_channels(state, desc);
>     >       if (err < 0) {
>    
>     Hm, what is the desc->bLength value in the error case?
>    
>     Basically the buffer boundary is already checked against bLength in
>     snd_usb_find_desc() which is called from obtaining the raw_desc in the
>     caller of this function (parse_audio_unit()).
>    
>     So, if any, we need to check bLength for the possible overflow like
>     below.
> 
>     thanks,
>    
>     Takashi
>    
>     --- a/sound/usb/mixer.c
>     +++ b/sound/usb/mixer.c
>     @@ -744,6 +744,8 @@ static int uac_mixer_unit_get_channels(struct
>     mixer_build *state,
>                     return -EINVAL;
>             if (!desc->bNrInPins)
>                     return -EINVAL;
>     +       if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
>     +               return -EINVAL;
>    
>             switch (state->mixer->protocol) {
>             case UAC_VERSION_1:
> 
> --
> May the Lord Richly Bless you and yours !
> 
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit
  2019-08-14 16:33     ` Takashi Iwai
@ 2019-08-14 16:52       ` 彭辉
  2019-08-14 18:21         ` Takashi Iwai
  0 siblings, 1 reply; 11+ messages in thread
From: 彭辉 @ 2019-08-14 16:52 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: Mathias Payer, security, Wenwen Wang, linux-kernel, Takashi Iwai,
	YueHaibing, alsa-devel, Thomas Gleixner, Allison Randal

[-- Attachment #1: Type: text/plain, Size: 4226 bytes --]

Hi, Takashi:

Thanks for the guide.
The new patch is confirmed and attached.

On Wed, Aug 14, 2019 at 12:33 PM Takashi Iwai <tiwai@suse.de> wrote:

> On Wed, 14 Aug 2019 18:28:39 +0200,
> 彭辉 wrote:
> >
> > Hi, Takashi:
> > Here the problem is that `desc->bLength` is controlled by the device
> side,
> > so  `desc->bLength` may not represent the real length of the descriptor.
> > That is why I use pointer arithmetic operations to derive the real size
> of the
> > buffer
> > in my patch.
>
> But bLength is checked before calling this, i.e. it's already assured
> that bLength fits within the buffer limit.  So, the result calls don't
> have to care about the buffer limit itself, and they can just
> concentrate on overflow over bLength.
>
>
> thanks,
>
> Takashi
>
> >
> > On Wed, Aug 14, 2019 at 2:36 AM Takashi Iwai <tiwai@suse.de> wrote:
> >
> >     On Wed, 14 Aug 2019 04:36:24 +0200,
> >     Hui Peng wrote:
> >     >
> >     > The `uac_mixer_unit_descriptor` shown as below is read from the
> >     > device side. In `parse_audio_mixer_unit`, `baSourceID` field is
> >     > accessed from index 0 to `bNrInPins` - 1, the current
> implementation
> >     > assumes that descriptor is always valid (the length  of descriptor
> >     > is no shorter than 5 + `bNrInPins`). If a descriptor read from
> >     > the device side is invalid, it may trigger out-of-bound memory
> >     > access.
> >     >
> >     > ```
> >     > struct uac_mixer_unit_descriptor {
> >     >       __u8 bLength;
> >     >       __u8 bDescriptorType;
> >     >       __u8 bDescriptorSubtype;
> >     >       __u8 bUnitID;
> >     >       __u8 bNrInPins;
> >     >       __u8 baSourceID[];
> >     > }
> >     > ```
> >     >
> >     > This patch fixes the bug by add a sanity check on the length of
> >     > the descriptor.
> >     >
> >     > Signed-off-by: Hui Peng <benquike@gmail.com>
> >     > Reported-by: Hui Peng <benquike@gmail.com>
> >     > Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> >     > ---
> >     >  sound/usb/mixer.c | 9 +++++++++
> >     >  1 file changed, 9 insertions(+)
> >     >
> >     > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
> >     > index 7498b5191b68..38202ce67237 100644
> >     > --- a/sound/usb/mixer.c
> >     > +++ b/sound/usb/mixer.c
> >     > @@ -2091,6 +2091,15 @@ static int parse_audio_mixer_unit(struct
> >     mixer_build *state, int unitid,
> >     >       struct usb_audio_term iterm;
> >     >       int input_pins, num_ins, num_outs;
> >     >       int pin, ich, err;
> >     > +     int desc_len = (int) ((unsigned long) state->buffer +
> >     > +                     state->buflen - (unsigned long) raw_desc);
> >     > +
> >     > +     if (desc_len < sizeof(*desc) + desc->bNrInPins) {
> >     > +             usb_audio_err(state->chip,
> >     > +                           "descriptor %d too short\n",
> >     > +                           unitid);
> >     > +             return -EINVAL;
> >     > +     }
> >     >
> >     >       err = uac_mixer_unit_get_channels(state, desc);
> >     >       if (err < 0) {
> >
> >     Hm, what is the desc->bLength value in the error case?
> >
> >     Basically the buffer boundary is already checked against bLength in
> >     snd_usb_find_desc() which is called from obtaining the raw_desc in
> the
> >     caller of this function (parse_audio_unit()).
> >
> >     So, if any, we need to check bLength for the possible overflow like
> >     below.
> >
> >     thanks,
> >
> >     Takashi
> >
> >     --- a/sound/usb/mixer.c
> >     +++ b/sound/usb/mixer.c
> >     @@ -744,6 +744,8 @@ static int uac_mixer_unit_get_channels(struct
> >     mixer_build *state,
> >                     return -EINVAL;
> >             if (!desc->bNrInPins)
> >                     return -EINVAL;
> >     +       if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
> >     +               return -EINVAL;
> >
> >             switch (state->mixer->protocol) {
> >             case UAC_VERSION_1:
> >
> > --
> > May the Lord Richly Bless you and yours !
> >
> >
>


-- 
May the *Lord* Richly Bless you and yours !

[-- Attachment #2: 0001-Fix-an-OOB-bug-in-parse_audio_mixer_unit.patch --]
[-- Type: text/x-patch, Size: 1488 bytes --]

From da17e2db04d29d39f3298738badd036cf3f256d2 Mon Sep 17 00:00:00 2001
From: Hui Peng <benquike@gmail.com>
Date: Tue, 13 Aug 2019 22:34:04 -0400
Subject: [PATCH] Fix an OOB bug in parse_audio_mixer_unit

The `uac_mixer_unit_descriptor` shown as below is read from the
device side. In `parse_audio_mixer_unit`, `baSourceID` field is
accessed from index 0 to `bNrInPins` - 1, the current implementation
assumes that descriptor is always valid (the length  of descriptor
is no shorter than 5 + `bNrInPins`). If a descriptor read from
the device side is invalid, it may trigger out-of-bound memory
access.

```
struct uac_mixer_unit_descriptor {
	__u8 bLength;
	__u8 bDescriptorType;
	__u8 bDescriptorSubtype;
	__u8 bUnitID;
	__u8 bNrInPins;
	__u8 baSourceID[];
}
```

This patch fixes the bug by add a sanity check on the length of
the descriptor.

Signed-off-by: Hui Peng <benquike@gmail.com>
Reported-by: Hui Peng <benquike@gmail.com>
Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
---
 sound/usb/mixer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index 7498b5191b68..ea487378be17 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -744,6 +744,8 @@ static int uac_mixer_unit_get_channels(struct mixer_build *state,
 		return -EINVAL;
 	if (!desc->bNrInPins)
 		return -EINVAL;
+	if (desc->bLength < sizeof(*desc) + desc->bNrInPins)
+		return -EINVAL;
 
 	switch (state->mixer->protocol) {
 	case UAC_VERSION_1:
-- 
2.22.1


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] Fix an OOB bug in parse_audio_mixer_unit
  2019-08-14 16:52       ` 彭辉
@ 2019-08-14 18:21         ` Takashi Iwai
  0 siblings, 0 replies; 11+ messages in thread
From: Takashi Iwai @ 2019-08-14 18:21 UTC (permalink / raw)
  To: 彭辉
  Cc: security, alsa-devel, YueHaibing, Thomas Gleixner,
	Allison Randal, Mathias Payer, Jaroslav Kysela, Wenwen Wang,
	linux-kernel

On Wed, 14 Aug 2019 18:52:07 +0200,
彭辉 wrote:
> 
> Hi, Takashi:
> 
> Thanks for the guide.
> The new patch is confirmed and attached.

Thanks, applied now.


Takashi

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-08-14 18:21 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-14  2:36 [PATCH] Fix an OOB bug in parse_audio_mixer_unit Hui Peng
2019-08-14  2:44 ` 彭辉
2019-08-14  6:36 ` Takashi Iwai
2019-08-14  6:36   ` Takashi Iwai
2019-08-14  9:09   ` Dan Carpenter
2019-08-14  9:09     ` Dan Carpenter
2019-08-14 15:14     ` Takashi Iwai
2019-08-14 16:28   ` 彭辉
2019-08-14 16:33     ` Takashi Iwai
2019-08-14 16:52       ` 彭辉
2019-08-14 18:21         ` Takashi Iwai

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.