| Message ID | 20230324171812.221086-2-theflamefire89@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ALSA: control: Fix locking around snd_ctl_elem_read/write | expand |
Hi, > -----Original Message----- > From: cip-dev@lists.cip-project.org <cip-dev@lists.cip-project.org> On > Behalf Of Alexander Grund > Sent: Saturday, March 25, 2023 2:18 AM > To: cip-dev@lists.cip-project.org > Cc: uli+cip@fpond.eu > Subject: [cip-dev] [PATCH 4.4 1/3] ALSA: control: code refactoring for > ELEM_READ/ELEM_WRITE operations > > From: Takashi Sakamoto <o-takashi@sakamocchi.jp> Please add original commit id. > > ALSA control core handles ELEM_READ/ELEM_WRITE requests within lock > acquisition of a counting semaphore. The lock is acquired in helper functions > in the end of call path before calling implementations of each driver. > > ioctl(2) with SNDRV_CTL_ELEM_READ > ... > ->snd_ctl_ioctl() > ->snd_ctl_elem_read_user() > ->snd_ctl_elem_read() > ->down_read(controls_rwsem) > ->snd_ctl_find_id() > ->struct snd_kcontrol.get() > ->up_read(controls_rwsem) > > ioctl(2) with SNDRV_CTL_ELEM_WRITE > ... > ->snd_ctl_ioctl() > ->snd_ctl_elem_write_user() > ->snd_ctl_elem_write() > ->down_read(controls_rwsem) > ->snd_ctl_find_id() > ->struct snd_kcontrol.put() > ->up_read(controls_rwsem) > > This commit moves the lock acquisition to middle of the call graph to simplify > the helper functions. As a result: > > ioctl(2) with SNDRV_CTL_ELEM_READ > ... > ->snd_ctl_ioctl() > ->snd_ctl_elem_read_user() > ->down_read(controls_rwsem) > ->snd_ctl_elem_read() > ->snd_ctl_find_id() > ->struct snd_kcontrol.get() > ->up_read(controls_rwsem) > > ioctl(2) with SNDRV_CTL_ELEM_WRITE > ... > ->snd_ctl_ioctl() > ->snd_ctl_elem_write_user() > ->down_read(controls_rwsem) > ->snd_ctl_elem_write() > ->snd_ctl_find_id() > ->struct snd_kcontrol.put() > ->up_read(controls_rwsem) > > Change-Id: I6b39209aaf08afcbeca7c759b77bc96c67db4c77 The original commit doesn't seem to have this tag. > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > Fixes: e8064dec769e6 "ALSA: pcm: Move rwsem lock inside > snd_ctl_elem_read to prevent UAF" > Signed-off-by: Alexander Grund <theflamefire89@gmail.com> > --- > sound/core/control.c | 78 > +++++++++++++++++++++----------------------- > 1 file changed, 38 insertions(+), 40 deletions(-) > > diff --git a/sound/core/control.c b/sound/core/control.c index > 43c8eac250b8a..a042a30d6a728 100644 > --- a/sound/core/control.c > +++ b/sound/core/control.c > @@ -877,24 +877,18 @@ static int snd_ctl_elem_read(struct snd_card *card, > struct snd_kcontrol *kctl; > struct snd_kcontrol_volatile *vd; > unsigned int index_offset; > - int result; > > - down_read(&card->controls_rwsem); > kctl = snd_ctl_find_id(card, &control->id); > - if (kctl == NULL) { > - result = -ENOENT; > - } else { > - index_offset = snd_ctl_get_ioff(kctl, &control->id); > - vd = &kctl->vd[index_offset]; > - if ((vd->access & SNDRV_CTL_ELEM_ACCESS_READ) && > - kctl->get != NULL) { > - snd_ctl_build_ioff(&control->id, kctl, index_offset); > - result = kctl->get(kctl, control); > - } else > - result = -EPERM; > - } > - up_read(&card->controls_rwsem); > - return result; > + if (kctl == NULL) > + return -ENOENT; > + > + index_offset = snd_ctl_get_ioff(kctl, &control->id); > + vd = &kctl->vd[index_offset]; > + if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_READ) && kctl->get > == NULL) > + return -EPERM; > + > + snd_ctl_build_ioff(&control->id, kctl, index_offset); > + return kctl->get(kctl, control); > } > > static int snd_ctl_elem_read_user(struct snd_card *card, @@ -909,8 +903,11 > @@ static int snd_ctl_elem_read_user(struct snd_card *card, > > snd_power_lock(card); > result = snd_power_wait(card, SNDRV_CTL_POWER_D0); > - if (result >= 0) > + if (result >= 0) { > + down_read(&card->controls_rwsem); > result = snd_ctl_elem_read(card, control); > + up_read(&card->controls_rwsem); > + } > snd_power_unlock(card); > if (result >= 0) > if (copy_to_user(_control, control, sizeof(*control))) @@ > -927,30 +924,28 @@ static int snd_ctl_elem_write(struct snd_card *card, > struct snd_ctl_file *file, > unsigned int index_offset; > int result; > > - down_read(&card->controls_rwsem); > kctl = snd_ctl_find_id(card, &control->id); > - if (kctl == NULL) { > - result = -ENOENT; > - } else { > - index_offset = snd_ctl_get_ioff(kctl, &control->id); > - vd = &kctl->vd[index_offset]; > - if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_WRITE) || > - kctl->put == NULL || > - (file && vd->owner && vd->owner != file)) { > - result = -EPERM; > - } else { > - snd_ctl_build_ioff(&control->id, kctl, index_offset); > - result = kctl->put(kctl, control); > - } > - if (result > 0) { > - struct snd_ctl_elem_id id = control->id; > - up_read(&card->controls_rwsem); > - snd_ctl_notify(card, > SNDRV_CTL_EVENT_MASK_VALUE, &id); > - return 0; > - } > + if (kctl == NULL) > + return -ENOENT; > + > + index_offset = snd_ctl_get_ioff(kctl, &control->id); > + vd = &kctl->vd[index_offset]; > + if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_WRITE) || kctl->put > == NULL || > + (file && vd->owner && vd->owner != file)) { > + return -EPERM; > } > - up_read(&card->controls_rwsem); > - return result; > + > + snd_ctl_build_ioff(&control->id, kctl, index_offset); > + result = kctl->put(kctl, control); > + if (result < 0) > + return result; > + > + if (result > 0) { > + struct snd_ctl_elem_id id = control->id; > + snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, > &id); > + } > + > + return 0; > } > > static int snd_ctl_elem_write_user(struct snd_ctl_file *file, @@ -967,8 > +962,11 @@ static int snd_ctl_elem_write_user(struct snd_ctl_file *file, > card = file->card; > snd_power_lock(card); > result = snd_power_wait(card, SNDRV_CTL_POWER_D0); > - if (result >= 0) > + if (result >= 0) { > + down_read(&card->controls_rwsem); > result = snd_ctl_elem_write(card, file, control); > + up_read(&card->controls_rwsem); > + } > snd_power_unlock(card); > if (result >= 0) > if (copy_to_user(_control, control, sizeof(*control))) > -- > 2.40.0 Best regards, Nobuhiro
Hi, > Please add original commit id. becf9e5d553c2389d857a3c178ce80fdb34a02e1 > The original commit doesn't seem to have this tag. Oof, this gets auto-added when operating/comitting inside the Android tree. Just ignore/remove this. Do I need to submit the updated patch (actually only the description changes) as a new mail? If so how'd I do that, e.g. with git send-mail?
diff --git a/sound/core/control.c b/sound/core/control.c index 43c8eac250b8a..a042a30d6a728 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -877,24 +877,18 @@ static int snd_ctl_elem_read(struct snd_card *card, struct snd_kcontrol *kctl; struct snd_kcontrol_volatile *vd; unsigned int index_offset; - int result; - down_read(&card->controls_rwsem); kctl = snd_ctl_find_id(card, &control->id); - if (kctl == NULL) { - result = -ENOENT; - } else { - index_offset = snd_ctl_get_ioff(kctl, &control->id); - vd = &kctl->vd[index_offset]; - if ((vd->access & SNDRV_CTL_ELEM_ACCESS_READ) && - kctl->get != NULL) { - snd_ctl_build_ioff(&control->id, kctl, index_offset); - result = kctl->get(kctl, control); - } else - result = -EPERM; - } - up_read(&card->controls_rwsem); - return result; + if (kctl == NULL) + return -ENOENT; + + index_offset = snd_ctl_get_ioff(kctl, &control->id); + vd = &kctl->vd[index_offset]; + if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_READ) && kctl->get == NULL) + return -EPERM; + + snd_ctl_build_ioff(&control->id, kctl, index_offset); + return kctl->get(kctl, control); } static int snd_ctl_elem_read_user(struct snd_card *card, @@ -909,8 +903,11 @@ static int snd_ctl_elem_read_user(struct snd_card *card, snd_power_lock(card); result = snd_power_wait(card, SNDRV_CTL_POWER_D0); - if (result >= 0) + if (result >= 0) { + down_read(&card->controls_rwsem); result = snd_ctl_elem_read(card, control); + up_read(&card->controls_rwsem); + } snd_power_unlock(card); if (result >= 0) if (copy_to_user(_control, control, sizeof(*control))) @@ -927,30 +924,28 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file, unsigned int index_offset; int result; - down_read(&card->controls_rwsem); kctl = snd_ctl_find_id(card, &control->id); - if (kctl == NULL) { - result = -ENOENT; - } else { - index_offset = snd_ctl_get_ioff(kctl, &control->id); - vd = &kctl->vd[index_offset]; - if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_WRITE) || - kctl->put == NULL || - (file && vd->owner && vd->owner != file)) { - result = -EPERM; - } else { - snd_ctl_build_ioff(&control->id, kctl, index_offset); - result = kctl->put(kctl, control); - } - if (result > 0) { - struct snd_ctl_elem_id id = control->id; - up_read(&card->controls_rwsem); - snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &id); - return 0; - } + if (kctl == NULL) + return -ENOENT; + + index_offset = snd_ctl_get_ioff(kctl, &control->id); + vd = &kctl->vd[index_offset]; + if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_WRITE) || kctl->put == NULL || + (file && vd->owner && vd->owner != file)) { + return -EPERM; } - up_read(&card->controls_rwsem); - return result; + + snd_ctl_build_ioff(&control->id, kctl, index_offset); + result = kctl->put(kctl, control); + if (result < 0) + return result; + + if (result > 0) { + struct snd_ctl_elem_id id = control->id; + snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &id); + } + + return 0; } static int snd_ctl_elem_write_user(struct snd_ctl_file *file, @@ -967,8 +962,11 @@ static int snd_ctl_elem_write_user(struct snd_ctl_file *file, card = file->card; snd_power_lock(card); result = snd_power_wait(card, SNDRV_CTL_POWER_D0); - if (result >= 0) + if (result >= 0) { + down_read(&card->controls_rwsem); result = snd_ctl_elem_write(card, file, control); + up_read(&card->controls_rwsem); + } snd_power_unlock(card); if (result >= 0) if (copy_to_user(_control, control, sizeof(*control)))