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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A TOCTOU double-fetch vulnerability on `ctrl->ready_buf` allows an out-of-bounds slab read, potentially leaking sensitive kernel heap memory to user-space.
--

commit 77e81df6d58ee7403495ad362c8b579033ed04a6
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: fsl_spdif: Use guard() for spin locks

This patch converts the spin lock handling in fsl_spdif_subcode_get() and
fsl_spdif_qget() to use the guard() macro. It refactors the code without
changing its intended behavior.

> 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,
[ ... ]
>  	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;

[Severity: High]
This is a pre-existing issue, but since the compiler can fetch ctrl->ready_buf
twice (once for the if check, and once for the idx calculation), could this
lead to a Time-of-Check to Time-of-Use (TOCTOU) race?

While this code holds ctl_lock to disable local interrupts, it appears
spdif_irq_uq_err() can run on another CPU and zero ready_buf without taking
ctl_lock:

spdif_irq_uq_err() {
    ...
    /* Drop this U/Q buffer */
    ctrl->ready_buf = 0;
}

If ready_buf becomes 0 between the check and the calculation, idx would
evaluate to a negative index. This would cause the subsequent memcpy to read
out of bounds and potentially leak adjacent slab memory to user-space.

Would caching ready_buf in a local variable using READ_ONCE() prevent this?

>  		memcpy(&ucontrol->value.iec958.subcode[0],
>  				&ctrl->subcode[idx], SPDIF_UBITS_SIZE);
>  		ret = 0;
>  	}
[ ... ]
> @@ -885,17 +883,15 @@ static int fsl_spdif_qget(struct snd_kcontrol *kcontrol,
[ ... ]
>  	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;

[Severity: High]
This is also a pre-existing issue, but does the same double-fetch
vulnerability exist here?

Like the subcode getter above, if ctrl->ready_buf is zeroed by the interrupt
handler after the check, idx becomes a negative offset. This would result in
memcpy reading from the adjacent memory within the struct instead of the
intended qsub array.

>  		memcpy(&ucontrol->value.bytes.data[0],
>  				&ctrl->qsub[idx], SPDIF_QSUB_SIZE);
>  		ret = 0;
>  	}

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