[alsa-devel] ALSA: tons of false positives because of snd_ctl_find_id()

[Takashi Sakamoto]

Hi Dan,

On Feb 2 2018 17:29, Dan Carpenter wrote:
> Hello Takashi Iwai,

I'm not him, but can proceed this discussion with my understanding about
a design of ALSA control core and interface.

> The patch 72cbfd45fac6: "ALSA: ice1724 - Add ESI Maya44 support" from
> May 6, 2009, leads to the following static checker warning:
> 
> 	sound/pci/ice1712/maya44.c:204 maya_vol_put()
> 	error: buffer overflow 'chip->wm' 2 <= u32max.
> 
> sound/pci/ice1712/maya44.c
>     199  static int maya_vol_put(struct snd_kcontrol *kcontrol,
>     200                          struct snd_ctl_elem_value *ucontrol)
>     201  {
>     202          struct snd_maya44 *chip = snd_kcontrol_chip(kcontrol);
>     203          struct snd_wm8776 *wm =
>     204                  &chip->wm[snd_ctl_get_ioff(kcontrol, &ucontrol->id)];
>                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This is a false positive obviously, but this code is crazy hard for
> static analysis to parse.
> 
> snd_ctl_find_id() in sound/core/control.c
>     682  /**
>     683   * snd_ctl_find_id - find the control instance with the given id
>     684   * @card: the card instance
>     685   * @id: the id to search
>     686   *
>     687   * Finds the control instance with the given id from the card.
>     688   *
>     689   * The caller must down card->controls_rwsem before calling this function
>     690   * (if the race condition can happen).
>     691   *
>     692   * Return: The pointer of the instance if found, or %NULL if not.
>     693   *
>     694   */
>     695  struct snd_kcontrol *snd_ctl_find_id(struct snd_card *card,
>     696                                       struct snd_ctl_elem_id *id)
>     697  {
>     698          struct snd_kcontrol *kctl;
>     699
>     700          if (snd_BUG_ON(!card || !id))
>     701                  return NULL;
>     702          if (id->numid != 0)
>     703                  return snd_ctl_find_numid(card, id->numid);
>                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> On this path, we don't check id->index.  Which is fine because we don't
> use it.  It would really make my life simpler if we could set the
> unchecked fields to zero.  How Smatch works is that we merge all the
> success paths together so it would either be zero or checked which is
> easy to deal with.  In the current code it's either checked or
> unchecked which is just unchecked when you merge the success paths
> together.
> 
> I can probably figure out other ways to deal with this if that's not a
> good idea.
> 
>     704          list_for_each_entry(kctl, &card->controls, list) {
>     705                  if (kctl->id.iface != id->iface)
>     706                          continue;
>     707                  if (kctl->id.device != id->device)
>     708                          continue;
>     709                  if (kctl->id.subdevice != id->subdevice)
>     710                          continue;
>     711                  if (strncmp(kctl->id.name, id->name, sizeof(kctl->id.name)))
>     712                          continue;
>     713                  if (kctl->id.index > id->index)
>     714                          continue;
>     715                  if (kctl->id.index + kctl->count <= id->index)
>     716                          continue;
>     717                  return kctl;
>     718          }
>     719          return NULL;
>     720  }
>     721  EXPORT_SYMBOL(snd_ctl_find_id);

In a design of ALSA control core, each element can be pointed according
to data of 'struct snd_ctl_elem_id'. There're two independent ways to
indicate arbitrary element:
  1. by 'numerical ID' (.numid)
  2. by a combination of 'interface' (.iface), 'device' (.device),
     'sub device' (.subdevice), 'name' (.name) and 'index' (.index).

For our information, in ALSA control core, some elements with the same
attributes are managed by data of 'struct snd_kcontrol'. I call it as
'element set'. The value of '.index' represents offset from the first
element in the element set for a target element.

I don't get your concern clearly. But it's my pleasure that the above
information will help you if you missed the two ways.


Regards

Takashi Sakamoto