On 2015年04月09日 14:36, Takashi Iwai wrote:

At Thu, 9 Apr 2015 02:07:15 +0900, Takashi Sakamoto wrote:

...

When event originator doesn't set numerical ID in identical information, the event data includes no numerical ID, thus userspace applications cannot identify the control just by unique ID in event data.

This commit fix this bug so as the event data includes all of identical information.

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp

sound/core/control.c | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/sound/core/control.c b/sound/core/control.c index d677c27..6d12e85 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -578,6 +578,7 @@ error:

• Finds the control instance with the given id, and activate or
• inactivate the control together with notification, if changed.

• • The given ID data is filled by full information.
  • Return: 0 if unchanged, 1 if changed, or a negative error code on failure.
  */

@@ -609,6 +610,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id, } ret = 1; unlock:

• *id = kctl->id;

This isn't always correct. When the element is looked by a numid and a kctl has multiple items (count > 0), this will overwrite with a wrong numid.

It also overwrites with a wrong index, too. I should have used a combination of snd_ctl_get_ioff() and snd_ctl_build_ioff() for this purpose. Thanks.

Takashi

...

up_write(&card->controls_rwsem); if (ret > 0) snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id); -- 2.1.0

Well, I'll re-post the patch 01 and 04 with these fixes because the others has somewhat changes of API behaviour, expecially patch 03. I'll postpone it to next developing period.

But how about patch 02? I think filling all identical information gives advantage to userspace applications, instead of a part of given information given.

And I have no confidence about giving index value in add operations for userspace. In my understanding, the index is an offset from the first element in the element set. But actually, via the add operation, we can add new userspace element with an arbitrary index, like:

$ amixer controls (added by an operation with index=0. As a result, 11 is given as numid for this element set.) numid=11,iface=MIXER,name='my-enum-element' numid=12,iface=MIXER,name='my-enum-element',index=1 numid=13,iface=MIXER,name='my-enum-element',index=2 numid=14,iface=MIXER,name='my-enum-element',index=3 - there're some other elements. - (added by another operation with index=5. As a result, 21 is given as numid for this element set.) numid=21,iface=MIXER,name='my-enum-element',index=4 numid=22,iface=MIXER,name='my-enum-element',index=5 numid=23,iface=MIXER,name='my-enum-element',index=6 numid=24,iface=MIXER,name='my-enum-element',index=7 (FYI, addition with index=1-8 causes EBUSY.)

Of cource, we can do this. (added by an operation with 100. As a result, 0 is given as numid for this element set.) numid=0,iface=MIXER,name='my-enum-element',index=100 numid=1,iface=MIXER,name='my-enum-element',index=101 numid=2,iface=MIXER,name='my-enum-element',index=102 numid=3,iface=MIXER,name='my-enum-element',index=103

Is this behaviour legal or not?

# I'm sorry to post these questions just before opening next merge window but it takes me # a long time to understand ALSA control interface and prepare for some helper programs # to confirm its behaviours...

Thanks

Takashi Sakamoto

When event originator doesn't set numerical ID in identical information, the event data includes no numerical ID, thus userspace applications cannot identify the control just by unique ID in event data.

This commit fix this bug so as the event data includes all of identical information.

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/sound/core/control.c b/sound/core/control.c index 00fcaa0..90a9e5d 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -578,6 +578,7 @@ error: * * Finds the control instance with the given id, and activate or * inactivate the control together with notification, if changed. + * The given ID data is filled with full information. * * Return: 0 if unchanged, 1 if changed, or a negative error code on failure. */ @@ -607,6 +608,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id, goto unlock; vd->access |= SNDRV_CTL_ELEM_ACCESS_INACTIVE; } + snd_ctl_build_ioff(id, kctl, index_offset); ret = 1; unlock: up_write(&card->controls_rwsem);

In operations of SNDRV_CTL_IOCTL_ELEM_INFO, identical information in returned value is cleared. This is not better to userspace application.

This commit confirms to return full identical information to the operations.

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/sound/core/control.c b/sound/core/control.c index 90a9e5d..a750846 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1040,8 +1040,12 @@ static int snd_ctl_elem_user_info(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_info *uinfo) { struct user_element *ue = kcontrol->private_data; + unsigned int offset;

+ offset = snd_ctl_get_ioff(kcontrol, &uinfo->id); *uinfo = ue->info; + snd_ctl_build_ioff(&uinfo->id, kcontrol, offset); + return 0; }

