Re: [PATCH v5 2/2] ASoC: codecs: nau8360: Add support for NAU83G60 amplifier

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Asynchronous firmware loading via request_firmware_nowait() causes a Use-After-Free when the device is unbound.
- [High] Missing NULL check on strstr() result causes a kernel panic if the ALSA control name is truncated by an ASoC prefix.
- [Medium] device_property_read_string() always returns the first array element, causing all DSP cores to use identical firmware.
- [Medium] Mathematical invalidity of TDM TX byte offsets for PDM Loopback when changing channel length.
- [Medium] Concurrent hardware state corruption due to missing locking around DSP protocol communication during system resume.
- [Medium] Race condition between nau8360_anc_put() and nau8360_dac_mux_put_enum() leads to inconsistent PEQ band configuration.
- [Low] The DSP KCS setup result is retrieved from hardware but its value is completely ignored, masking configuration failures.
--

commit 90fa475e6f47a06dabf426d4caf28ef96fe5ed05
Author: Neo Chang <YLCHANG2@nuvoton.com>

ASoC: codecs: nau8360: Add support for NAU83G60 amplifier

Add support for the Nuvoton NAU83G60 audio codec. The NAU83G60 is a
stereo 30W+30W smart amplifier with an integrated low-latency
Advanced Audio DSP.

