Re: [PATCH v2 07/11] ASoC: fsl_xcvr: 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 probe function registers the IRQ handler and ALSA component before initializing the spinlock and workqueue, creating a critical race condition.
- [High] Device removal cancels the work struct before the IRQ is unregistered, permitting a Use-After-Free if an interrupt fires during the teardown phase.
--

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

ASoC: fsl_xcvr: 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_xcvr.c b/sound/soc/fsl/fsl_xcvr.c
> index 6677d3bf36ec2..41d100500534f 100644
> --- a/sound/soc/fsl/fsl_xcvr.c
> +++ b/sound/soc/fsl/fsl_xcvr.c
> @@ -797,10 +797,9 @@ static int fsl_xcvr_trigger(struct snd_pcm_substream *substream, int cmd,
>  {
>  	struct fsl_xcvr *xcvr = snd_soc_dai_get_drvdata(dai);
>  	bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
> -	unsigned long lock_flags;
>  	int ret = 0;
>  
> -	spin_lock_irqsave(&xcvr->lock, lock_flags);
> +	guard(spinlock_irqsave)(&xcvr->lock);

[Severity: High]
This is a pre-existing issue, but since we are modifying the lock usage here,
could the uninitialized lock be acquired if an interrupt fires early or ALSA
binds immediately?

In fsl_xcvr_probe(), the IRQ handler and ALSA component are registered
before the lock and workqueue are initialized:

fsl_xcvr_probe() {
	...
	ret = devm_request_irq(dev, irq, irq0_isr, 0, pdev->name, xcvr);
	...
	ret = devm_snd_soc_register_component(dev, &fsl_xcvr_comp,
					      &fsl_xcvr_dai, 1);
	...
	INIT_WORK(&xcvr->work_rst, reset_rx_work);
	spin_lock_init(&xcvr->lock);
	...
}

If an interrupt fires immediately upon registration, irq0_isr() will invoke
schedule_work() on an uninitialized work struct.

Similarly, if ALSA immediately invokes the stream trigger callbacks upon
component registration, fsl_xcvr_trigger() will attempt to acquire the
uninitialized spinlock.

[ ... ]

> @@ -1448,11 +1445,10 @@ static void reset_rx_work(struct work_struct *work)
>  {
>  	struct fsl_xcvr *xcvr = container_of(work, struct fsl_xcvr, work_rst);
>  	struct device *dev = &xcvr->pdev->dev;
> -	unsigned long lock_flags;
>  	u32 ext_ctrl;
>  
>  	dev_dbg(dev, "reset rx path\n");
> -	spin_lock_irqsave(&xcvr->lock, lock_flags);
> +	guard(spinlock_irqsave)(&xcvr->lock);

[Severity: High]
This is a pre-existing issue, but could there be a use-after-free during
device removal if an interrupt fires after the work struct is cancelled?

The driver manages the IRQ lifetime using devres, which defers the IRQ
unregistration until after fsl_xcvr_remove() returns:

fsl_xcvr_remove() {
	...
	cancel_work_sync(&xcvr->work_rst);
	pm_runtime_disable(&pdev->dev);
}

Because the IRQ is still actively registered at this time, an incoming
interrupt right after cancel_work_sync() completes will cause irq0_isr() to
call schedule_work(&xcvr->work_rst) again.

As the removal sequence proceeds, devres ultimately frees the xcvr struct,
leaving a scheduled work item in the system pointing to freed memory.

When reset_rx_work() subsequently executes, it will dereference the freed
pointer.

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