Re: [PATCH 4/4] ASoC: amd: ps: guard SDW IRQ work scheduling with interrupt_enabled flag

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] ASoC: amd: ps: build failure in acp63_irq_handler() due to
  missing struct member
- [Medium] ASoC: amd: ps: TOCTOU race on interrupt_enabled in
  acp63_irq_handler()
--

commit 371c40e74acce37765aa0469d9b4a4ac8ef4b3eb
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

ASoC: amd: ps: guard SDW IRQ work scheduling with interrupt_enabled flag

This commit guards SoundWire manager work scheduling in acp63_irq_handler()
with an amd_manager->interrupt_enabled check for both SoundWire instances.

> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
> index d0b66df101579..0442010e969c0 100644
> --- a/sound/soc/amd/ps/pci-ps.c
> +++ b/sound/soc/amd/ps/pci-ps.c
> @@ -215,7 +215,7 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
>  		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>  		if (adata->sdw) {
>  			amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
> -			if (amd_manager)
> +			if (amd_manager && amd_manager->interrupt_enabled)
>  				schedule_work(&amd_manager->amd_sdw_irq_thread);
>  		}
>  		irq_flag = 1;
> @@ -226,7 +226,7 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
>  		writel(ACP_SDW1_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
>  		if (adata->sdw) {
>  			amd_manager = dev_get_drvdata(&adata->sdw->pdev[1]->dev);
> -			if (amd_manager)
> +			if (amd_manager && amd_manager->interrupt_enabled)
>  				schedule_work(&amd_manager->amd_sdw_irq_thread);
>  		}
>  		irq_flag = 1;

[Severity: High]
Does this break the build when CONFIG_SND_SOC_AMD_PS is enabled?  struct
amd_sdw_manager in include/linux/soundwire/sdw_amd.h has no
interrupt_enabled member, and neither amd_enable_sdw_interrupts() nor
amd_disable_sdw_interrupts() in drivers/soundwire/amd_manager.c sets or
clears such a flag.

[Severity: Medium]
Even with interrupt_enabled added and cleared in
amd_disable_sdw_interrupts(), can acp63_irq_handler() still race with
SoundWire manager teardown and cause a use-after-free of amd_manager?

CPU0                                    CPU1 (<hardirq>)
amd_sdw_manager_remove()
  cancel_work_sync(&amd_sdw_work)
                                        acp63_irq_handler()
                                          amd_manager = dev_get_drvdata(...)
                                          if (amd_manager->interrupt_enabled)
  amd_disable_sdw_interrupts()
    interrupt_enabled = false
                                            schedule_work(&amd_sdw_irq_thread)
device_unbind_cleanup()
  devres_release_all()  /* frees amd_manager */
  dev_set_drvdata(dev, NULL)
                                        amd_sdw_irq_thread()
                                          /* dereferences freed amd_manager */
                                            <-- use-after-free

acp63_irq_handler() checks dev_get_drvdata() and interrupt_enabled locklessly
in hardirq context on the parent PCI device, while amd_sdw_manager_remove()
never cancels amd_sdw_irq_thread or synchronizes with the parent IRQ before
devres frees amd_manager.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107175339.3547918-1-Vijendar.Mukunda@amd.com?part=4