* ALSA: tons of false positives because of snd_ctl_find_id()
@ 2018-02-02 8:29 Dan Carpenter
2018-02-02 9:07 ` Takashi Sakamoto
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2018-02-02 8:29 UTC (permalink / raw)
To: tiwai; +Cc: alsa-devel
Hello Takashi Iwai,
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);
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: ALSA: tons of false positives because of snd_ctl_find_id()
2018-02-02 8:29 ALSA: tons of false positives because of snd_ctl_find_id() Dan Carpenter
@ 2018-02-02 9:07 ` Takashi Sakamoto
2018-02-02 9:57 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Takashi Sakamoto @ 2018-02-02 9:07 UTC (permalink / raw)
To: Dan Carpenter, tiwai; +Cc: alsa-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: ALSA: tons of false positives because of snd_ctl_find_id()
2018-02-02 9:07 ` Takashi Sakamoto
@ 2018-02-02 9:57 ` Dan Carpenter
2018-02-12 13:20 ` Takashi Iwai
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2018-02-02 9:57 UTC (permalink / raw)
To: Takashi Sakamoto; +Cc: tiwai, alsa-devel
On Fri, Feb 02, 2018 at 06:07:14PM +0900, Takashi Sakamoto wrote:
>
> 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.
>
Right. Thanks. I understood all that...
I've actually figured out a hack that will probably work fine to fix my
issue. Smatch provides a way to hand edit the cross function DB:
http://repo.or.cz/smatch.git/blob/HEAD:/smatch_data/db/fixup_kernel.sh
I can probably solve this by adding a few lines of code to that file.
I'm testing it now.
What I was trying to say was even though we don't need to check .index
since we're not using it, it would simplify static analysis if we would
would set it to zero. It shouldn't hurt anything since we're not going
to use it.
Anyway, leave it for now because I've got this easier hack around.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: ALSA: tons of false positives because of snd_ctl_find_id()
2018-02-02 9:57 ` Dan Carpenter
@ 2018-02-12 13:20 ` Takashi Iwai
0 siblings, 0 replies; 4+ messages in thread
From: Takashi Iwai @ 2018-02-12 13:20 UTC (permalink / raw)
To: Dan Carpenter; +Cc: alsa-devel, Takashi Sakamoto
On Fri, 02 Feb 2018 10:57:28 +0100,
Dan Carpenter wrote:
>
> On Fri, Feb 02, 2018 at 06:07:14PM +0900, Takashi Sakamoto wrote:
> >
> > 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.
> >
>
> Right. Thanks. I understood all that...
>
> I've actually figured out a hack that will probably work fine to fix my
> issue. Smatch provides a way to hand edit the cross function DB:
> http://repo.or.cz/smatch.git/blob/HEAD:/smatch_data/db/fixup_kernel.sh
> I can probably solve this by adding a few lines of code to that file.
> I'm testing it now.
>
> What I was trying to say was even though we don't need to check .index
> since we're not using it, it would simplify static analysis if we would
> would set it to zero. It shouldn't hurt anything since we're not going
> to use it.
Actually the value index is copied from the kctl->id before calling
the get and put callbacks. In snd_ctl_elem_read() and
snd_ctl_elem_write(), both call snd_ctl_build_ioff() just before
kctl->get() and kctl->put() calls. This overwrites both index and
numid fields with the correction of the offset.
Takashi
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-02-12 13:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-02 8:29 ALSA: tons of false positives because of snd_ctl_find_id() Dan Carpenter
2018-02-02 9:07 ` Takashi Sakamoto
2018-02-02 9:57 ` Dan Carpenter
2018-02-12 13:20 ` Takashi Iwai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.