Re: [PATCH v2 2/3] ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Uninitialized stack memory is copied to userspace when regmap_read() fails in sun4i_spdif_get_status().
- [High] IEC958 status bits configured via ALSA kcontrols are unconditionally overwritten by hw_params.
--

commit 564921e7b2898d3b9cb384871d5d927721ed7c42
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: sunxi: sun4i-spdif: Resume device before kcontrol register access

Accessing registers while the device is runtime-suspended may lead to invalid
hardware accesses. Ensure the device is resumed before accessing registers.

> diff --git a/sound/soc/sunxi/sun4i-spdif.c b/sound/soc/sunxi/sun4i-spdif.c
> index 89eccc83a1301..f54eb14c9ed86 100644
> --- a/sound/soc/sunxi/sun4i-spdif.c
> +++ b/sound/soc/sunxi/sun4i-spdif.c
> @@ -428,6 +428,11 @@ static int sun4i_spdif_get_status(struct snd_kcontrol *kcontrol,
>  	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
>  	u8 *status = ucontrol->value.iec958.status;
>  	unsigned int reg;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(cpu_dai->dev);
> +	if (ret)
> +		return ret;
>  
>  	scoped_guard(spinlock_irqsave, &host->lock) {
>  		regmap_read(host->regmap, SUN4I_SPDIF_TXCHSTA0, &reg);

[Severity: High]
This isn't a bug introduced by this patch, but does this code copy uninitialized
stack memory to userspace if regmap_read() fails?

The return value of regmap_read() isn't checked here. If it fails, reg remains
unmodified, containing whatever was on the kernel stack. The code then copies it
directly into status, which is part of the snd_ctl_elem_value structure
returned to userspace.

>  		status[0] = reg & 0xff;
>  		status[1] = (reg >> 8) & 0xff;
>  		status[2] = (reg >> 16) & 0xff;
>  		status[3] = (reg >> 24) & 0xff;

[ ... ]

> @@ -453,8 +460,13 @@ static int sun4i_spdif_set_status(struct snd_kcontrol *kcontrol,
>  	struct sun4i_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
>  	u8 *status = ucontrol->value.iec958.status;
>  	unsigned int reg;
> +	int ret;
>  	bool chg0, chg1;

[ ... ]

>  		reg = SUN4I_SPDIF_TXCFG_CHSTMODE;
>  		if (status[0] & IEC958_AES0_NONAUDIO)
>  			reg |= SUN4I_SPDIF_TXCFG_NONAUDIO;
>  
>  		regmap_update_bits(host->regmap, SUN4I_SPDIF_TXCFG,
>  				   SUN4I_SPDIF_TXCFG_CHSTMODE |
>  				   SUN4I_SPDIF_TXCFG_NONAUDIO, reg);
>  	}

[Severity: High]
This is a pre-existing issue, but is the NONAUDIO bit set here unconditionally
overwritten later by sun4i_spdif_hw_params()?

It looks like sun4i_spdif_hw_params() builds a new register configuration from
scratch and uses regmap_write(host->regmap, SUN4I_SPDIF_TXCFG, reg_val). This
would destroy the NONAUDIO bit previously configured here by the user via
kcontrols, unless the stream has exactly 4 channels.

Can this break standard IEC958 passthrough (AC3/DTS) which uses 2 channels and
relies on the kcontrol to set the non-audio bit?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522095401.72915-1-phucduc.bui@gmail.com?part=2