[alsa-devel] [PATCH 0/3] ALSA: usb-audio: fix OOB and return value check
Hi, This patch series fixes the out-of-bound error caused by the return value of usb_string(). It was descovered by KASAN. The Patch 1 is the V2 about http://www.spinics.net/lists/alsa-devel/msg69487.html Chanes in V2: - put an explicit error bail out(by Takashi iwai) Patch1 was founded by connecting the following product. http://www.lg.com/uk/lg-friends/lg-AFD-1200 I found that it only check if the return value from usb_string() is always zero while modifying OOB KASAN message. So instead of making the modifications to OOB to V2, I sent a patch series. I am sorry to break the mail thread. Thanks jaejoong Jaejoong Kim (3): ALSA: usb-audio: Fix out-of-bound error ALSA: usb-audio: Fix return value check for usb_string() ALSA: usb-audio: Add check return value for usb_string() sound/usb/mixer.c | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-) -- 2.7.4
The snd_usb_copy_string_desc() retrieves the usb string corresponding to the index number through the usb_string(). The problem is that the usb_string() returns the length of the string (>= 0) when successful, but it can also return a negative value about the error case or status of usb_control_msg(). If iClockSource is '0' as shown below, usb_string() will returns -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 it, check usb_string() return value and bail out. ================================================================== 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> --- Changes in V2: - put an explicit error bail-out(by Takashi iwai) sound/usb/mixer.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index e630813..da7cbe7 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; } -- 2.7.4
In case of failure, the usb_string() can return a negative number. Therefore, the return value of snd_usb_copy_string_desc() and get_term_name() can also be negative. Check the return values for these functions as follows: len = snd_usb_copy_string_desc(); if (!len) ... If len is negative, the if-statement is false and fails to handle exceptions. So, we need to change it to a value less than or equal to zero. Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com> --- sound/usb/mixer.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index da7cbe7..8a434b7 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -1235,7 +1235,7 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc, { struct uac_feature_unit_descriptor *desc = raw_desc; struct usb_feature_control_info *ctl_info; - unsigned int len = 0; + int len = 0; int mapped_name = 0; int nameid = uac_feature_unit_iFeature(desc); struct snd_kcontrol *kctl; @@ -1313,14 +1313,14 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc, * - if the connected output can be determined, use it. * - otherwise, anonymous name. */ - if (!len) { + if (len <= 0) { len = get_term_name(state, iterm, kctl->id.name, sizeof(kctl->id.name), 1); - if (!len) + if (len <= 0) len = get_term_name(state, &state->oterm, kctl->id.name, sizeof(kctl->id.name), 1); - if (!len) + if (len <= 0) snprintf(kctl->id.name, sizeof(kctl->id.name), "Feature %d", unitid); } @@ -1343,7 +1343,7 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc, " Switch" : " Volume"); break; default: - if (!len) + if (len <= 0) strlcpy(kctl->id.name, audio_feature_info[control-1].name, sizeof(kctl->id.name)); break; @@ -1610,7 +1610,8 @@ static void build_mixer_unit_ctl(struct mixer_build *state, { struct usb_mixer_elem_info *cval; unsigned int num_outs = uac_mixer_unit_bNrChannels(desc); - unsigned int i, len; + unsigned int i; + int len; struct snd_kcontrol *kctl; const struct usbmix_name_map *map; @@ -1649,7 +1650,7 @@ static void build_mixer_unit_ctl(struct mixer_build *state, if (!len) len = get_term_name(state, iterm, kctl->id.name, sizeof(kctl->id.name), 0); - if (!len) + if (len <= 0) len = sprintf(kctl->id.name, "Mixer Source %d", in_ch + 1); append_ctl_name(kctl, " Volume"); @@ -1947,7 +1948,7 @@ static int build_audio_procunit(struct mixer_build *state, int unitid, len = snd_usb_copy_string_desc(state, nameid, kctl->id.name, sizeof(kctl->id.name)); - if (!len) + if (len <= 0) strlcpy(kctl->id.name, name, sizeof(kctl->id.name)); } append_ctl_name(kctl, " "); @@ -2077,7 +2078,8 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, void *raw_desc) { struct uac_selector_unit_descriptor *desc = raw_desc; - unsigned int i, nameid, len; + unsigned int i, nameid; + int len; int err; struct usb_mixer_elem_info *cval; struct snd_kcontrol *kctl; @@ -2138,9 +2140,10 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, } len = check_mapped_selector_name(state, unitid, i, namelist[i], MAX_ITEM_NAME_LEN); - if (! len && check_input_term(state, desc->baSourceID[i], &iterm) >= 0) + if (!len && check_input_term(state, desc->baSourceID[i], &iterm) >= 0) len = get_term_name(state, &iterm, namelist[i], MAX_ITEM_NAME_LEN, 0); - if (! len) + + if (len <= 0) sprintf(namelist[i], "Input %u", i); } @@ -2164,7 +2167,7 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, else { len = get_term_name(state, &state->oterm, kctl->id.name, sizeof(kctl->id.name), 0); - if (!len) + if (len <= 0) strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name)); if (desc->bDescriptorSubtype == UAC2_CLOCK_SELECTOR) -- 2.7.4
On Tue, 28 Nov 2017 01:36:27 +0100, Jaejoong Kim wrote:
In case of failure, the usb_string() can return a negative number. Therefore, the return value of snd_usb_copy_string_desc() and get_term_name() can also be negative.
Check the return values for these functions as follows:
len = snd_usb_copy_string_desc(); if (!len) ...
If len is negative, the if-statement is false and fails to handle exceptions. So, we need to change it to a value less than or equal to zero.
Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com>
In that case, an easier (and likely safer) solution is to limit snd_usb_copy_string_desc() not to return a negative value. That is, at the first patch, instead of return len for a negative value, return 0. Then the second patch can be dropped. The drawback is that the caller can't know of the error, but the current code doesn't differentiate between error and zero length. And dealing such an error (e.g. EINVAL) as zero-length isn't bad, either, as it just indicates the invalid string, not a fatal error. thanks, Takashi
--- sound/usb/mixer.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index da7cbe7..8a434b7 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -1235,7 +1235,7 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc, { struct uac_feature_unit_descriptor *desc = raw_desc; struct usb_feature_control_info *ctl_info; - unsigned int len = 0; + int len = 0; int mapped_name = 0; int nameid = uac_feature_unit_iFeature(desc); struct snd_kcontrol *kctl; @@ -1313,14 +1313,14 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc, * - if the connected output can be determined, use it. * - otherwise, anonymous name. */ - if (!len) { + if (len <= 0) { len = get_term_name(state, iterm, kctl->id.name, sizeof(kctl->id.name), 1); - if (!len) + if (len <= 0) len = get_term_name(state, &state->oterm, kctl->id.name, sizeof(kctl->id.name), 1); - if (!len) + if (len <= 0) snprintf(kctl->id.name, sizeof(kctl->id.name), "Feature %d", unitid); } @@ -1343,7 +1343,7 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc, " Switch" : " Volume"); break; default: - if (!len) + if (len <= 0) strlcpy(kctl->id.name, audio_feature_info[control-1].name, sizeof(kctl->id.name)); break; @@ -1610,7 +1610,8 @@ static void build_mixer_unit_ctl(struct mixer_build *state, { struct usb_mixer_elem_info *cval; unsigned int num_outs = uac_mixer_unit_bNrChannels(desc); - unsigned int i, len; + unsigned int i; + int len; struct snd_kcontrol *kctl; const struct usbmix_name_map *map;
@@ -1649,7 +1650,7 @@ static void build_mixer_unit_ctl(struct mixer_build *state, if (!len) len = get_term_name(state, iterm, kctl->id.name, sizeof(kctl->id.name), 0); - if (!len) + if (len <= 0) len = sprintf(kctl->id.name, "Mixer Source %d", in_ch + 1); append_ctl_name(kctl, " Volume");
@@ -1947,7 +1948,7 @@ static int build_audio_procunit(struct mixer_build *state, int unitid, len = snd_usb_copy_string_desc(state, nameid, kctl->id.name, sizeof(kctl->id.name)); - if (!len) + if (len <= 0) strlcpy(kctl->id.name, name, sizeof(kctl->id.name)); } append_ctl_name(kctl, " "); @@ -2077,7 +2078,8 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, void *raw_desc) { struct uac_selector_unit_descriptor *desc = raw_desc; - unsigned int i, nameid, len; + unsigned int i, nameid; + int len; int err; struct usb_mixer_elem_info *cval; struct snd_kcontrol *kctl; @@ -2138,9 +2140,10 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, } len = check_mapped_selector_name(state, unitid, i, namelist[i], MAX_ITEM_NAME_LEN); - if (! len && check_input_term(state, desc->baSourceID[i], &iterm) >= 0) + if (!len && check_input_term(state, desc->baSourceID[i], &iterm) >= 0) len = get_term_name(state, &iterm, namelist[i], MAX_ITEM_NAME_LEN, 0); - if (! len) + + if (len <= 0) sprintf(namelist[i], "Input %u", i); }
@@ -2164,7 +2167,7 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, else { len = get_term_name(state, &state->oterm, kctl->id.name, sizeof(kctl->id.name), 0); - if (!len) + if (len <= 0) strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));
if (desc->bDescriptorSubtype == UAC2_CLOCK_SELECTOR) -- 2.7.4
2017-11-28 15:33 GMT+09:00 Takashi Iwai <tiwai@suse.de>:
On Tue, 28 Nov 2017 01:36:27 +0100, Jaejoong Kim wrote:
In case of failure, the usb_string() can return a negative number. Therefore, the return value of snd_usb_copy_string_desc() and get_term_name() can also be negative.
Check the return values for these functions as follows:
len = snd_usb_copy_string_desc(); if (!len) ...
If len is negative, the if-statement is false and fails to handle exceptions. So, we need to change it to a value less than or equal to zero.
Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com>
In that case, an easier (and likely safer) solution is to limit snd_usb_copy_string_desc() not to return a negative value. That is, at the first patch, instead of return len for a negative value, return 0. Then the second patch can be dropped.
I agree. Just return 0 is more simple way.
The drawback is that the caller can't know of the error, but the current code doesn't differentiate between error and zero length. And dealing such an error (e.g. EINVAL) as zero-length isn't bad, either, as it just indicates the invalid string, not a fatal error.
Right. The current code does not distinguish between errors and zero length. I will resend patch1 and patch 3 with your suggestion. Thanks for your feedback. jaejoong
thanks,
Takashi
--- sound/usb/mixer.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index da7cbe7..8a434b7 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -1235,7 +1235,7 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc, { struct uac_feature_unit_descriptor *desc = raw_desc; struct usb_feature_control_info *ctl_info; - unsigned int len = 0; + int len = 0; int mapped_name = 0; int nameid = uac_feature_unit_iFeature(desc); struct snd_kcontrol *kctl; @@ -1313,14 +1313,14 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc, * - if the connected output can be determined, use it. * - otherwise, anonymous name. */ - if (!len) { + if (len <= 0) { len = get_term_name(state, iterm, kctl->id.name, sizeof(kctl->id.name), 1); - if (!len) + if (len <= 0) len = get_term_name(state, &state->oterm, kctl->id.name, sizeof(kctl->id.name), 1); - if (!len) + if (len <= 0) snprintf(kctl->id.name, sizeof(kctl->id.name), "Feature %d", unitid); } @@ -1343,7 +1343,7 @@ static void build_feature_ctl(struct mixer_build *state, void *raw_desc, " Switch" : " Volume"); break; default: - if (!len) + if (len <= 0) strlcpy(kctl->id.name, audio_feature_info[control-1].name, sizeof(kctl->id.name)); break; @@ -1610,7 +1610,8 @@ static void build_mixer_unit_ctl(struct mixer_build *state, { struct usb_mixer_elem_info *cval; unsigned int num_outs = uac_mixer_unit_bNrChannels(desc); - unsigned int i, len; + unsigned int i; + int len; struct snd_kcontrol *kctl; const struct usbmix_name_map *map;
@@ -1649,7 +1650,7 @@ static void build_mixer_unit_ctl(struct mixer_build *state, if (!len) len = get_term_name(state, iterm, kctl->id.name, sizeof(kctl->id.name), 0); - if (!len) + if (len <= 0) len = sprintf(kctl->id.name, "Mixer Source %d", in_ch + 1); append_ctl_name(kctl, " Volume");
@@ -1947,7 +1948,7 @@ static int build_audio_procunit(struct mixer_build *state, int unitid, len = snd_usb_copy_string_desc(state, nameid, kctl->id.name, sizeof(kctl->id.name)); - if (!len) + if (len <= 0) strlcpy(kctl->id.name, name, sizeof(kctl->id.name)); } append_ctl_name(kctl, " "); @@ -2077,7 +2078,8 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, void *raw_desc) { struct uac_selector_unit_descriptor *desc = raw_desc; - unsigned int i, nameid, len; + unsigned int i, nameid; + int len; int err; struct usb_mixer_elem_info *cval; struct snd_kcontrol *kctl; @@ -2138,9 +2140,10 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, } len = check_mapped_selector_name(state, unitid, i, namelist[i], MAX_ITEM_NAME_LEN); - if (! len && check_input_term(state, desc->baSourceID[i], &iterm) >= 0) + if (!len && check_input_term(state, desc->baSourceID[i], &iterm) >= 0) len = get_term_name(state, &iterm, namelist[i], MAX_ITEM_NAME_LEN, 0); - if (! len) + + if (len <= 0) sprintf(namelist[i], "Input %u", i); }
@@ -2164,7 +2167,7 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, else { len = get_term_name(state, &state->oterm, kctl->id.name, sizeof(kctl->id.name), 0); - if (!len) + if (len <= 0) strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name));
if (desc->bDescriptorSubtype == UAC2_CLOCK_SELECTOR) -- 2.7.4
snd_usb_copy_string_desc() returns a negative number if usb_string() fails. In case of failure, we need to check the snd_usb_copy_string_desc()'s return value and add an exception case Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com> --- sound/usb/mixer.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index 8a434b7..3501eb5 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -2162,13 +2162,14 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, if (len) ; else if (nameid) - snd_usb_copy_string_desc(state, nameid, kctl->id.name, - sizeof(kctl->id.name)); - else { + len = snd_usb_copy_string_desc(state, nameid, kctl->id.name, + sizeof(kctl->id.name)); + else len = get_term_name(state, &state->oterm, kctl->id.name, sizeof(kctl->id.name), 0); - if (len <= 0) - strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name)); + + if (len <= 0) { + strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name)); if (desc->bDescriptorSubtype == UAC2_CLOCK_SELECTOR) append_ctl_name(kctl, " Clock Source"); @@ -2177,7 +2178,6 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, else append_ctl_name(kctl, " Playback Source"); } - usb_audio_dbg(state->chip, "[%d] SU [%s] items = %d\n", cval->head.id, kctl->id.name, desc->bNrInPins); return snd_usb_mixer_add_control(&cval->head, kctl); -- 2.7.4
Hi, This patch series fixes the out-of-bound error caused by the return value of usb_string(). It was descovered by KASAN. KASAN OOB warning meesage was founded by connecting the following product. http://www.lg.com/uk/lg-friends/lg-AFD-1200 Changes in v2: - Changes return value check for second patch (by Takashi Iwai) - In case of failure case, return 0 not negative value (by Takashi Iwai) - Put an explicit error and bail out (by Takashi Iwai) Thanks jaejoong Jaejoong Kim (2): ALSA: usb-audio: Fix out-of-bound error ALSA: usb-audio: Add check return value for usb_string() sound/usb/mixer.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) -- 2.7.4
The snd_usb_copy_string_desc() retrieves the usb string corresponding to the index number through the usb_string(). The problem is that the usb_string() returns the length of the string (>= 0) when successful, but it can also return a negative value about the error case or status of usb_control_msg(). If iClockSource is '0' as shown below, usb_string() will returns -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 it, check usb_string()'return value and bail out. ================================================================== 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> --- Changes in v2: - In case of failure case, return 0 not negative value (by Takashi Iwai) - put an explicit error and bail out (by Takashi Iwai) sound/usb/mixer.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index 91bc8f1..3294e3a 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 0; + buf[len] = 0; return len; } -- 2.7.4
On Mon, 04 Dec 2017 07:31:48 +0100, Jaejoong Kim wrote:
The snd_usb_copy_string_desc() retrieves the usb string corresponding to the index number through the usb_string(). The problem is that the usb_string() returns the length of the string (>= 0) when successful, but it can also return a negative value about the error case or status of usb_control_msg().
If iClockSource is '0' as shown below, usb_string() will returns -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 it, check usb_string()'return value and bail out.
================================================================== 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>
Applied, thanks. Takashi
snd_usb_copy_string_desc() returns zero if usb_string() fails. In case of failure, we need to check the snd_usb_copy_string_desc()'s return value and add an exception case Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com> --- Changes in V2: - change return value check. sound/usb/mixer.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index 3294e3a..84b9f9c 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -2165,13 +2165,14 @@ static int parse_audio_selector_unit(struct mixer_build *state, int unitid, if (len) ; else if (nameid) - snd_usb_copy_string_desc(state, nameid, kctl->id.name, + len = snd_usb_copy_string_desc(state, nameid, kctl->id.name, sizeof(kctl->id.name)); - else { + else len = get_term_name(state, &state->oterm, kctl->id.name, sizeof(kctl->id.name), 0); - if (!len) - strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name)); + + if (!len) { + strlcpy(kctl->id.name, "USB", sizeof(kctl->id.name)); if (desc->bDescriptorSubtype == UAC2_CLOCK_SELECTOR) append_ctl_name(kctl, " Clock Source"); -- 2.7.4
On Mon, 04 Dec 2017 07:31:49 +0100, Jaejoong Kim wrote:
snd_usb_copy_string_desc() returns zero if usb_string() fails. In case of failure, we need to check the snd_usb_copy_string_desc()'s return value and add an exception case
Signed-off-by: Jaejoong Kim <climbbb.kim@gmail.com>
Applied, thanks. Takashi
participants (2)
-
Jaejoong Kim -
Takashi Iwai