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; }

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);

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

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; }