Re: [PATCH v2] ASoC: SOF: Intel: hda: Fix NULL pointer dereference on SoundWire IRQ during removal

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

On 3/12/26 08:08, gaggery.tsai@intel.com wrote:
> From: Gaggery Tsai <gaggery.tsai@intel.com>
> 
> hda_sdw_exit() sets hdev->sdw to NULL after calling sdw_intel_exit(),
> but the shared IPC IRQ handler is not freed until much later in
> hda_dsp_remove(). If a SoundWire interrupt fires in this window, the
> IRQ thread calls hda_dsp_sdw_thread() -> sdw_intel_thread() with a
> NULL context pointer or with link->cdns already freed, causing a NULL
> pointer dereference:
> 
>   BUG: kernel NULL pointer dereference, address: 00000000000003d0
>   RIP: 0010:sdw_cdns_irq+0x9/0x2b0 [soundwire_cadence]
>   Call Trace:
>    sdw_intel_thread+0x2d/0x50 [soundwire_intel]
>    hda_dsp_interrupt_thread+0x99/0x3a0 [snd_sof_intel_hda_generic]
>    irq_thread_fn+0x25/0x60
> 
> The race window is between hda_sdw_exit() tearing down SoundWire
> links and free_irq() in hda_dsp_remove(). During sdw_intel_exit() ->
> sdw_intel_cleanup(), each link's auxiliary device is unregistered,
> which clears link->cdns. Meanwhile the IRQ thread can still fire and
> iterate the link list, calling sdw_cdns_irq() with a NULL cdns.
> 
> Fix this in three ways:
> 
>   1. In hda_sdw_exit(), disable SoundWire interrupts at the hardware
>      level (hda_sdw_int_enable) and call synchronize_irq() BEFORE
>      tearing down the SoundWire context, preventing new IRQ threads
>      from entering the SoundWire path.
> 
>   2. Add a NULL guard for link->cdns in sdw_intel_thread() to handle
>      the case where the IRQ thread races with individual link
>      removal during sdw_intel_cleanup().
> 
>   3. Add a NULL guard in hda_dsp_sdw_thread() as defense-in-depth
>      for the case where hdev->sdw is already NULL.
> 
> Tested on Intel Panther Lake with SoundWire codecs by manually
> unbinding the SOF PCI device while audio was active.
> 
> Fixes: 722ba5f1f530 ("ASoC: SOF: Intel: hda: merge IPC, stream and SoundWire interrupt handlers")
> Cc: stable@vger.kernel.org
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>
> Cc: Bard Liao <yung-chuan.liao@linux.intel.com>
> Cc: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> Signed-off-by: Gaggery Tsai <gaggery.tsai@intel.com>
> ---
>  drivers/soundwire/intel_init.c |  6 ++++--
>  sound/soc/sof/intel/hda.c      | 11 +++++++++--
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soundwire/intel_init.c b/drivers/soundwire/intel_init.c
> index ad48d67fa935..e093a29f1590 100644
> --- a/drivers/soundwire/intel_init.c
> +++ b/drivers/soundwire/intel_init.c
> @@ -145,8 +145,10 @@ irqreturn_t sdw_intel_thread(int irq, void *dev_id)
>  	struct sdw_intel_ctx *ctx = dev_id;
>  	struct sdw_intel_link_res *link;
>  
> -	list_for_each_entry(link, &ctx->link_list, list)
> -		sdw_cdns_irq(irq, link->cdns);
> +	list_for_each_entry(link, &ctx->link_list, list) {
> +		if (link->cdns)
> +			sdw_cdns_irq(irq, link->cdns);

is this really a fix? Couldn't you have a case where the branch is taken, but the context is freed by the exit below?
You'd have a case of use-after-free, which is just as problematic as accessing a NULL pointer...

The synchronize_irq() only guarantees no new IRQ will be generated, but it doesn't control if a thread can execute and when the context is used.

It almost feels like you need some sort of lock to prevent this list from being accessed.

Bard, can you review this as well?

> +	}
>  
>  	return IRQ_HANDLED;
>  }
> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
> index c0cc7d3ce526..02a0e354414e 100644
> --- a/sound/soc/sof/intel/hda.c
> +++ b/sound/soc/sof/intel/hda.c
> @@ -256,12 +256,17 @@ static int hda_sdw_exit(struct snd_sof_dev *sdev)
>  
>  	hdev = sdev->pdata->hw_pdata;
>  
> +	/* Disable SoundWire IRQ at the hardware level first to prevent
> +	 * the IRQ handler from accessing hdev->sdw after it is freed.
> +	 * synchronize_irq() ensures any in-flight handler has completed.
> +	 */
> +	hda_sdw_int_enable(sdev, false);
> +	synchronize_irq(sdev->ipc_irq);

In addition to my concern above, this feels like a layering violation. The IRQ is used for SoundWire,
but also IPC and DSP interrupts. You'd want to do this in hda_dsp_remove(), no?

This may require hda_sdw_exit() to be broken in two, with a _disable() followed by _remove() helper.

> +
>  	if (hdev->sdw)
>  		sdw_intel_exit(hdev->sdw);
>  	hdev->sdw = NULL;
>  
> -	hda_sdw_int_enable(sdev, false);
> -
>  	return 0;
>  }
>  
> @@ -309,6 +314,8 @@ static bool hda_dsp_check_sdw_irq(struct snd_sof_dev *sdev)
>  
>  static irqreturn_t hda_dsp_sdw_thread(int irq, void *context)
>  {
> +	if (!context)
> +		return IRQ_HANDLED;

If the issues are solved, is this really needed?

>  	return sdw_intel_thread(irq, context);
>  }
>