Re: [PATCH v3 14/17] ASoC: simple-amplifier: gpio-audio-amp: Add support for gain-ranges

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unsynchronized freeing of dynamically allocated TLV array causes a Use-After-Free during concurrent ALSA TLV read.
- [Medium] Integer overflow in `kctrl_max` accumulation leads to validation bypass and a reachable `WARN(1)`.
- [Medium] Integer overflow in `simple_amp_cmp_ranges` can corrupt the sorted array.
- [Medium] Unprotected state modifications in ALSA control `put` handlers lead to a TOCTOU race and hardware state desynchronization.
--

commit 24a3721afe8e210f60da48c3439973f4c9f7e094
Author: Herve Codina <herve.codina@bootlin.com>

ASoC: simple-amplifier: gpio-audio-amp: Add support for gain-ranges

This commit introduces support for gain-ranges in the simple-amplifier
driver, mapping physical gain values to GPIO values through an array of
ranges and exporting them via an ALSA kcontrol with TLV range information.

diff --git a/sound/soc/codecs/simple-amplifier.c b/sound/soc/codecs/simple-amplifier.c
index 5b172e520dcd0..5759f9bc2f4ff 100644
--- a/sound/soc/codecs/simple-amplifier.c
+++ b/sound/soc/codecs/simple-amplifier.c

[ ... ]

> @@ -252,6 +312,38 @@ static int simple_amp_multi_kctrl_int_put(struct snd_kcontrol *kcontrol,
>  	return 1; /* The value changed */
>  }

Can concurrent calls to simple_amp_multi_kctrl_int_put() and
simple_amp_single_kctrl_put() lead to hardware state desynchronization?

These handlers read the cached software state, issue hardware I/O via
gpiod_multi_set_value_cansleep(), and update the cached state without
driver-level locking.

The OSS emulation layer only holds a read lock when invoking kctl->put().
Two racing threads could pass the equality check and interleave their
hardware writes, causing the cached software state to permanently
desynchronize from the actual hardware GPIO state.

[ ... ]

> @@ -407,8 +526,17 @@ static int simple_amp_component_probe(struct snd_soc_component *component)
>  	return 0;
>  }
>  
> +static void simple_amp_component_remove(struct snd_soc_component *component)
> +{
> +	struct simple_amp *simple_amp = snd_soc_component_get_drvdata(component);
> +
> +	kfree(simple_amp->gain.tlv_array);
> +	simple_amp->gain.tlv_array = NULL;
> +}

Does this kfree() cause a use-after-free during concurrent ALSA TLV reads?

ALSA controls remain accessible as long as a user holds an open file
descriptor, and snd_card_disconnect() does not drain active ioctls. A
concurrent SNDRV_CTL_IOCTL_TLV_READ could be blocked in copy_to_user()
reading control.tlv.p when this memory is synchronously freed.

Should the array's lifetime instead be bound to the kcontrol via the
kcontrol->private_free callback?

[ ... ]

> @@ -426,10 +554,179 @@ static int simple_amp_parse_single_gpio(struct device *dev,
>  	return 0;
>  }
>  
> +static int simple_amp_cmp_ranges(const void *a, const void *b)
> +{
> +	const struct simple_amp_range *a_range = a;
> +	const struct simple_amp_range *b_range = b;
> +
> +	/* Ranges a and b don't overlap. This has been already checked */
> +
> +	return a_range->min.gain_db - b_range->max.gain_db;
> +}

Can this subtraction mathematically overflow?

If highly negative and highly positive s32 values are provided via the
Device Tree, this operation could reverse the intended sign and break the
strict weak ordering required by sort(), corrupting the resulting array state.

Could this use safe inequality checks instead of subtraction?

[ ... ]

> +static int simple_amp_parse_ranges(struct device *dev,
> +				   struct simple_amp_multi *multi,
> +				   const char *ranges_property)
> +{

[ ... ]

> +		range->nb_points = abs_diff(range->min.gpio_val,
> +					    range->max.gpio_val) + 1;
> +
> +		multi->kctrl_max += range->nb_points;
> +	}
> +
> +	multi->kctrl_max -= 1;

Does this code overflow multi->kctrl_max?

A malformed Device Tree could define up to 65,536 ranges, each mapping to
65,536 points. This summation would reach exactly 4,294,967,296, completely
overflowing the u32 back to 0.

The subsequent multi->kctrl_max -= 1 operation would then underflow it to
0xFFFFFFFF, bypassing the bounds check in
simple_amp_multi_kctrl_write_gpios() and triggering a reachable WARN(1) in
simple_amp_multi_ranges_kctrl_to_gpio() which could act as a denial of
service if panic_on_warn is set.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513081702.317117-1-herve.codina@bootlin.com?part=14