@@ -1051,10 +1055,13 @@ static int snd_ctl_elem_user_enum_info(struct snd_kcontrol *kcontrol, struct user_element *ue = kcontrol->private_data; const char *names; unsigned int item; + unsigned int offset;

item = uinfo->value.enumerated.item;

+ offset = snd_ctl_get_ioff(kcontrol, &uinfo->id); *uinfo = ue->info; + snd_ctl_build_ioff(&uinfo->id, kcontrol, offset);

item = min(item, uinfo->value.enumerated.items - 1); uinfo->value.enumerated.item = item;

currently some members related identical information are not fiiled in returned parameter of SNDRV_CTL_IOCTL_ELEM_ADD. This is not better for userspace application.

This commit copies information to returned value. When failing to copy into userspace, the added elements are going to be removed. Then, no applications can lock these elements between adding and removing because these are already locked.

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/sound/core/control.c b/sound/core/control.c index a750846..ccb1ca2 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1214,6 +1214,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, unsigned int access; long private_size; struct user_element *ue; + unsigned int offset; int err;

if (!*info->id.name) @@ -1316,6 +1317,15 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, err = snd_ctl_add(card, kctl); if (err < 0) return err; + offset = snd_ctl_get_ioff(kctl, &info->id); + snd_ctl_build_ioff(&info->id, kctl, offset); + /* + * Here we cannot fill any field for the number of elements added by + * this operation because there're no specific fields. The usage of + * 'owner' field for this purpose may cause any bugs to userspace + * applications because the field originally means PID of a process + * which locks the element. + */

down_write(&card->controls_rwsem); card->user_ctl_count++; @@ -1328,9 +1338,19 @@ static int snd_ctl_elem_add_user(struct snd_ctl_file *file, struct snd_ctl_elem_info __user *_info, int replace) { struct snd_ctl_elem_info info; + int err; + if (copy_from_user(&info, _info, sizeof(info))) return -EFAULT; - return snd_ctl_elem_add(file, &info, replace); + err = snd_ctl_elem_add(file, &info, replace); + if (err < 0) + return err; + if (copy_to_user(_info, &info, sizeof(info))) { + snd_ctl_remove_user_ctl(file, &info.id); + return -EFAULT; + } + + return 0; }

static int snd_ctl_elem_remove(struct snd_ctl_file *file,

An element instance can have several elements with the same feature. Some userspace applications can add such an element instance by add operation with the number of elements. Then, an userspace element instance keeps a memory object to keep state of these elements.

But the element instance has just one memory object for the elements. This causes the same result to each read/write operations to the different elements.

This commit fixes this bug by allocating enough memory objects to element instance for each of elements.

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c index ccb1ca2..23ea738 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1078,9 +1078,12 @@ static int snd_ctl_elem_user_get(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct user_element *ue = kcontrol->private_data; + unsigned int size = ue->elem_data_size; + /* The caller filled whole identical information. */ + char *target = ue->elem_data + ucontrol->id.index * size;

mutex_lock(&ue->card->user_ctl_lock); - memcpy(&ucontrol->value, ue->elem_data, ue->elem_data_size); + memcpy(&ucontrol->value, target, size); mutex_unlock(&ue->card->user_ctl_lock); return 0; } @@ -1090,11 +1093,14 @@ static int snd_ctl_elem_user_put(struct snd_kcontrol *kcontrol, { int change; struct user_element *ue = kcontrol->private_data; + unsigned int size = ue->elem_data_size; + /* The caller filled whole identical information. */ + char *target = ue->elem_data + ucontrol->id.index * size;

mutex_lock(&ue->card->user_ctl_lock); - change = memcmp(&ucontrol->value, ue->elem_data, ue->elem_data_size) != 0; + change = memcmp(&ucontrol->value, target, size) != 0; if (change) - memcpy(ue->elem_data, &ucontrol->value, ue->elem_data_size); + memcpy(target, &ucontrol->value, size); mutex_unlock(&ue->card->user_ctl_lock); return change; } @@ -1278,7 +1284,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, if (err < 0) return err; memcpy(&kctl->id, &info->id, sizeof(kctl->id)); - kctl->private_data = kzalloc(sizeof(struct user_element) + private_size, + kctl->private_data = kzalloc(sizeof(struct user_element) + private_size * count, GFP_KERNEL); if (kctl->private_data == NULL) { kfree(kctl);

On 2015年04月12日 00:42, Takashi Iwai wrote:

At Sat, 11 Apr 2015 17:41:05 +0900, Takashi Sakamoto wrote:

...

An element instance can have several elements with the same feature. Some userspace applications can add such an element instance by add operation with the number of elements. Then, an userspace element instance keeps a memory object to keep state of these elements.

But the element instance has just one memory object for the elements. This causes the same result to each read/write operations to the different elements.

This commit fixes this bug by allocating enough memory objects to element instance for each of elements.

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp

sound/core/control.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c index ccb1ca2..23ea738 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1078,9 +1078,12 @@ static int snd_ctl_elem_user_get(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct user_element *ue = kcontrol->private_data;

• unsigned int size = ue->elem_data_size;

• /* The caller filled whole identical information. */

• char *target = ue->elem_data + ucontrol->id.index * size;

  mutex_lock(&ue->card->user_ctl_lock);

• memcpy(&ucontrol->value, ue->elem_data, ue->elem_data_size);

• memcpy(&ucontrol->value, target, size);

You must check the validity of the address, i.e. index offset. This is a security hole.

OK. I forgot the case that the value of kctl->index starts 1 or larger...

Thanks

Takashi Sakamoto

thanks,

Takashi

...

mutex_unlock(&ue->card->user_ctl_lock); return 0; } @@ -1090,11 +1093,14 @@ static int snd_ctl_elem_user_put(struct snd_kcontrol *kcontrol, { int change; struct user_element *ue = kcontrol->private_data;

• unsigned int size = ue->elem_data_size;

• /* The caller filled whole identical information. */

• char *target = ue->elem_data + ucontrol->id.index * size;

  mutex_lock(&ue->card->user_ctl_lock);

• change = memcmp(&ucontrol->value, ue->elem_data, ue->elem_data_size) != 0;

• change = memcmp(&ucontrol->value, target, size) != 0; if (change)

• memcpy(ue->elem_data, &ucontrol->value, ue->elem_data_size);
  

• memcpy(target, &ucontrol->value, size);
  

  mutex_unlock(&ue->card->user_ctl_lock); return change;

} @@ -1278,7 +1284,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, if (err < 0) return err; memcpy(&kctl->id, &info->id, sizeof(kctl->id));

• kctl->private_data = kzalloc(sizeof(struct user_element) + private_size,

• kctl->private_data = kzalloc(sizeof(struct user_element) + private_size * count, GFP_KERNEL); if (kctl->private_data == NULL) { kfree(kctl);

-- 2.1.0

---

Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel