Re: [PATCH v8 4/8] ALSA: usb: babyfacepro: add routing flags and varispeed pitch

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ALSA: usb: babyfacepro: out-of-bounds value[1] write in bf_loopback_get()
- [Low] ALSA: usb: babyfacepro: leftover contradictory comment above bf_out_reg8()
--

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

ALSA: usb: babyfacepro: add routing flags and varispeed pitch

This commit adds ALSA mixer controls and state persistence for routing flags
(loopback, AN 1>2, sample clock source, optical out format, AN1/2 link, MS
processor, DIM, width, and FX send) and varispeed pitch on the RME Babyface
Pro.

> diff --git a/sound/usb/babyfacepro/babyfacepro-ctl.c b/sound/usb/babyfacepro/babyfacepro-ctl.c
> index fe6266ea118e6..5482b6f09b73a 100644
> --- a/sound/usb/babyfacepro/babyfacepro-ctl.c
> +++ b/sound/usb/babyfacepro/babyfacepro-ctl.c

[ ... ]

> @@ -325,6 +329,33 @@ static int bf_master_get(struct snd_kcontrol *kctl,
>  	return 0;
>  }
>  
> +/* The master step to put on the wire.  The cache keeps the real
> + * volume: mute wins, and DIM is the fixed -20 dB step on the Phones
> + * pair, so every writer derives the wire value the same way.
> + */
> +/* The two master registers to put on the wire.  The cache keeps the
> + * real volume: mute wins, and DIM is the exact -20 dB pair the
> + * hardware's own button writes (0xcb / 0x0333, cap_dim2.pcap), not the
> + * master curve's value at that step.
> + */
> +static u8 bf_out_reg8(bool muted, bool dim, u16 step)

[Severity: Low]
This isn't a bug, but is the first comment block above bf_out_reg8() in
sound/usb/babyfacepro/babyfacepro-ctl.c a leftover from an earlier draft
that was meant to be replaced by the second comment block?

[ ... ]

> @@ -1278,3 +1305,679 @@ int babyface_create_preamp(struct snd_usb_babyface *chip)

[ ... ]

> +static int bf_loopback_get(struct snd_kcontrol *kctl,
> +			   struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_usb_babyface *chip = snd_kcontrol_chip(kctl);
> +	int out = kctl->private_value;
> +
> +	ucontrol->value.integer.value[0] = chip->loopback[out];
> +	ucontrol->value.integer.value[1] = chip->loopback[out];
> +	return 0;
> +}

[Severity: Medium]
Does bf_loopback_get() in sound/usb/babyfacepro/babyfacepro-ctl.c write
past the declared element count when populating
ucontrol->value.integer.value[1]?

In babyface_create_flags(), the Loopback Switch controls are registered with
.info = bf_mute_info and .get = bf_loopback_get:

	for (i = 0; i < 6; i++) {
		kctl = snd_ctl_new1(&(struct snd_kcontrol_new){
			.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
			.name = "Loopback Switch",
			.index = i,
			.info = bf_mute_info,
			.get = bf_loopback_get,
			.put = bf_loopback_put,
			.private_value = i,
		}, chip);

However, bf_mute_info() sets uinfo->count = 1:

	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;

When userspace reads the control via snd_ctl_elem_read() -> bf_loopback_get()
with CONFIG_SND_CTL_DEBUG enabled, the ALSA control core poisons the buffer
past info.count with fill_remaining_elem_value() and then checks it in
sanity_check_elem_value(). Because bf_loopback_get() (like bf_mute_get())
writes ucontrol->value.integer.value[1] on a count = 1 control,
sanity_check_elem_value() logs an access overflow error and fails the read
with -EINVAL.

Should bf_loopback_get() only write ucontrol->value.integer.value[0] (or use
bf_switch_info and a single-value get, and similarly in bf_mute_get())?

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