| Message ID | 1428741665-22262-5-git-send-email-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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 >
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 >
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);
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(-)