[smatch stuff] reading beyond the end of the array in build_feature_ctl()

[smatch stuff] reading beyond the end of the array in build_feature_ctl()

[Dan Carpenter]

Hi Daniel,

Smatch complains about a potential read past the end of the array in
build_feature_ctl().

sound/usb/mixer.c +1035 build_feature_ctl(34)
	error: buffer overflow 'audio_feature_info' 13 <= 14

This is in the init code so probably it's correct, and I've just
missed how it works.  Can you take a look?

The problem is that audio_feature_info[] has 13 elements, but Smatch
thinks control - 1 can go up to 14 so we're two elements past the end.

  1035          cval->val_type = audio_feature_info[control-1].type;

Smatch get the value for "control" from "i" in parse_audio_feature_unit().

sound/usb/mixer.c
  1261          } else { /* UAC_VERSION_2 */
  1262                  for (i = 0; i < 30/2; i++) {
  1263                          unsigned int ch_bits = 0;
  1264                          unsigned int ch_read_only = 0;
  1265  
  1266                          for (j = 0; j < channels; j++) {
  1267                                  unsigned int mask = snd_usb_combine_bytes(bmaControls + csize * (j+1), csize);
  1268                                  if (uac2_control_is_readable(mask, i)) {
  1269                                          ch_bits |= (1 << j);
  1270                                          if (!uac2_control_is_writeable(mask, i))
  1271                                                  ch_read_only |= (1 << j);
  1272                                  }
  1273                          }
  1274  
  1275                          /* NOTE: build_feature_ctl() will mark the control read-only if all channels
  1276                           * are marked read-only in the descriptors. Otherwise, the control will be
  1277                           * reported as writeable, but the driver will not actually issue a write
  1278                           * command for read-only channels */
  1279                          if (ch_bits & 1) /* the first channel must be set (for ease of programming) */
  1280                                  build_feature_ctl(state, _ftr, ch_bits, i, &iterm, unitid, ch_read_only);
                                                                                ^
"i" can be 13 here.

  1281                          if (uac2_control_is_readable(master_bits, i))
  1282                                  build_feature_ctl(state, _ftr, 0, i, &iterm, unitid,
                                                                          ^
"i" maybe can go up to 14 here, I'm not sure.

  1283                                                    !uac2_control_is_writeable(master_bits, i));
  1284                  }
  1285          }

regards,
dan carpenter

Re: [smatch stuff] reading beyond the end of the array in build_feature_ctl()

[Takashi Iwai]

At Thu, 6 Oct 2011 16:39:48 +0300,
Dan Carpenter wrote:
> 
> Hi Daniel,
> 
> Smatch complains about a potential read past the end of the array in
> build_feature_ctl().
> 
> sound/usb/mixer.c +1035 build_feature_ctl(34)
> 	error: buffer overflow 'audio_feature_info' 13 <= 14
> 
> This is in the init code so probably it's correct, and I've just
> missed how it works.  Can you take a look?
> 
> The problem is that audio_feature_info[] has 13 elements, but Smatch
> thinks control - 1 can go up to 14 so we're two elements past the end.
> 
>   1035          cval->val_type = audio_feature_info[control-1].type;
> 
> Smatch get the value for "control" from "i" in parse_audio_feature_unit().

It looks like a flaw, indeed.
audio_feature_info[] should contain all data for UAC2_FU_*.

An easy fix is a patch like below.
(It's anyway better to replace the digits there.)
Later we can add the missing entries to audio_feature_info[].


thanks,

Takashi

---
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index b13b7ac..60f65ac 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -1259,7 +1259,7 @@ static int parse_audio_feature_unit(struct mixer_build *state, int unitid, void
 				build_feature_ctl(state, _ftr, 0, i, &iterm, unitid, 0);
 		}
 	} else { /* UAC_VERSION_2 */
-		for (i = 0; i < 30/2; i++) {
+		for (i = 0; i < ARRAY_SIZE(audio_feature_info); i++) {
 			unsigned int ch_bits = 0;
 			unsigned int ch_read_only = 0;