From ef21e1750158558d8369067e94d02d285011c37e Mon Sep 17 00:00:00 2001 From: Jaejoong Kim Date: Tue, 27 Feb 2018 11:15:59 +0900 Subject: ALSA: Use scnprintf() instead of snprintf() for show The show() method should use scnprintf() not snprintf() because snprintf() may returns a value that exceeds its second argument. Signed-off-by: Jaejoong Kim Signed-off-by: Takashi Iwai --- sound/core/init.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'sound/core') diff --git a/sound/core/init.c b/sound/core/init.c index 4fa5dd955740..79b4df1c1dc7 100644 --- a/sound/core/init.c +++ b/sound/core/init.c @@ -670,7 +670,7 @@ card_id_show_attr(struct device *dev, struct device_attribute *attr, char *buf) { struct snd_card *card = container_of(dev, struct snd_card, card_dev); - return snprintf(buf, PAGE_SIZE, "%s\n", card->id); + return scnprintf(buf, PAGE_SIZE, "%s\n", card->id); } static ssize_t @@ -710,7 +710,7 @@ card_number_show_attr(struct device *dev, struct device_attribute *attr, char *buf) { struct snd_card *card = container_of(dev, struct snd_card, card_dev); - return snprintf(buf, PAGE_SIZE, "%i\n", card->number); + return scnprintf(buf, PAGE_SIZE, "%i\n", card->number); } static DEVICE_ATTR(number, S_IRUGO, card_number_show_attr, NULL); -- cgit v1.2.3 From dd5f313be0d4d7a46f0e82c42aed4fbc784699ea Mon Sep 17 00:00:00 2001 From: Richard Fitzgerald Date: Tue, 27 Feb 2018 17:29:54 +0000 Subject: ALSA: control: Fix a bunch of whitespace errors Remove a bunch of trailing whitespace errors. They are fairly annoying if you have your editor set to strip trailing whitespace because you find you've introduced more changes than you were trying to make. Signed-off-by: Richard Fitzgerald Signed-off-by: Takashi Iwai --- sound/core/control.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) (limited to 'sound/core') diff --git a/sound/core/control.c b/sound/core/control.c index 8a77620a3854..69734b0eafd0 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -105,7 +105,7 @@ static void snd_ctl_empty_read_queue(struct snd_ctl_file * ctl) { unsigned long flags; struct snd_kctl_event *cread; - + spin_lock_irqsave(&ctl->read_lock, flags); while (!list_empty(&ctl->events)) { cread = snd_kctl_event(ctl->events.next); @@ -159,7 +159,7 @@ void snd_ctl_notify(struct snd_card *card, unsigned int mask, unsigned long flags; struct snd_ctl_file *ctl; struct snd_kctl_event *ev; - + if (snd_BUG_ON(!card || !id)) return; if (card->shutdown) @@ -213,7 +213,7 @@ static int snd_ctl_new(struct snd_kcontrol **kctl, unsigned int count, { unsigned int size; unsigned int idx; - + if (count == 0 || count > MAX_CONTROL_COUNT) return -EINVAL; @@ -238,7 +238,7 @@ static int snd_ctl_new(struct snd_kcontrol **kctl, unsigned int count, * @ncontrol: the initialization record * @private_data: the private data to set * - * Allocates a new struct snd_kcontrol instance and initialize from the given + * Allocates a new struct snd_kcontrol instance and initialize from the given * template. When the access field of ncontrol is 0, it's assumed as * READWRITE access. When the count field is 0, it's assumes as one. * @@ -251,7 +251,7 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol, unsigned int count; unsigned int access; int err; - + if (snd_BUG_ON(!ncontrol || !ncontrol->info)) return NULL; @@ -753,7 +753,7 @@ static int snd_ctl_elem_list(struct snd_card *card, struct snd_ctl_elem_id id; unsigned int offset, space, jidx; int err = 0; - + if (copy_from_user(&list, _list, sizeof(list))) return -EFAULT; offset = list.offset; @@ -827,7 +827,7 @@ static int snd_ctl_elem_info(struct snd_ctl_file *ctl, struct snd_kcontrol_volatile *vd; unsigned int index_offset; int result; - + down_read(&card->controls_rwsem); kctl = snd_ctl_find_id(card, &info->id); if (kctl == NULL) { @@ -992,7 +992,7 @@ static int snd_ctl_elem_lock(struct snd_ctl_file *file, struct snd_kcontrol *kctl; struct snd_kcontrol_volatile *vd; int result; - + if (copy_from_user(&id, _id, sizeof(id))) return -EFAULT; down_write(&card->controls_rwsem); @@ -1020,7 +1020,7 @@ static int snd_ctl_elem_unlock(struct snd_ctl_file *file, struct snd_kcontrol *kctl; struct snd_kcontrol_volatile *vd; int result; - + if (copy_from_user(&id, _id, sizeof(id))) return -EFAULT; down_write(&card->controls_rwsem); -- cgit v1.2.3 From 2e2c177ca84aff092c3c96714b0f6a12900f3946 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Thu, 8 Mar 2018 08:26:48 +0100 Subject: ALSA: vmaster: Propagate slave error In slave_update() of vmaster code ignores the error from the slave get() callback and copies the values. It's not only about the missing error code but also that this may potentially lead to a leak of uninitialized variables when the slave get() don't clear them. This patch fixes slave_update() not to copy the potentially uninitialized values when an error is returned from the slave get() callback, and to propagate the error value properly. Signed-off-by: Takashi Iwai --- sound/core/vmaster.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'sound/core') diff --git a/sound/core/vmaster.c b/sound/core/vmaster.c index 8632301489fa..b67de2bb06a2 100644 --- a/sound/core/vmaster.c +++ b/sound/core/vmaster.c @@ -68,10 +68,13 @@ static int slave_update(struct link_slave *slave) return -ENOMEM; uctl->id = slave->slave.id; err = slave->slave.get(&slave->slave, uctl); + if (err < 0) + goto error; for (ch = 0; ch < slave->info.count; ch++) slave->vals[ch] = uctl->value.integer.value[ch]; + error: kfree(uctl); - return 0; + return err < 0 ? err : 0; } /* get the slave ctl info and save the initial values */ -- cgit v1.2.3 From 7a33a02ffb0620b01892c6c6808bb711b3f63e9c Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Thu, 8 Mar 2018 08:32:41 +0100 Subject: ALSA: vmaster: Zero-clear ctl before calling slave get Use kzalloc() instead of kmalloc() so that we don't need to rely fully on the slave get() callback to clear the control value that might be copied to user-space. Signed-off-by: Takashi Iwai --- sound/core/vmaster.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'sound/core') diff --git a/sound/core/vmaster.c b/sound/core/vmaster.c index b67de2bb06a2..9e96186742d0 100644 --- a/sound/core/vmaster.c +++ b/sound/core/vmaster.c @@ -63,7 +63,7 @@ static int slave_update(struct link_slave *slave) struct snd_ctl_elem_value *uctl; int err, ch; - uctl = kmalloc(sizeof(*uctl), GFP_KERNEL); + uctl = kzalloc(sizeof(*uctl), GFP_KERNEL); if (!uctl) return -ENOMEM; uctl->id = slave->slave.id; -- cgit v1.2.3 From 5730f9f744cfe20b771adc33f3b476b95d3eebba Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Tue, 13 Mar 2018 11:18:57 +0100 Subject: ALSA: pcm: Remove VLA usage A helper function used by snd_pcm_hw_refine() still keeps using VLA for timestamps of hw constraint rules that are non-fixed size. Let's replace the VLA with a simple kmalloc() array. Reference: https://lkml.org/lkml/2018/3/7/621 Reviewed-by: Takashi Sakamoto Signed-off-by: Takashi Iwai --- sound/core/pcm_native.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) (limited to 'sound/core') diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 77ba50ddcf9e..756a9a3884a5 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -323,7 +323,7 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, struct snd_pcm_hw_constraints *constrs = &substream->runtime->hw_constraints; unsigned int k; - unsigned int rstamps[constrs->rules_num]; + unsigned int *rstamps; unsigned int vstamps[SNDRV_PCM_HW_PARAM_LAST_INTERVAL + 1]; unsigned int stamp; struct snd_pcm_hw_rule *r; @@ -331,7 +331,7 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, struct snd_mask old_mask; struct snd_interval old_interval; bool again; - int changed; + int changed, err = 0; /* * Each application of rule has own sequence number. @@ -339,8 +339,9 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, * Each member of 'rstamps' array represents the sequence number of * recent application of corresponding rule. */ - for (k = 0; k < constrs->rules_num; k++) - rstamps[k] = 0; + rstamps = kcalloc(constrs->rules_num, sizeof(unsigned int), GFP_KERNEL); + if (!rstamps) + return -ENOMEM; /* * Each member of 'vstamps' array represents the sequence number of @@ -398,8 +399,10 @@ retry: } changed = r->func(params, r); - if (changed < 0) - return changed; + if (changed < 0) { + err = changed; + goto out; + } /* * When the parameter is changed, notify it to the caller @@ -430,7 +433,9 @@ retry: if (again) goto retry; - return 0; + out: + kfree(rstamps); + return err; } static int fixup_unreferenced_params(struct snd_pcm_substream *substream, -- cgit v1.2.3 From 09b9ddfaa18317f463085d602cf5f60a5fa88665 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Tue, 13 Mar 2018 11:31:25 +0100 Subject: ALSA: pcm: Use krealloc() for resizing the rules array Just a minor simplification. Change from kcalloc() shouldn't matter as each array element is fully initialized. Reviewed-by: Takashi Sakamoto Signed-off-by: Takashi Iwai --- sound/core/pcm_lib.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) (limited to 'sound/core') diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index a83152e7d387..f4a19509cccf 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -1129,16 +1129,12 @@ int snd_pcm_hw_rule_add(struct snd_pcm_runtime *runtime, unsigned int cond, if (constrs->rules_num >= constrs->rules_all) { struct snd_pcm_hw_rule *new; unsigned int new_rules = constrs->rules_all + 16; - new = kcalloc(new_rules, sizeof(*c), GFP_KERNEL); + new = krealloc(constrs->rules, new_rules * sizeof(*c), + GFP_KERNEL); if (!new) { va_end(args); return -ENOMEM; } - if (constrs->rules) { - memcpy(new, constrs->rules, - constrs->rules_num * sizeof(*c)); - kfree(constrs->rules); - } constrs->rules = new; constrs->rules_all = new_rules; } -- cgit v1.2.3 From 02a5d6925cd34c3b774bdb8eefb057c40a30e870 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Thu, 22 Mar 2018 18:10:14 +0100 Subject: ALSA: pcm: Avoid potential races between OSS ioctls and read/write Although we apply the params_lock mutex to the whole read and write operations as well as snd_pcm_oss_change_params(), we may still face some races. First off, the params_lock is taken inside the read and write loop. This is intentional for avoiding the too long locking, but it allows the in-between parameter change, which might lead to invalid pointers. We check the readiness of the stream and set up via snd_pcm_oss_make_ready() at the beginning of read and write, but it's called only once, by assuming that it remains ready in the rest. Second, many ioctls that may change the actual parameters (i.e. setting runtime->oss.params=1) aren't protected, hence they can be processed in a half-baked state. This patch is an attempt to plug these holes. The stream readiness check is moved inside the read/write inner loop, so that the stream is always set up in a proper state before further processing. Also, each ioctl that may change the parameter is wrapped with the params_lock for avoiding the races. The issues were triggered by syzkaller in a few different scenarios, particularly the one below appearing as GPF in loopback_pos_update. Reported-by: syzbot+c4227aec125487ec3efa@syzkaller.appspotmail.com Cc: Signed-off-by: Takashi Iwai --- sound/core/oss/pcm_oss.c | 134 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 106 insertions(+), 28 deletions(-) (limited to 'sound/core') diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c index 02298c9c6020..f8bfee8022e0 100644 --- a/sound/core/oss/pcm_oss.c +++ b/sound/core/oss/pcm_oss.c @@ -823,8 +823,8 @@ static int choose_rate(struct snd_pcm_substream *substream, return snd_pcm_hw_param_near(substream, params, SNDRV_PCM_HW_PARAM_RATE, best_rate, NULL); } -static int snd_pcm_oss_change_params(struct snd_pcm_substream *substream, - bool trylock) +/* call with params_lock held */ +static int snd_pcm_oss_change_params_locked(struct snd_pcm_substream *substream) { struct snd_pcm_runtime *runtime = substream->runtime; struct snd_pcm_hw_params *params, *sparams; @@ -838,11 +838,8 @@ static int snd_pcm_oss_change_params(struct snd_pcm_substream *substream, const struct snd_mask *sformat_mask; struct snd_mask mask; - if (trylock) { - if (!(mutex_trylock(&runtime->oss.params_lock))) - return -EAGAIN; - } else if (mutex_lock_interruptible(&runtime->oss.params_lock)) - return -ERESTARTSYS; + if (!runtime->oss.params) + return 0; sw_params = kzalloc(sizeof(*sw_params), GFP_KERNEL); params = kmalloc(sizeof(*params), GFP_KERNEL); sparams = kmalloc(sizeof(*sparams), GFP_KERNEL); @@ -1068,6 +1065,23 @@ failure: kfree(sw_params); kfree(params); kfree(sparams); + return err; +} + +/* this one takes the lock by itself */ +static int snd_pcm_oss_change_params(struct snd_pcm_substream *substream, + bool trylock) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + int err; + + if (trylock) { + if (!(mutex_trylock(&runtime->oss.params_lock))) + return -EAGAIN; + } else if (mutex_lock_interruptible(&runtime->oss.params_lock)) + return -ERESTARTSYS; + + err = snd_pcm_oss_change_params_locked(substream); mutex_unlock(&runtime->oss.params_lock); return err; } @@ -1096,11 +1110,14 @@ static int snd_pcm_oss_get_active_substream(struct snd_pcm_oss_file *pcm_oss_fil return 0; } +/* call with params_lock held */ static int snd_pcm_oss_prepare(struct snd_pcm_substream *substream) { int err; struct snd_pcm_runtime *runtime = substream->runtime; + if (!runtime->oss.prepare) + return 0; err = snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_PREPARE, NULL); if (err < 0) { pcm_dbg(substream->pcm, @@ -1120,14 +1137,35 @@ static int snd_pcm_oss_make_ready(struct snd_pcm_substream *substream) struct snd_pcm_runtime *runtime; int err; - if (substream == NULL) - return 0; runtime = substream->runtime; if (runtime->oss.params) { err = snd_pcm_oss_change_params(substream, false); if (err < 0) return err; } + if (runtime->oss.prepare) { + if (mutex_lock_interruptible(&runtime->oss.params_lock)) + return -ERESTARTSYS; + err = snd_pcm_oss_prepare(substream); + mutex_unlock(&runtime->oss.params_lock); + if (err < 0) + return err; + } + return 0; +} + +/* call with params_lock held */ +static int snd_pcm_oss_make_ready_locked(struct snd_pcm_substream *substream) +{ + struct snd_pcm_runtime *runtime; + int err; + + runtime = substream->runtime; + if (runtime->oss.params) { + err = snd_pcm_oss_change_params_locked(substream); + if (err < 0) + return err; + } if (runtime->oss.prepare) { err = snd_pcm_oss_prepare(substream); if (err < 0) @@ -1332,13 +1370,14 @@ static ssize_t snd_pcm_oss_write1(struct snd_pcm_substream *substream, const cha if (atomic_read(&substream->mmap_count)) return -ENXIO; - if ((tmp = snd_pcm_oss_make_ready(substream)) < 0) - return tmp; while (bytes > 0) { if (mutex_lock_interruptible(&runtime->oss.params_lock)) { tmp = -ERESTARTSYS; break; } + tmp = snd_pcm_oss_make_ready_locked(substream); + if (tmp < 0) + goto err; if (bytes < runtime->oss.period_bytes || runtime->oss.buffer_used > 0) { tmp = bytes; if (tmp + runtime->oss.buffer_used > runtime->oss.period_bytes) @@ -1439,13 +1478,14 @@ static ssize_t snd_pcm_oss_read1(struct snd_pcm_substream *substream, char __use if (atomic_read(&substream->mmap_count)) return -ENXIO; - if ((tmp = snd_pcm_oss_make_ready(substream)) < 0) - return tmp; while (bytes > 0) { if (mutex_lock_interruptible(&runtime->oss.params_lock)) { tmp = -ERESTARTSYS; break; } + tmp = snd_pcm_oss_make_ready_locked(substream); + if (tmp < 0) + goto err; if (bytes < runtime->oss.period_bytes || runtime->oss.buffer_used > 0) { if (runtime->oss.buffer_used == 0) { tmp = snd_pcm_oss_read2(substream, runtime->oss.buffer, runtime->oss.period_bytes, 1); @@ -1501,10 +1541,12 @@ static int snd_pcm_oss_reset(struct snd_pcm_oss_file *pcm_oss_file) continue; runtime = substream->runtime; snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_DROP, NULL); + mutex_lock(&runtime->oss.params_lock); runtime->oss.prepare = 1; runtime->oss.buffer_used = 0; runtime->oss.prev_hw_ptr_period = 0; runtime->oss.period_ptr = 0; + mutex_unlock(&runtime->oss.params_lock); } return 0; } @@ -1590,9 +1632,10 @@ static int snd_pcm_oss_sync(struct snd_pcm_oss_file *pcm_oss_file) goto __direct; if ((err = snd_pcm_oss_make_ready(substream)) < 0) return err; + if (mutex_lock_interruptible(&runtime->oss.params_lock)) + return -ERESTARTSYS; format = snd_pcm_oss_format_from(runtime->oss.format); width = snd_pcm_format_physical_width(format); - mutex_lock(&runtime->oss.params_lock); if (runtime->oss.buffer_used > 0) { #ifdef OSS_DEBUG pcm_dbg(substream->pcm, "sync: buffer_used\n"); @@ -1643,7 +1686,9 @@ static int snd_pcm_oss_sync(struct snd_pcm_oss_file *pcm_oss_file) substream->f_flags = saved_f_flags; if (err < 0) return err; + mutex_lock(&runtime->oss.params_lock); runtime->oss.prepare = 1; + mutex_unlock(&runtime->oss.params_lock); } substream = pcm_oss_file->streams[SNDRV_PCM_STREAM_CAPTURE]; @@ -1654,8 +1699,10 @@ static int snd_pcm_oss_sync(struct snd_pcm_oss_file *pcm_oss_file) err = snd_pcm_kernel_ioctl(substream, SNDRV_PCM_IOCTL_DROP, NULL); if (err < 0) return err; + mutex_lock(&runtime->oss.params_lock); runtime->oss.buffer_used = 0; runtime->oss.prepare = 1; + mutex_unlock(&runtime->oss.params_lock); } return 0; } @@ -1674,10 +1721,13 @@ static int snd_pcm_oss_set_rate(struct snd_pcm_oss_file *pcm_oss_file, int rate) rate = 1000; else if (rate > 192000) rate = 192000; + if (mutex_lock_interruptible(&runtime->oss.params_lock)) + return -ERESTARTSYS; if (runtime->oss.rate != rate) { runtime->oss.params = 1; runtime->oss.rate = rate; } + mutex_unlock(&runtime->oss.params_lock); } return snd_pcm_oss_get_rate(pcm_oss_file); } @@ -1705,10 +1755,13 @@ static int snd_pcm_oss_set_channels(struct snd_pcm_oss_file *pcm_oss_file, unsig if (substream == NULL) continue; runtime = substream->runtime; + if (mutex_lock_interruptible(&runtime->oss.params_lock)) + return -ERESTARTSYS; if (runtime->oss.channels != channels) { runtime->oss.params = 1; runtime->oss.channels = channels; } + mutex_unlock(&runtime->oss.params_lock); } return snd_pcm_oss_get_channels(pcm_oss_file); } @@ -1794,10 +1847,13 @@ static int snd_pcm_oss_set_format(struct snd_pcm_oss_file *pcm_oss_file, int for if (substream == NULL) continue; runtime = substream->runtime; + if (mutex_lock_interruptible(&runtime->oss.params_lock)) + return -ERESTARTSYS; if (runtime->oss.format != format) { runtime->oss.params = 1; runtime->oss.format = format; } + mutex_unlock(&runtime->oss.params_lock); } } return snd_pcm_oss_get_format(pcm_oss_file); @@ -1817,8 +1873,6 @@ static int snd_pcm_oss_set_subdivide1(struct snd_pcm_substream *substream, int s { struct snd_pcm_runtime *runtime; - if (substream == NULL) - return 0; runtime = substream->runtime; if (subdivide == 0) { subdivide = runtime->oss.subdivision; @@ -1842,9 +1896,16 @@ static int snd_pcm_oss_set_subdivide(struct snd_pcm_oss_file *pcm_oss_file, int for (idx = 1; idx >= 0; --idx) { struct snd_pcm_substream *substream = pcm_oss_file->streams[idx]; + struct snd_pcm_runtime *runtime; + if (substream == NULL) continue; - if ((err = snd_pcm_oss_set_subdivide1(substream, subdivide)) < 0) + runtime = substream->runtime; + if (mutex_lock_interruptible(&runtime->oss.params_lock)) + return -ERESTARTSYS; + err = snd_pcm_oss_set_subdivide1(substream, subdivide); + mutex_unlock(&runtime->oss.params_lock); + if (err < 0) return err; } return err; @@ -1854,8 +1915,6 @@ static int snd_pcm_oss_set_fragment1(struct snd_pcm_substream *substream, unsign { struct snd_pcm_runtime *runtime; - if (substream == NULL) - return 0; runtime = substream->runtime; if (runtime->oss.subdivision || runtime->oss.fragshift) return -EINVAL; @@ -1875,9 +1934,16 @@ static int snd_pcm_oss_set_fragment(struct snd_pcm_oss_file *pcm_oss_file, unsig for (idx = 1; idx >= 0; --idx) { struct snd_pcm_substream *substream = pcm_oss_file->streams[idx]; + struct snd_pcm_runtime *runtime; + if (substream == NULL) continue; - if ((err = snd_pcm_oss_set_fragment1(substream, val)) < 0) + runtime = substream->runtime; + if (mutex_lock_interruptible(&runtime->oss.params_lock)) + return -ERESTARTSYS; + err = snd_pcm_oss_set_fragment1(substream, val); + mutex_unlock(&runtime->oss.params_lock); + if (err < 0) return err; } return err; @@ -1961,6 +2027,9 @@ static int snd_pcm_oss_set_trigger(struct snd_pcm_oss_file *pcm_oss_file, int tr } if (psubstream) { runtime = psubstream->runtime; + cmd = 0; + if (mutex_lock_interruptible(&runtime->oss.params_lock)) + return -ERESTARTSYS; if (trigger & PCM_ENABLE_OUTPUT) { if (runtime->oss.trigger) goto _skip1; @@ -1978,13 +2047,19 @@ static int snd_pcm_oss_set_trigger(struct snd_pcm_oss_file *pcm_oss_file, int tr cmd = SNDRV_PCM_IOCTL_DROP; runtime->oss.prepare = 1; } - err = snd_pcm_kernel_ioctl(psubstream, cmd, NULL); - if (err < 0) - return err; - } _skip1: + mutex_unlock(&runtime->oss.params_lock); + if (cmd) { + err = snd_pcm_kernel_ioctl(psubstream, cmd, NULL); + if (err < 0) + return err; + } + } if (csubstream) { runtime = csubstream->runtime; + cmd = 0; + if (mutex_lock_interruptible(&runtime->oss.params_lock)) + return -ERESTARTSYS; if (trigger & PCM_ENABLE_INPUT) { if (runtime->oss.trigger) goto _skip2; @@ -1999,11 +2074,14 @@ static int snd_pcm_oss_set_trigger(struct snd_pcm_oss_file *pcm_oss_file, int tr cmd = SNDRV_PCM_IOCTL_DROP; runtime->oss.prepare = 1; } - err = snd_pcm_kernel_ioctl(csubstream, cmd, NULL); - if (err < 0) - return err; - } _skip2: + mutex_unlock(&runtime->oss.params_lock); + if (cmd) { + err = snd_pcm_kernel_ioctl(csubstream, cmd, NULL); + if (err < 0) + return err; + } + } return 0; } -- cgit v1.2.3 From 40cab6e88cb0b6c56d3f30b7491a20e803f948f6 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Fri, 23 Mar 2018 08:03:26 +0100 Subject: ALSA: pcm: Return -EBUSY for OSS ioctls changing busy streams OSS PCM stream management isn't modal but it allows ioctls issued at any time for changing the parameters. In the previous hardening patch ("ALSA: pcm: Avoid potential races between OSS ioctls and read/write"), we covered these races and prevent the corruption by protecting the concurrent accesses via params_lock mutex. However, this means that some ioctls that try to change the stream parameter (e.g. channels or format) would be blocked until the read/write finishes, and it may take really long. Basically changing the parameter while reading/writing is an invalid operation, hence it's even more user-friendly from the API POV if it returns -EBUSY in such a situation. This patch adds such checks in the relevant ioctls with the addition of read/write access refcount. Cc: Signed-off-by: Takashi Iwai --- include/sound/pcm_oss.h | 1 + sound/core/oss/pcm_oss.c | 36 +++++++++++++++++++++++++++--------- 2 files changed, 28 insertions(+), 9 deletions(-) (limited to 'sound/core') diff --git a/include/sound/pcm_oss.h b/include/sound/pcm_oss.h index 760c969d885d..12bbf8c81112 100644 --- a/include/sound/pcm_oss.h +++ b/include/sound/pcm_oss.h @@ -57,6 +57,7 @@ struct snd_pcm_oss_runtime { char *buffer; /* vmallocated period */ size_t buffer_used; /* used length from period buffer */ struct mutex params_lock; + atomic_t rw_ref; /* concurrent read/write accesses */ #ifdef CONFIG_SND_PCM_OSS_PLUGINS struct snd_pcm_plugin *plugin_first; struct snd_pcm_plugin *plugin_last; diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c index f8bfee8022e0..a9082f219561 100644 --- a/sound/core/oss/pcm_oss.c +++ b/sound/core/oss/pcm_oss.c @@ -1370,6 +1370,7 @@ static ssize_t snd_pcm_oss_write1(struct snd_pcm_substream *substream, const cha if (atomic_read(&substream->mmap_count)) return -ENXIO; + atomic_inc(&runtime->oss.rw_ref); while (bytes > 0) { if (mutex_lock_interruptible(&runtime->oss.params_lock)) { tmp = -ERESTARTSYS; @@ -1433,6 +1434,7 @@ static ssize_t snd_pcm_oss_write1(struct snd_pcm_substream *substream, const cha } tmp = 0; } + atomic_dec(&runtime->oss.rw_ref); return xfer > 0 ? (snd_pcm_sframes_t)xfer : tmp; } @@ -1478,6 +1480,7 @@ static ssize_t snd_pcm_oss_read1(struct snd_pcm_substream *substream, char __use if (atomic_read(&substream->mmap_count)) return -ENXIO; + atomic_inc(&runtime->oss.rw_ref); while (bytes > 0) { if (mutex_lock_interruptible(&runtime->oss.params_lock)) { tmp = -ERESTARTSYS; @@ -1526,6 +1529,7 @@ static ssize_t snd_pcm_oss_read1(struct snd_pcm_substream *substream, char __use } tmp = 0; } + atomic_dec(&runtime->oss.rw_ref); return xfer > 0 ? (snd_pcm_sframes_t)xfer : tmp; } @@ -1632,8 +1636,11 @@ static int snd_pcm_oss_sync(struct snd_pcm_oss_file *pcm_oss_file) goto __direct; if ((err = snd_pcm_oss_make_ready(substream)) < 0) return err; - if (mutex_lock_interruptible(&runtime->oss.params_lock)) + atomic_inc(&runtime->oss.rw_ref); + if (mutex_lock_interruptible(&runtime->oss.params_lock)) { + atomic_dec(&runtime->oss.rw_ref); return -ERESTARTSYS; + } format = snd_pcm_oss_format_from(runtime->oss.format); width = snd_pcm_format_physical_width(format); if (runtime->oss.buffer_used > 0) { @@ -1645,10 +1652,8 @@ static int snd_pcm_oss_sync(struct snd_pcm_oss_file *pcm_oss_file) runtime->oss.buffer + runtime->oss.buffer_used, size); err = snd_pcm_oss_sync1(substream, runtime->oss.period_bytes); - if (err < 0) { - mutex_unlock(&runtime->oss.params_lock); - return err; - } + if (err < 0) + goto unlock; } else if (runtime->oss.period_ptr > 0) { #ifdef OSS_DEBUG pcm_dbg(substream->pcm, "sync: period_ptr\n"); @@ -1658,10 +1663,8 @@ static int snd_pcm_oss_sync(struct snd_pcm_oss_file *pcm_oss_file) runtime->oss.buffer, size * 8 / width); err = snd_pcm_oss_sync1(substream, size); - if (err < 0) { - mutex_unlock(&runtime->oss.params_lock); - return err; - } + if (err < 0) + goto unlock; } /* * The ALSA's period might be a bit large than OSS one. @@ -1675,7 +1678,11 @@ static int snd_pcm_oss_sync(struct snd_pcm_oss_file *pcm_oss_file) else if (runtime->access == SNDRV_PCM_ACCESS_RW_NONINTERLEAVED) snd_pcm_lib_writev(substream, NULL, size); } +unlock: mutex_unlock(&runtime->oss.params_lock); + atomic_dec(&runtime->oss.rw_ref); + if (err < 0) + return err; /* * finish sync: drain the buffer */ @@ -1723,6 +1730,8 @@ static int snd_pcm_oss_set_rate(struct snd_pcm_oss_file *pcm_oss_file, int rate) rate = 192000; if (mutex_lock_interruptible(&runtime->oss.params_lock)) return -ERESTARTSYS; + if (atomic_read(&runtime->oss.rw_ref)) + return -EBUSY; if (runtime->oss.rate != rate) { runtime->oss.params = 1; runtime->oss.rate = rate; @@ -1757,6 +1766,8 @@ static int snd_pcm_oss_set_channels(struct snd_pcm_oss_file *pcm_oss_file, unsig runtime = substream->runtime; if (mutex_lock_interruptible(&runtime->oss.params_lock)) return -ERESTARTSYS; + if (atomic_read(&runtime->oss.rw_ref)) + return -EBUSY; if (runtime->oss.channels != channels) { runtime->oss.params = 1; runtime->oss.channels = channels; @@ -1847,6 +1858,8 @@ static int snd_pcm_oss_set_format(struct snd_pcm_oss_file *pcm_oss_file, int for if (substream == NULL) continue; runtime = substream->runtime; + if (atomic_read(&runtime->oss.rw_ref)) + return -EBUSY; if (mutex_lock_interruptible(&runtime->oss.params_lock)) return -ERESTARTSYS; if (runtime->oss.format != format) { @@ -1901,6 +1914,8 @@ static int snd_pcm_oss_set_subdivide(struct snd_pcm_oss_file *pcm_oss_file, int if (substream == NULL) continue; runtime = substream->runtime; + if (atomic_read(&runtime->oss.rw_ref)) + return -EBUSY; if (mutex_lock_interruptible(&runtime->oss.params_lock)) return -ERESTARTSYS; err = snd_pcm_oss_set_subdivide1(substream, subdivide); @@ -1939,6 +1954,8 @@ static int snd_pcm_oss_set_fragment(struct snd_pcm_oss_file *pcm_oss_file, unsig if (substream == NULL) continue; runtime = substream->runtime; + if (atomic_read(&runtime->oss.rw_ref)) + return -EBUSY; if (mutex_lock_interruptible(&runtime->oss.params_lock)) return -ERESTARTSYS; err = snd_pcm_oss_set_fragment1(substream, val); @@ -2333,6 +2350,7 @@ static void snd_pcm_oss_init_substream(struct snd_pcm_substream *substream, runtime->oss.maxfrags = 0; runtime->oss.subdivision = 0; substream->pcm_release = snd_pcm_oss_release_substream; + atomic_set(&runtime->oss.rw_ref, 0); } static int snd_pcm_oss_release_file(struct snd_pcm_oss_file *pcm_oss_file) -- cgit v1.2.3 From f6d297df4dd47ef949540e4a201230d0c5308325 Mon Sep 17 00:00:00 2001 From: Takashi Iwai Date: Tue, 27 Mar 2018 14:32:23 +0200 Subject: ALSA: pcm: Fix mutex unbalance in OSS emulation ioctls The previous fix 40cab6e88cb0 ("ALSA: pcm: Return -EBUSY for OSS ioctls changing busy streams") introduced some mutex unbalance; the check of runtime->oss.rw_ref was inserted in a wrong place after the mutex lock. This patch fixes the inconsistency by rewriting with the helper functions to lock/unlock parameters with the stream check. Fixes: 40cab6e88cb0 ("ALSA: pcm: Return -EBUSY for OSS ioctls changing busy streams") Reported-by: Dan Carpenter Cc: Signed-off-by: Takashi Iwai --- sound/core/oss/pcm_oss.c | 67 ++++++++++++++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 25 deletions(-) (limited to 'sound/core') diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c index a9082f219561..f19b4586e33b 100644 --- a/sound/core/oss/pcm_oss.c +++ b/sound/core/oss/pcm_oss.c @@ -823,6 +823,23 @@ static int choose_rate(struct snd_pcm_substream *substream, return snd_pcm_hw_param_near(substream, params, SNDRV_PCM_HW_PARAM_RATE, best_rate, NULL); } +/* parameter locking: returns immediately if tried during streaming */ +static int lock_params(struct snd_pcm_runtime *runtime) +{ + if (mutex_lock_interruptible(&runtime->oss.params_lock)) + return -ERESTARTSYS; + if (atomic_read(&runtime->oss.rw_ref)) { + mutex_unlock(&runtime->oss.params_lock); + return -EBUSY; + } + return 0; +} + +static void unlock_params(struct snd_pcm_runtime *runtime) +{ + mutex_unlock(&runtime->oss.params_lock); +} + /* call with params_lock held */ static int snd_pcm_oss_change_params_locked(struct snd_pcm_substream *substream) { @@ -1721,6 +1738,8 @@ static int snd_pcm_oss_set_rate(struct snd_pcm_oss_file *pcm_oss_file, int rate) for (idx = 1; idx >= 0; --idx) { struct snd_pcm_substream *substream = pcm_oss_file->streams[idx]; struct snd_pcm_runtime *runtime; + int err; + if (substream == NULL) continue; runtime = substream->runtime; @@ -1728,15 +1747,14 @@ static int snd_pcm_oss_set_rate(struct snd_pcm_oss_file *pcm_oss_file, int rate) rate = 1000; else if (rate > 192000) rate = 192000; - if (mutex_lock_interruptible(&runtime->oss.params_lock)) - return -ERESTARTSYS; - if (atomic_read(&runtime->oss.rw_ref)) - return -EBUSY; + err = lock_params(runtime); + if (err < 0) + return err; if (runtime->oss.rate != rate) { runtime->oss.params = 1; runtime->oss.rate = rate; } - mutex_unlock(&runtime->oss.params_lock); + unlock_params(runtime); } return snd_pcm_oss_get_rate(pcm_oss_file); } @@ -1761,18 +1779,19 @@ static int snd_pcm_oss_set_channels(struct snd_pcm_oss_file *pcm_oss_file, unsig for (idx = 1; idx >= 0; --idx) { struct snd_pcm_substream *substream = pcm_oss_file->streams[idx]; struct snd_pcm_runtime *runtime; + int err; + if (substream == NULL) continue; runtime = substream->runtime; - if (mutex_lock_interruptible(&runtime->oss.params_lock)) - return -ERESTARTSYS; - if (atomic_read(&runtime->oss.rw_ref)) - return -EBUSY; + err = lock_params(runtime); + if (err < 0) + return err; if (runtime->oss.channels != channels) { runtime->oss.params = 1; runtime->oss.channels = channels; } - mutex_unlock(&runtime->oss.params_lock); + unlock_params(runtime); } return snd_pcm_oss_get_channels(pcm_oss_file); } @@ -1845,6 +1864,7 @@ static int snd_pcm_oss_get_formats(struct snd_pcm_oss_file *pcm_oss_file) static int snd_pcm_oss_set_format(struct snd_pcm_oss_file *pcm_oss_file, int format) { int formats, idx; + int err; if (format != AFMT_QUERY) { formats = snd_pcm_oss_get_formats(pcm_oss_file); @@ -1858,15 +1878,14 @@ static int snd_pcm_oss_set_format(struct snd_pcm_oss_file *pcm_oss_file, int for if (substream == NULL) continue; runtime = substream->runtime; - if (atomic_read(&runtime->oss.rw_ref)) - return -EBUSY; - if (mutex_lock_interruptible(&runtime->oss.params_lock)) - return -ERESTARTSYS; + err = lock_params(runtime); + if (err < 0) + return err; if (runtime->oss.format != format) { runtime->oss.params = 1; runtime->oss.format = format; } - mutex_unlock(&runtime->oss.params_lock); + unlock_params(runtime); } } return snd_pcm_oss_get_format(pcm_oss_file); @@ -1914,12 +1933,11 @@ static int snd_pcm_oss_set_subdivide(struct snd_pcm_oss_file *pcm_oss_file, int if (substream == NULL) continue; runtime = substream->runtime; - if (atomic_read(&runtime->oss.rw_ref)) - return -EBUSY; - if (mutex_lock_interruptible(&runtime->oss.params_lock)) - return -ERESTARTSYS; + err = lock_params(runtime); + if (err < 0) + return err; err = snd_pcm_oss_set_subdivide1(substream, subdivide); - mutex_unlock(&runtime->oss.params_lock); + unlock_params(runtime); if (err < 0) return err; } @@ -1954,12 +1972,11 @@ static int snd_pcm_oss_set_fragment(struct snd_pcm_oss_file *pcm_oss_file, unsig if (substream == NULL) continue; runtime = substream->runtime; - if (atomic_read(&runtime->oss.rw_ref)) - return -EBUSY; - if (mutex_lock_interruptible(&runtime->oss.params_lock)) - return -ERESTARTSYS; + err = lock_params(runtime); + if (err < 0) + return err; err = snd_pcm_oss_set_fragment1(substream, val); - mutex_unlock(&runtime->oss.params_lock); + unlock_params(runtime); if (err < 0) return err; } -- cgit v1.2.3