Re: [PATCH v8 2/8] ALSA: usb: babyfacepro: add output masters and crosspoint routing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ALSA: usb: babyfacepro: out-of-bounds value[1] write in bf_mute_get()
- [Medium] ALSA: usb: babyfacepro: missing shutdown check in mixer put callbacks
--

commit a0ae4df204731e5d864689d9a4a9ee415579f1a2
Author: Ismaïl Bahloul <i.bahloul01@gmail.com>

ALSA: usb: babyfacepro: add output masters and crosspoint routing

This commit adds ALSA mixer controls for the six output masters (volume and
mute) and the 6x14 crosspoint routing matrix, caching their state on the
host and initializing safe power-on defaults during probe.

> diff --git a/sound/usb/babyfacepro/babyfacepro-ctl.c b/sound/usb/babyfacepro/babyfacepro-ctl.c
> new file mode 100644
> index 0000000000000..942d4732cbe07
> --- /dev/null
> +++ b/sound/usb/babyfacepro/babyfacepro-ctl.c

[ ... ]

> +static int bf_master_put(struct snd_kcontrol *kctl,
> +			 struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_usb_babyface *chip = snd_kcontrol_chip(kctl);
> +	int out = bf_master_out[kctl->private_value];
> +	long nl = ucontrol->value.integer.value[0];
> +	long nr = ucontrol->value.integer.value[1];
> +	u16 l, r, wire_l, wire_r, flag;
> +	int ret = 0;
> +
> +	/* Reject anything outside the declared range, so the registers and
> +	 * the cache stay in spec (the ALSA core only enforces this with
> +	 * CONFIG_SND_CTL_INPUT_VALIDATION).  Check before narrowing:
> +	 * .value.integer.value[] is a long.
> +	 */
> +	if (nl < 0 || nl > BF_MASTER_STEP_MAX ||
> +	    nr < 0 || nr > BF_MASTER_STEP_MAX)
> +		return -EINVAL;
> +	l = nl;
> +	r = nr;
> +
> +	guard(mutex)(&chip->mutex);
> +	if (l == chip->master[out][0] && r == chip->master[out][1])
> +		return 0;

[Severity: Medium]
As reported by Sashiko on v7 [1], should bf_master_put() (along with
bf_mute_put() and bf_xpoint_put() below) check chip->shutdown and return
-ENODEV after acquiring chip->mutex, matching babyface_pcm_hw_params(),
babyface_pcm_prepare(), and babyface_stream_work()?

In babyface_disconnect(), chip->shutdown is set to true and chip->mutex is
acquired and released to fence in-flight operations before calling
usb_enable_autosuspend(chip->dev) and snd_card_disconnect(chip->card):

sound/usb/babyfacepro/babyfacepro.c:babyface_disconnect() {
    ...
	chip->shutdown = true;
	scoped_guard(mutex, &chip->mutex) {
		if (chip->streaming)
			babyface_stream_kill(chip);
	}
	cancel_work_sync(&chip->stream_work);
	usb_enable_autosuspend(chip->dev);
    ...
	snd_card_disconnect(chip->card);
}

Without checking chip->shutdown under chip->mutex, a concurrent
SNDRV_CTL_IOCTL_ELEM_WRITE ioctl in bf_master_put(), bf_mute_put(), or
bf_xpoint_put() can proceed to issue synchronous USB control transfers via
bf_vendor_write() or bf_xpoint_write() after the disconnect fence has passed
and USB autosuspend has been re-enabled (and on !CONFIG_PM builds, where
snd_power_sync_ref() in snd_card_disconnect() is a no-op, while
babyface_disconnect() is completing).

> +
> +	wire_l = chip->muted[out] ? 0 : l;
> +	wire_r = chip->muted[out] ? 0 : r;
> +	flag = bf_flag_cycle[chip->flag_cnt];
> +	chip->flag_cnt = (chip->flag_cnt + 1) & 3;
> +
> +	/* Both registers, as TotalMix writes them: the 8-bit is the analog
> +	 * level, the 16-bit the digital one.
> +	 */
> +	ret = bf_vendor_write(chip, BF_REQ_GAIN, bf_master_reg8(wire_l),
> +			      BF_REG_MASTER_8 + 2 * out);
> +	if (ret < 0)
> +		return ret;
> +	ret = bf_vendor_write(chip, BF_REQ_GAIN, bf_master_reg8(wire_r),
> +			      BF_REG_MASTER_8 + 2 * out + 1);
> +	if (ret < 0)
> +		return ret;
> +	ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, bf_master_reg16(wire_l),
> +			      (BF_REG_MASTER_16 + 2 * out) | flag);
> +	if (ret < 0)
> +		return ret;
> +	ret = bf_vendor_write(chip, BF_REQ_CROSSPOINT, bf_master_reg16(wire_r),
> +			      (BF_REG_MASTER_16 + 2 * out + 1) | flag);
> +	if (ret < 0)
> +		return ret;
> +
> +	chip->master[out][0] = l;
> +	chip->master[out][1] = r;
> +	return 1;
> +}
> +
> +static int bf_mute_info(struct snd_kcontrol *kctl,
> +			struct snd_ctl_elem_info *uinfo)
> +{
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN;
> +	/* One switch for the pair: bf_mute_put() writes both channels
> +	 * together, so a count of 2 would silently ignore value[1].
> +	 */
> +	uinfo->count = 1;
> +	uinfo->value.integer.min = 0;
> +	uinfo->value.integer.max = 1;
> +	return 0;
> +}
> +
> +static int bf_mute_get(struct snd_kcontrol *kctl,
> +		       struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_usb_babyface *chip = snd_kcontrol_chip(kctl);
> +	int out = bf_master_out[kctl->private_value];
> +
> +	/* ALSA convention: 1 = enabled (sound on) = not muted. */
> +	ucontrol->value.integer.value[0] = !chip->muted[out];
> +	ucontrol->value.integer.value[1] = !chip->muted[out];
> +	return 0;
> +}

[Severity: High]
Since bf_mute_info() declares uinfo->count = 1, doesn't writing to
ucontrol->value.integer.value[1] in bf_mute_get() write past the declared
element count?

When CONFIG_SND_CTL_DEBUG is enabled, snd_ctl_elem_read() poisons bytes in
ucontrol->value past info.count and checks them in sanity_check_elem_value()
after kctl->get(), so overwriting ucontrol->value.integer.value[1] causes
every read of the Playback Switch controls to log an error and fail with
-EINVAL, and causes snd_ctl_put_verify() to report a false mismatch whenever
the output is unmuted. Even without CONFIG_SND_CTL_DEBUG, setting value[1]
breaks userspace comparisons via snd_ctl_elem_value_compare() (such as the
ALSA mixer-test kselftest).

[1] https://lore.kernel.org/r/20261027113140.134257-3-i.bahloul01@gmail.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105151553.223722-1-i.bahloul01@gmail.com?part=2