[PATCH] ASoC: amd: ps: fix snd_acp63_remove() teardown ordering

[PATCH] ASoC: amd: ps: fix snd_acp63_remove() teardown ordering

[Fan Wu]

The ACP threaded interrupt handler dereferences the SoundWire and PDM
child platform devices, but snd_acp63_remove() unregisters them while
the interrupt is still registered: devm_request_threaded_irq() ties its
release to devres cleanup, which runs only after the remove callback
returns. A completion in this window is a use-after-free.

Fix this by masking the ACP interrupt sources and calling devm_free_irq()
before the first child device is unregistered. The interrupt line is
shared, and acp_hw_deinit() clears the sources only after the children
are gone, which would leave the line raised with no handler left to
ack it. The window predates the tagged refactor, which only reshaped
the dereferences.

This issue was found by an in-house static analysis tool.

Fixes: eaf825037d6d ("ASoC: amd: ps: refactor acp child platform device creation code")
Cc: stable@vger.kernel.org
Co-developed-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 sound/soc/amd/ps/pci-ps.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
index 729f9aa..01ee697 100644
--- a/sound/soc/amd/ps/pci-ps.c
+++ b/sound/soc/amd/ps/pci-ps.c
@@ -738,6 +738,12 @@ static void snd_acp63_remove(struct pci_dev *pci)
 	int ret;
 
 	adata = pci_get_drvdata(pci);
+	/* Mask the interrupt sources before freeing the shared IRQ. */
+	writel(ACP_EXT_INTR_STAT_CLEAR_MASK,
+	       adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
+	writel(0, adata->acp63_base + ACP_EXTERNAL_INTR_CNTL);
+	writel(0, adata->acp63_base + ACP_EXTERNAL_INTR_ENB);
+	devm_free_irq(&pci->dev, pci->irq, adata);
 	if (adata->sdw) {
 		amd_sdw_exit(adata);
 		platform_device_unregister(adata->sdw_dma_dev);

Re: [PATCH] ASoC: amd: ps: fix snd_acp63_remove() teardown ordering

[Mukunda,Vijendar]

On 9/23/26 14:56, Fan Wu wrote:
> The ACP threaded interrupt handler dereferences the SoundWire and PDM
> child platform devices, but snd_acp63_remove() unregisters them while
> the interrupt is still registered: devm_request_threaded_irq() ties its
> release to devres cleanup, which runs only after the remove callback
> returns. A completion in this window is a use-after-free.
>
> Fix this by masking the ACP interrupt sources and calling devm_free_irq()
> before the first child device is unregistered. The interrupt line is
> shared, and acp_hw_deinit() clears the sources only after the children
> are gone, which would leave the line raised with no handler left to
> ack it. The window predates the tagged refactor, which only reshaped
> the dereferences.
>
> This issue was found by an in-house static analysis tool.
>
> Fixes: eaf825037d6d ("ASoC: amd: ps: refactor acp child platform device creation code")
> Cc: stable@vger.kernel.org
> Co-developed-by: Song Li <songl@zju.edu.cn>
> Signed-off-by: Song Li <songl@zju.edu.cn>
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---
>   sound/soc/amd/ps/pci-ps.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
> index 729f9aa..01ee697 100644
> --- a/sound/soc/amd/ps/pci-ps.c
> +++ b/sound/soc/amd/ps/pci-ps.c
> @@ -738,6 +738,12 @@ static void snd_acp63_remove(struct pci_dev *pci)
>   	int ret;
>   
>   	adata = pci_get_drvdata(pci);
> +	/* Mask the interrupt sources before freeing the shared IRQ. */
> +	writel(ACP_EXT_INTR_STAT_CLEAR_MASK,
> +	       adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
> +	writel(0, adata->acp63_base + ACP_EXTERNAL_INTR_CNTL);
> +	writel(0, adata->acp63_base + ACP_EXTERNAL_INTR_ENB);
++ The fix open-codes the register offsets (ACP_EXTERNAL_INTR_STAT, 
ACP_EXTERNAL_INTR_CNTL, ACP_EXTERNAL_INTR_ENB) directly in 
snd_acp63_remove(). Future platforms may have different interrupt 
control register offsets, which would require changes in this remove 
path as well. Consider adding a disable_interrupts callback to struct 
acp_hw_ops and invoking it here instead. This keeps the remove path 
platform-agnostic and the interrupt masking logic co-located with its 
platform-specific counterpart in ps-common.c.
> +	devm_free_irq(&pci->dev, pci->irq, adata);
>   	if (adata->sdw) {
>   		amd_sdw_exit(adata);
>   		platform_device_unregister(adata->sdw_dma_dev);