[alsa-devel] [PATCH] ALSA: usb-audio: Fix out-of-bound error
The snd_usb_copy_string_desc() retrieves the usb string corresponding to the index number thought the usb_string(). And for NULL-terminated, insert '0' by using the return value of usb_string() as the index of buffer to hold the string.
The problem is that the usb_string() also returns the length of the string read(>= 0), but it can also return a negative value, the error or status value of usb_control_msg(). If iClockSource is '0' as shown below, usb_string() will return -EINVAL. This will result in '0' being inserted into buf[-22], and the following KASAN out-of-bound error message will be output.
AudioControl Interface Descriptor: bLength 8 bDescriptorType 36 bDescriptorSubtype 10 (CLOCK_SOURCE) bClockID 1 bmAttributes 0x07 Internal programmable Clock (synced to SOF) bmControls 0x07 Clock Frequency Control (read/write) Clock Validity Control (read-only) bAssocTerminal 0 iClockSource 0
To fix out-of-bound error, insert 0 only if the return value of usb_string() is greater than 0.
================================================================== BUG: KASAN: stack-out-of-bounds in parse_audio_unit+0x1327/0x1960 [snd_usb_audio] Write of size 1 at addr ffff88007e66735a by task systemd-udevd/18376
CPU: 0 PID: 18376 Comm: systemd-udevd Not tainted 4.13.0+ #3 Hardware name: LG Electronics 15N540-RFLGL/White Tip Mountain, BIOS 15N5 Call Trace: dump_stack+0x63/0x8d print_address_description+0x70/0x290 ? parse_audio_unit+0x1327/0x1960 [snd_usb_audio] kasan_report+0x265/0x350 __asan_store1+0x4a/0x50 parse_audio_unit+0x1327/0x1960 [snd_usb_audio] ? save_stack+0xb5/0xd0 ? save_stack_trace+0x1b/0x20 ? save_stack+0x46/0xd0 ? kasan_kmalloc+0xad/0xe0 ? kmem_cache_alloc_trace+0xff/0x230 ? snd_usb_create_mixer+0xb0/0x4b0 [snd_usb_audio] ? usb_audio_probe+0x4de/0xf40 [snd_usb_audio] ? usb_probe_interface+0x1f5/0x440 ? driver_probe_device+0x3ed/0x660 ? build_feature_ctl+0xb10/0xb10 [snd_usb_audio] ? save_stack_trace+0x1b/0x20 ? init_object+0x69/0xa0 ? snd_usb_find_csint_desc+0xa8/0xf0 [snd_usb_audio] snd_usb_mixer_controls+0x1dc/0x370 [snd_usb_audio] ? build_audio_procunit+0x890/0x890 [snd_usb_audio] ? snd_usb_create_mixer+0xb0/0x4b0 [snd_usb_audio] ? kmem_cache_alloc_trace+0xff/0x230 ? usb_ifnum_to_if+0xbd/0xf0 snd_usb_create_mixer+0x25b/0x4b0 [snd_usb_audio] ? snd_usb_create_stream+0x255/0x2c0 [snd_usb_audio] usb_audio_probe+0x4de/0xf40 [snd_usb_audio] ? snd_usb_autosuspend.part.7+0x30/0x30 [snd_usb_audio] ? __pm_runtime_idle+0x90/0x90 ? kernfs_activate+0xa6/0xc0 ? usb_match_one_id_intf+0xdc/0x130 ? __pm_runtime_set_status+0x2d4/0x450 usb_probe_interface+0x1f5/0x440
Cc: stable@vger.kernel.org Signed-off-by: Jaejoong Kim climbbb.kim@gmail.com ---
The AudioControl Interface Descriptor in commit message is from lsusb output with real usb audio DAC.
The usb audio product causing the OOB are as follows: http://www.lg.com/uk/lg-friends/lg-AFD-1200
It only prints OOB error and usb audio works well. :)
sound/usb/mixer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index e630813..5a83c2c 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -204,7 +204,8 @@ static int snd_usb_copy_string_desc(struct mixer_build *state, int index, char *buf, int maxlen) { int len = usb_string(state->chip->dev, index, buf, maxlen - 1); - buf[len] = 0; + if (len > 0) + buf[len] = 0; return len; }
2017. 11. 15. 오후 5:08에 "Jaejoong Kim" climbbb.kim@gmail.com님이 작성:
The snd_usb_copy_string_desc() retrieves the usb string corresponding to the index number thought the usb_string(). And for NULL-terminated, insert '0' by using the return value of usb_string() as the index of buffer to hold the string.
wrong description for about '0'.
The problem is that the usb_string() also returns the length of the string read(>= 0), but it can also return a negative value, the error or status value of usb_control_msg(). If iClockSource is '0' as shown below, usb_string() will return -EINVAL. This will result in '0' being inserted into buf[-22], and the following KASAN out-of-bound error message will be output.
I found UAC2_CLOCK_SOURCE is nop in the latest kernel. So commit message is not fit in last kernel version. (I test and patch in 4.13, sorry about that)
i will send v2 in the latest kernel
AudioControl Interface Descriptor: bLength 8 bDescriptorType 36 bDescriptorSubtype 10 (CLOCK_SOURCE) bClockID 1 bmAttributes 0x07 Internal programmable Clock (synced to SOF) bmControls 0x07 Clock Frequency Control (read/write) Clock Validity Control (read-only) bAssocTerminal 0 iClockSource 0
To fix out-of-bound error, insert 0 only if the return value of usb_string() is greater than 0.
================================================================== BUG: KASAN: stack-out-of-bounds in parse_audio_unit+0x1327/0x1960 [snd_usb_audio] Write of size 1 at addr ffff88007e66735a by task systemd-udevd/18376
CPU: 0 PID: 18376 Comm: systemd-udevd Not tainted 4.13.0+ #3 Hardware name: LG Electronics 15N540-RFLGL/White Tip Mountain, BIOS 15N5 Call Trace: dump_stack+0x63/0x8d print_address_description+0x70/0x290 ? parse_audio_unit+0x1327/0x1960 [snd_usb_audio] kasan_report+0x265/0x350 __asan_store1+0x4a/0x50 parse_audio_unit+0x1327/0x1960 [snd_usb_audio] ? save_stack+0xb5/0xd0 ? save_stack_trace+0x1b/0x20 ? save_stack+0x46/0xd0 ? kasan_kmalloc+0xad/0xe0 ? kmem_cache_alloc_trace+0xff/0x230 ? snd_usb_create_mixer+0xb0/0x4b0 [snd_usb_audio] ? usb_audio_probe+0x4de/0xf40 [snd_usb_audio] ? usb_probe_interface+0x1f5/0x440 ? driver_probe_device+0x3ed/0x660 ? build_feature_ctl+0xb10/0xb10 [snd_usb_audio] ? save_stack_trace+0x1b/0x20 ? init_object+0x69/0xa0 ? snd_usb_find_csint_desc+0xa8/0xf0 [snd_usb_audio] snd_usb_mixer_controls+0x1dc/0x370 [snd_usb_audio] ? build_audio_procunit+0x890/0x890 [snd_usb_audio] ? snd_usb_create_mixer+0xb0/0x4b0 [snd_usb_audio] ? kmem_cache_alloc_trace+0xff/0x230 ? usb_ifnum_to_if+0xbd/0xf0 snd_usb_create_mixer+0x25b/0x4b0 [snd_usb_audio] ? snd_usb_create_stream+0x255/0x2c0 [snd_usb_audio] usb_audio_probe+0x4de/0xf40 [snd_usb_audio] ? snd_usb_autosuspend.part.7+0x30/0x30 [snd_usb_audio] ? __pm_runtime_idle+0x90/0x90 ? kernfs_activate+0xa6/0xc0 ? usb_match_one_id_intf+0xdc/0x130 ? __pm_runtime_set_status+0x2d4/0x450 usb_probe_interface+0x1f5/0x440
Cc: stable@vger.kernel.org Signed-off-by: Jaejoong Kim climbbb.kim@gmail.com ---
The AudioControl Interface Descriptor in commit message is from lsusb output with real usb audio DAC.
The usb audio product causing the OOB are as follows: http://www.lg.com/uk/lg-friends/lg-AFD-1200
It only prints OOB error and usb audio works well. :)
sound/usb/mixer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index e630813..5a83c2c 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -204,7 +204,8 @@ static int snd_usb_copy_string_desc(struct mixer_build *state, int index, char *buf, int maxlen) { int len = usb_string(state->chip->dev, index, buf, maxlen - 1); - buf[len] = 0; + if (len > 0) + buf[len] = 0; return len; }
thanks. jaejoong
-- 2.7.4
On Wed, 15 Nov 2017 09:57:56 +0100, Jaejoong Kim wrote:
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index e630813..5a83c2c 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -204,7 +204,8 @@ static int snd_usb_copy_string_desc(struct mixer_build *state, int index, char *buf, int maxlen) { int len = usb_string(state->chip->dev, index, buf, maxlen - 1);
buf[len] = 0;
if (len > 0)buf[len] = 0; return len;
I'd rather put an explicit error bail-out, i.e.
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index 91bc8f18791e..296a63a9d09c 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -204,6 +204,10 @@ static int snd_usb_copy_string_desc(struct mixer_build *state, int index, char *buf, int maxlen) { int len = usb_string(state->chip->dev, index, buf, maxlen - 1); + + if (len < 0) + return len; + buf[len] = 0; return len; }
thanks,
Takashi
Hi, Takashi
2017-11-15 18:16 GMT+09:00 Takashi Iwai tiwai@suse.de:
On Wed, 15 Nov 2017 09:57:56 +0100, Jaejoong Kim wrote:
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index e630813..5a83c2c 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -204,7 +204,8 @@ static int snd_usb_copy_string_desc(struct mixer_build *state, int index, char *buf, int maxlen) { int len = usb_string(state->chip->dev, index, buf, maxlen - 1);
buf[len] = 0;
if (len > 0)buf[len] = 0; return len;I'd rather put an explicit error bail-out, i.e.
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index 91bc8f18791e..296a63a9d09c 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -204,6 +204,10 @@ static int snd_usb_copy_string_desc(struct mixer_build *state, int index, char *buf, int maxlen) { int len = usb_string(state->chip->dev, index, buf, maxlen - 1);
if (len < 0)return len;buf[len] = 0; return len;}
OK.
I will resend v2 patch with your suggestion.
thanks,
Takashi
thanks, Jaejoong
participants (2)
-
Jaejoong Kim -
Takashi Iwai