Re: [PATCH 05/11] ASoC: fsl_spdif: Use guard() for spin locks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The Q-subcode interrupt handler incorrectly writes its data into the `subcode` (U-subcode) array, leaving `qsub` empty and corrupting U-subcode data.
- [High] The `ctl_lock` spinlock does not protect against concurrent modification by the interrupt handler on SMP systems, leading to a critical data race and severe out-of-bounds read.
--

commit 9c302e33fc7af448093d631b3d2663e0de990905
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: fsl_spdif: Use guard() for spin locks

Clean up the code using guard() for spin locks.
Merely code refactoring, and no behavior change.

> diff --git a/sound/soc/fsl/fsl_spdif.c b/sound/soc/fsl/fsl_spdif.c
> index 1b9be85b34c24..ad1206ed98828 100644
> --- a/sound/soc/fsl/fsl_spdif.c
> +++ b/sound/soc/fsl/fsl_spdif.c
> @@ -853,17 +853,15 @@ static int fsl_spdif_subcode_get(struct snd_kcontrol *kcontrol,
>  	struct snd_soc_dai *cpu_dai = snd_kcontrol_chip(kcontrol);
>  	struct fsl_spdif_priv *spdif_priv = snd_soc_dai_get_drvdata(cpu_dai);
>  	struct spdif_mixer_control *ctrl = &spdif_priv->fsl_spdif_control;
> -	unsigned long flags;
>  	int ret = -EAGAIN;
>  
> -	spin_lock_irqsave(&ctrl->ctl_lock, flags);
> +	guard(spinlock_irqsave)(&ctrl->ctl_lock);
>  	if (ctrl->ready_buf) {
>  		int idx = (ctrl->ready_buf - 1) * SPDIF_UBITS_SIZE;
>  		memcpy(&ucontrol->value.iec958.subcode[0],
>  				&ctrl->subcode[idx], SPDIF_UBITS_SIZE);
>  		ret = 0;
>  	}

[Severity: High]
This is a pre-existing issue, but does ctl_lock actually protect against
concurrent modification by the interrupt handler on SMP systems?

The ctl_lock is acquired here by readers in fsl_spdif_subcode_get() and
fsl_spdif_qget(). While this disables local interrupts, the interrupt
handler (spdif_isr and helpers like spdif_irq_uq_err) can execute
concurrently on another CPU in an SMP system because it does not acquire
ctl_lock.

If the ISR clears ctrl->ready_buf to 0 between the if (ctrl->ready_buf)
check and the calculation of idx, the compiler re-evaluation of
ready_buf could cause idx to evaluate to -96. This would trigger an
out-of-bounds read in memcpy, leaking arbitrary kernel heap memory to
userspace:

sound/soc/fsl/fsl_spdif.c:spdif_irq_uq_err() {
    ...
	/* Drop this U/Q buffer */
	ctrl->ready_buf = 0;
	ctrl->upos = 0;
	ctrl->qpos = 0;
    ...
}

[ ... ]

> @@ -885,17 +883,15 @@ static int fsl_spdif_qget(struct snd_kcontrol *kcontrol,
>  	struct snd_soc_dai *cpu_dai = snd_kcontrol_chip(kcontrol);
>  	struct fsl_spdif_priv *spdif_priv = snd_soc_dai_get_drvdata(cpu_dai);
>  	struct spdif_mixer_control *ctrl = &spdif_priv->fsl_spdif_control;
> -	unsigned long flags;
>  	int ret = -EAGAIN;
>  
> -	spin_lock_irqsave(&ctrl->ctl_lock, flags);
> +	guard(spinlock_irqsave)(&ctrl->ctl_lock);
>  	if (ctrl->ready_buf) {
>  		int idx = (ctrl->ready_buf - 1) * SPDIF_QSUB_SIZE;
>  		memcpy(&ucontrol->value.bytes.data[0],
>  				&ctrl->qsub[idx], SPDIF_QSUB_SIZE);

[Severity: High]
This isn't a bug introduced by this patch, but does the Q-subcode array
actually contain valid data here?

Looking at the Q-subcode interrupt handler spdif_irq_uqrx_full(), it
unconditionally writes its data into the subcode (U-subcode) array, even
when processing a Q-subcode interrupt:

sound/soc/fsl/fsl_spdif.c:spdif_irq_uqrx_full() {
    ...
	ctrl->subcode[*pos++] = val >> 16;
	ctrl->subcode[*pos++] = val >> 8;
	ctrl->subcode[*pos++] = val;
    ...
}

Because qpos increments up to 24 independently of upos, this overwrites
the first 24 bytes of the U-channel buffer (ctrl->subcode) and leaves
qsub empty. As a result, fsl_spdif_qget() will return uninitialized or
zeroed memory from ctrl->qsub, and the U-subcode data returned to
userspace is corrupted.

>  		ret = 0;
>  	}

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