Re: [PATCH v3 5/7] ASoC: Intel: avs: SoundWire support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] ASoC: Intel: avs: use-after-free of adev->sdw in
  avs_pci_remove()
- [Medium] ASoC: Intel: avs: uncleaned SoundWire links on
  avs_hda_probe_work() failure
--

commit 838bee43a4741d68e2f300d57b530a29e67bb205
Author: Cezary Rojewski <cezary.rojewski@intel.com>

ASoC: Intel: avs: SoundWire support

This commit adds SoundWire component enumeration, startup, teardown, and
interrupt handling to the Intel AVS driver.

> diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
> index 9413027e1e322..83c19d3e91e65 100644
> --- a/sound/soc/intel/avs/core.c
> +++ b/sound/soc/intel/avs/core.c
> @@ -230,6 +231,11 @@ static void avs_hda_probe_work(struct work_struct *work)
>  	list_for_each_entry(hlink, &bus->hlink_list, list)
>  		snd_hdac_ext_bus_link_put(bus, hlink);
>  
> +	/* Soundwire component is optional */
> +	ret = avs_sdw_probe(adev);
> +	if (ret && ret != -ENODEV)
> +		dev_err(adev->dev, "failed to probe sdw: %d\n", ret);
> +
>  	snd_hdac_ext_bus_ppcap_enable(bus, true);
>  	snd_hdac_ext_bus_ppcap_int_enable(bus, true);
>  	avs_debugfs_init(adev);
> @@ -240,6 +246,14 @@ static void avs_hda_probe_work(struct work_struct *work)
>  
>  	acpi_nhlt_get_gbl_table();
>  
> +	ret = avs_sdw_startup(adev);
> +	if (ret) {
> +		dev_err(adev->dev, "failed to startup sdw: %d\n", ret);
> +		return;
> +	}

[Severity: Medium]
Does returning early when avs_dsp_first_boot_firmware() or
avs_sdw_startup() fails leave the SoundWire auxiliary link devices
registered while the DSP and links are powered down?

avs_sdw_probe() registers the soundwire_intel.link.* auxiliary devices and
sets adev->sdw before firmware boot.  If avs_dsp_first_boot_firmware() or
avs_sdw_startup() fails, the links are never powered up via
sdw_intel_startup() and runtime PM is not enabled.  A later
avs_pci_remove() then runs avs_sdw_exit() -> sdw_intel_exit() ->
intel_link_remove(), which calls sdw_cdns_enable_interrupt(cdns, false) and
writes to unpowered link MMIO registers.

[ ... ]

> @@ -579,11 +595,13 @@ static void avs_pci_remove(struct pci_dev *pci)
>  
>  	acpi_nhlt_put_gbl_table();
>  	avs_debugfs_exit(adev);
> +	avs_sdw_exit(adev);
>  
>  	if (avs_platattr_test(adev, CLDMA))
>  		hda_cldma_free(&code_loader);
>  
>  	snd_hdac_stop_streams_and_chip(bus);
> +	avs_sdw_enable_irq(adev, false);
>  	avs_dsp_op(adev, int_control, false);
>  	snd_hdac_ext_bus_ppcap_int_enable(bus, false);

[Severity: High]
Can avs_cnl_dsp_interrupt() dereference adev->sdw after avs_sdw_exit()
frees it?

avs_sdw_exit() calls sdw_intel_exit(adev->sdw), which frees the
sdw_intel_ctx and its link structures, without clearing adev->sdw and
before SoundWire and DSP interrupts are disabled or freed via
pci_free_irq():

CPU0                              CPU1
avs_pci_remove()
  avs_sdw_exit(adev)
    sdw_intel_exit(adev->sdw)
      ...
      kfree(ctx)
                                  avs_dsp_irq_thread()
                                    avs_cnl_dsp_interrupt()
                                      if (adev->sdw)
                                        avs_sdw_thread(irq, adev)
                                          sdw_intel_thread(irq, adev->sdw)
                                            <-- use-after-free

Disabling SoundWire and DSP interrupts (and synchronizing the IRQ or moving
avs_sdw_exit() after pci_free_irq()) before tearing down adev->sdw, and
clearing adev->sdw in avs_sdw_exit(), would avoid this.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106121557.2338556-1-cezary.rojewski@intel.com?part=5