> diff --git a/sound/soc/codecs/nau8360-dsp.c b/sound/soc/codecs/nau8360-dsp.c
> index 0000000000000..d0483805607d4 100644
> --- /dev/null
> +++ b/sound/soc/codecs/nau8360-dsp.c
[ ... ]
> @@ -310,6 +310,23 @@ static void nau8360_dsp_fw_cb(const struct firmware *fw, void *context)
>  {
>  	struct nau8360_dsp_ctx *dsp_ctx = context;
>  	struct snd_soc_component *cp = dsp_ctx->cp;
>  	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(cp);

[Severity: High]
Can this sequence allow a use-after-free if the device is unbound?

If a user unbinds the I2C device (e.g., via sysfs) while the background
firmware worker is still pending, the nau8360 struct and the component are
freed via devres. When the firmware callback eventually runs, it attempts to
dereference the freed cp pointer and acquires a lock in the freed nau8360
struct.

It looks like the I2C driver lacks a .remove callback, meaning there is no
synchronization point to wait for or cancel the asynchronous firmware worker
before device resources are reclaimed.

[ ... ]
> @@ -341,6 +358,10 @@ static int nau8360_dsp_chan_kcs_setup(struct snd_soc_component *cp,
>  	struct device *dev = cp->dev;
>  	int ret, status = 0;
>  
>  	ret = nau8360_dsp_get_cmd_put(cp, dsp_addr,
>  			NAU8360_DSP_CMD_GET_FRAME_STATUS, &status);

[Severity: Medium]
Could this unlocked I2C command to the DSP cause hardware state corruption
during system resume?

During system resume, the PM thread resets the DSP and calls
nau8360_dsp_chan_kcs_setup(), which interacts with the DSP via
nau8360_dsp_get_cmd_put(). The PM thread executes these commands without
holding nau8360->lock.

If a background firmware loading worker from a previous request was
suspended and resumes simultaneously (or is running on another CPU), its
locked DSP communications will interleave with the unlocked ones from the
PM thread, destroying the expected DSP I2C fragment protocol.

[ ... ]
> @@ -481,6 +502,10 @@ static int nau8360_dsp_kcs_setup(struct snd_soc_component *cp, int offset, int s
>  		addr_offset += data_len;
>  		data_rem -= data_len;
>  
>  		/* checking KCS result */
>  		ret = nau8360_dsp_exec_command(cp, NAU8360_DSP_CMD_GET_KCS_RSLTS,
>  			0, NAU8360_DSP_DATA_BYTE, NULL,
>  			NAU8360_DSP_DATA_BYTE, &kcs_rst, dsp_addr);
>  		if (ret)
>  			return ret;

[Severity: Low]
Is the DSP KCS setup result retrieved from hardware but its value completely
ignored?

While the I2C transaction success is verified via the return value, the
actual hardware response stored in kcs_rst is never checked. Does this
silently mask algorithmic configuration failures in the DSP?

> diff --git a/sound/soc/codecs/nau8360.c b/sound/soc/codecs/nau8360.c
> index 0000000000000..f4dc351a4030c 100644
> --- /dev/null
> +++ b/sound/soc/codecs/nau8360.c
[ ... ]
> @@ -288,6 +288,14 @@ static int nau8360_anc_put(struct snd_kcontrol *kcontrol,
>  	if (ret != 1)
>  		return ret;
>  
>  	nau8360->anc_enable = ucontrol->value.integer.value[0];
>  	if (nau8360_dsp_active(cp))
>  		value = nau8360->anc_enable ? NAU8360_PEQ_BAND_15 : NAU8360_PEQ_BAND_12;
>  	regmap_update_bits(nau8360->regmap, NAU8360_R9D_PEQ_CTL, NAU8360_PEQ_BAND_MASK,
>  		value << NAU8360_PEQ_BAND_SFT);

[Severity: Medium]
Could a Time-Of-Check to Time-Of-Use race condition occur between
nau8360_anc_put() and nau8360_dac_mux_put_enum() that leads to an
inconsistent PEQ band configuration?

nau8360_anc_put() modifies nau8360->anc_enable and reads
nau8360_dsp_active() without holding a driver-specific lock.
Concurrently, nau8360_dac_mux_put_enum() updates the DAC path
(changing the result of nau8360_dsp_active()) and calls
nau8360_dsp_switch(), which reads nau8360->anc_enable locklessly.

Both execute under ALSA's controls_rwsem read lock, allowing concurrent
execution by two userspace threads. nau8360_anc_put() might calculate a target
register value based on the DSP being active, but before it writes to the
hardware, nau8360_dac_mux_put_enum() could disable the DSP. This leaves the
NAU8360_R9D_PEQ_CTL register with a value appropriate for an enabled DSP,
even though the DSP path is disabled.

[ ... ]
> @@ -375,6 +383,9 @@ static inline int nau8360_peq_regaddr(const char *id_name)
>  	int reg, band_num, dsp_addr = NAU8360_DSP_ADDR_BYNAME(id_name);
>  	char *band = strstr(id_name, "BIQ");
>  
>  	if (kstrtoint((band + 3), 10, &band_num))

[Severity: High]
Can strstr() return NULL here and cause a kernel panic when its result is
dereferenced?

The ASoC subsystem permits machine drivers to apply a sound-name-prefix from
the device tree to control names. If a long prefix causes the control name to
exceed the ALSA maximum length of 44 characters, the string will be truncated.

If truncation removes the "BIQ" substring, strstr() returns NULL. The code
then unconditionally executes pointer arithmetic (band + 3) and dereferences it,
which crashes the kernel when unprivileged userspace accesses the affected PEQ
control.

[ ... ]
> @@ -439,6 +450,9 @@ static int nau8360_pdm_slot_get(struct snd_kcontrol *kcontrol,
>  	struct snd_soc_component *cp = snd_kcontrol_chip(kcontrol);
>  	struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
>  	int scale, slot = snd_soc_component_read(cp, e->reg);
>  
>  	compute_slotx_scale(cp, &scale);
>  	slot = ((slot >> e->shift_l) & e->mask) / scale;

[Severity: Medium]
Will this return a mutated, invalid slot index to userspace if the TDM channel
length changes dynamically?

Because the PDM Loopback TX slots (PDM_L and PDM_R) are handled independently
and do not exist in the tdm_tx_func_slot cache array, nau8360_tdm_apply()
fails to update their hardware offsets. When nau8360_pdm_slot_get() divides
the stale byte offset by the new channel scale, it calculates an invalid value.

[ ... ]
> @@ -660,6 +674,13 @@ static void nau8360_tdm_apply(struct snd_soc_component *cp, int slot_scale)
>  	struct nau8360 *nau8360 = snd_soc_component_get_drvdata(cp);
>  	int i, chan_tx;
>  
>  	for (i = 0; i < NAU8360_TDM_TXN; i++) {
>  		if (nau8360->tdm_tx_func_slot[i] == TDM_SLOT_NONE)
>  			continue;
>  
>  		/* compute the slot location in bytes according to slot/chan width */
>  		chan_tx = slot_scale * nau8360->tdm_tx_func_slot[i];
>  		nau8360_set_tdm_tx_slot(cp, i, chan_tx);
>  	}

[Severity: Medium]
Does this code incorrectly ignore PDM slots when recalculating byte offsets
for TDM TX slots?

The loop iterates over standard TX slots but omits PDM slots because they are
absent from the iteration. The PDM slots are left with stale byte offsets that
no longer align with the new width boundaries, which corrupts the TDM stream.

[ ... ]
> @@ -2404,6 +2425,10 @@ static int nau8360_read_device_properties(struct nau8360 *nau8360)
>  		return -EINVAL;
>  	}
>  
>  	for (i = 0; i < NAU8360_DSP_FW_NUM; i++) {
>  		ret = device_property_read_string(dev, "firmware-name",
>  			&firmware);

[Severity: Medium]
Does device_property_read_string() always return the first array element
in this loop?

During device initialization, the loop uses device_property_read_string()
instead of device_property_read_string_array() to extract strings from an
array property. Since it simply fetches the first string of the array
regardless of the loop index, all iterations process the exact same firmware
name, causing both DSP cores to be loaded with the firmware intended only
for the first core.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630021510.821919-1-YLCHANG2@nuvoton.com?part=2