| Message ID | Y8at+W/7OGvEBY8O@kili (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: fireface: fix locking bug in ff400_copy_msg_to_user() | expand |
Hi, On Tue, Jan 17, 2023 at 05:17:29PM +0300, Dan Carpenter wrote: > The ff400_copy_msg_to_user() function drops the spin lock to call > copy_to_user(). However, if the copy_to_user() fails, then it must > take the lock again before returning. Failure to take the lock leads > to a double unlock in the caller, hwdep_read(). > > Fixes: acdebd8b4c0c ("ALSA: fireface: implement message parser for Fireface 400") > Signed-off-by: Dan Carpenter <error27@gmail.com> > --- > sound/firewire/fireface/ff-protocol-former.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) Indeed. Thanks for your care. Acked-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > diff --git a/sound/firewire/fireface/ff-protocol-former.c b/sound/firewire/fireface/ff-protocol-former.c > index f58008762fe6..fa41de978756 100644 > --- a/sound/firewire/fireface/ff-protocol-former.c > +++ b/sound/firewire/fireface/ff-protocol-former.c > @@ -680,28 +680,30 @@ static long ff400_copy_msg_to_user(struct snd_ff *ff, char __user *buf, long cou > struct ff400_msg_parser *parser = ff->msg_parser; > u32 type = SNDRV_FIREWIRE_EVENT_FF400_MESSAGE; > long consumed = 0; > + int ret = 0; > > if (count < 8) > return 0; > > spin_unlock_irq(&ff->lock); > - > if (copy_to_user(buf, &type, sizeof(type))) > - return -EFAULT; > - > + ret = -EFAULT; > spin_lock_irq(&ff->lock); > + if (ret) > + return ret; > > count -= sizeof(type); > consumed += sizeof(type); > > while (count >= sizeof(*parser->msgs) && parser->pull_pos != parser->push_pos) { > spin_unlock_irq(&ff->lock); > - > if (copy_to_user(buf + consumed, parser->msgs + parser->pull_pos, > sizeof(*parser->msgs))) > - return -EFAULT; > - > + ret = -EFAULT; > spin_lock_irq(&ff->lock); > + if (ret) > + return ret; > + > ++parser->pull_pos; > if (parser->pull_pos >= FF400_QUEUE_SIZE) > parser->pull_pos = 0; > -- > 2.35.1 Regards Takashi Sakamoto
On Tue, 17 Jan 2023 15:17:29 +0100, Dan Carpenter wrote: > > The ff400_copy_msg_to_user() function drops the spin lock to call > copy_to_user(). However, if the copy_to_user() fails, then it must > take the lock again before returning. Failure to take the lock leads > to a double unlock in the caller, hwdep_read(). > > Fixes: acdebd8b4c0c ("ALSA: fireface: implement message parser for Fireface 400") > Signed-off-by: Dan Carpenter <error27@gmail.com> Thanks, applied. Takashi
diff --git a/sound/firewire/fireface/ff-protocol-former.c b/sound/firewire/fireface/ff-protocol-former.c index f58008762fe6..fa41de978756 100644 --- a/sound/firewire/fireface/ff-protocol-former.c +++ b/sound/firewire/fireface/ff-protocol-former.c @@ -680,28 +680,30 @@ static long ff400_copy_msg_to_user(struct snd_ff *ff, char __user *buf, long cou struct ff400_msg_parser *parser = ff->msg_parser; u32 type = SNDRV_FIREWIRE_EVENT_FF400_MESSAGE; long consumed = 0; + int ret = 0; if (count < 8) return 0; spin_unlock_irq(&ff->lock); - if (copy_to_user(buf, &type, sizeof(type))) - return -EFAULT; - + ret = -EFAULT; spin_lock_irq(&ff->lock); + if (ret) + return ret; count -= sizeof(type); consumed += sizeof(type); while (count >= sizeof(*parser->msgs) && parser->pull_pos != parser->push_pos) { spin_unlock_irq(&ff->lock); - if (copy_to_user(buf + consumed, parser->msgs + parser->pull_pos, sizeof(*parser->msgs))) - return -EFAULT; - + ret = -EFAULT; spin_lock_irq(&ff->lock); + if (ret) + return ret; + ++parser->pull_pos; if (parser->pull_pos >= FF400_QUEUE_SIZE) parser->pull_pos = 0;
The ff400_copy_msg_to_user() function drops the spin lock to call copy_to_user(). However, if the copy_to_user() fails, then it must take the lock again before returning. Failure to take the lock leads to a double unlock in the caller, hwdep_read(). Fixes: acdebd8b4c0c ("ALSA: fireface: implement message parser for Fireface 400") Signed-off-by: Dan Carpenter <error27@gmail.com> --- sound/firewire/fireface/ff-protocol-former.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)