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

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

[Fan Wu]

The ACP IRQ handlers dereference the SoundWire, SoundWire DMA, and PDM
child platform devices, but snd_acp63_remove() unregisters them while
the interrupt is still held by devres, which frees it only after remove
returns. An interrupt in this window is a use-after-free.

Mask the ACP interrupt sources and call devm_free_irq() before the
first child device is unregistered. The masking goes through the new
acp_hw_ops->disable_interrupts callback, keeping the remove path
platform-agnostic. 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.

devm_free_irq() waits for in-flight handlers, but not for work the
hardirq has already queued: acp63_irq_handler() schedules
amd_sdw_irq_thread on the SoundWire manager. Draining that work in
the manager's remove path is a soundwire-side change and follows
separately.

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>
---

Link to v1: https://lore.kernel.org/linux-sound/20260923092640.502145-1-fanwu01@zju.edu.cn/

- mask ACP interrupt sources via the new acp_hw_ops->disable_interrupts
  callback instead of open-coding the register writes in
  snd_acp63_remove(), as suggested by Vijendar Mukunda.

 sound/soc/amd/ps/acp63.h     | 8 ++++++++
 sound/soc/amd/ps/pci-ps.c    | 2 ++
 sound/soc/amd/ps/ps-common.c | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
index 62cb6bef1..d0cfbd4da 100644
--- a/sound/soc/amd/ps/acp63.h
+++ b/sound/soc/amd/ps/acp63.h
@@ -294,6 +294,7 @@ struct acp63_dev_data;
  * struct acp_hw_ops - ACP PCI driver platform specific ops
  * @acp_init: ACP initialization
  * @acp_deinit: ACP de-initialization
+ * @disable_interrupts: disable ACP interrupt sources
  * @acp_get_config: function to read the acp pin configuration
  * @acp_sdw_dma_irq_thread: ACP SoundWire DMA interrupt thread
  * acp_suspend: ACP system level suspend callback
@@ -304,6 +305,7 @@ struct acp63_dev_data;
 struct acp_hw_ops {
 	int (*acp_init)(void __iomem *acp_base, struct device *dev);
 	int (*acp_deinit)(void __iomem *acp_base, struct device *dev);
+	void (*disable_interrupts)(void __iomem *acp_base);
 	void (*acp_get_config)(struct pci_dev *pci, struct acp63_dev_data *acp_data);
 	void (*acp_sdw_dma_irq_thread)(struct acp63_dev_data *acp_data);
 	int (*acp_suspend)(struct device *dev);
@@ -397,6 +399,12 @@ static inline int acp_hw_deinit(struct acp63_dev_data *adata, struct device *dev
 	return -EOPNOTSUPP;
 }
 
+static inline void acp_hw_disable_interrupts(struct acp63_dev_data *adata)
+{
+	if (adata && adata->hw_ops && adata->hw_ops->disable_interrupts)
+		ACP_HW_OPS(adata, disable_interrupts)(adata->acp63_base);
+}
+
 static inline void acp_hw_get_config(struct pci_dev *pci, struct acp63_dev_data *adata)
 {
 	if (adata && adata->hw_ops && adata->hw_ops->acp_get_config)
diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
index 729f9aaba..b604db494 100644
--- a/sound/soc/amd/ps/pci-ps.c
+++ b/sound/soc/amd/ps/pci-ps.c
@@ -738,6 +738,8 @@ static void snd_acp63_remove(struct pci_dev *pci)
 	int ret;
 
 	adata = pci_get_drvdata(pci);
+	acp_hw_disable_interrupts(adata);
+	devm_free_irq(&pci->dev, pci->irq, adata);
 	if (adata->sdw) {
 		amd_sdw_exit(adata);
 		platform_device_unregister(adata->sdw_dma_dev);
diff --git a/sound/soc/amd/ps/ps-common.c b/sound/soc/amd/ps/ps-common.c
index 7b4966b75..ea71cdf19 100644
--- a/sound/soc/amd/ps/ps-common.c
+++ b/sound/soc/amd/ps/ps-common.c
@@ -248,6 +248,7 @@ void acp63_hw_init_ops(struct acp_hw_ops *hw_ops)
 {
 	hw_ops->acp_init = acp63_init;
 	hw_ops->acp_deinit = acp63_deinit;
+	hw_ops->disable_interrupts = acp63_disable_interrupts;
 	hw_ops->acp_get_config = acp63_get_config;
 	hw_ops->acp_sdw_dma_irq_thread = acp63_sdw_dma_irq_thread;
 	hw_ops->acp_suspend = snd_acp63_suspend;
@@ -484,6 +485,7 @@ void acp70_hw_init_ops(struct acp_hw_ops *hw_ops)
 {
 	hw_ops->acp_init = acp70_init;
 	hw_ops->acp_deinit = acp70_deinit;
+	hw_ops->disable_interrupts = acp70_disable_interrupts;
 	hw_ops->acp_get_config = acp70_get_config;
 	hw_ops->acp_sdw_dma_irq_thread = acp70_sdw_dma_irq_thread;
 	hw_ops->acp_suspend = snd_acp70_suspend;
-- 
2.34.1

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

[Mukunda,Vijendar]

On 29/09/26 11:43, Fan Wu wrote:
> [Some people who received this message don't often get email from fanwu01@zju.edu.cn. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> The ACP IRQ handlers dereference the SoundWire, SoundWire DMA, and PDM
> child platform devices, but snd_acp63_remove() unregisters them while
> the interrupt is still held by devres, which frees it only after remove
> returns. An interrupt in this window is a use-after-free.
>
> Mask the ACP interrupt sources and call devm_free_irq() before the
> first child device is unregistered. The masking goes through the new
> acp_hw_ops->disable_interrupts callback, keeping the remove path
> platform-agnostic. 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.
>
> devm_free_irq() waits for in-flight handlers, but not for work the
> hardirq has already queued: acp63_irq_handler() schedules
> amd_sdw_irq_thread on the SoundWire manager. Draining that work in
> the manager's remove path is a soundwire-side change and follows
> separately.
>
> 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>
Reviewed-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> ---
>
> Link to v1: https://lore.kernel.org/linux-sound/20260923092640.502145-1-fanwu01@zju.edu.cn/
>
> - mask ACP interrupt sources via the new acp_hw_ops->disable_interrupts
>   callback instead of open-coding the register writes in
>   snd_acp63_remove(), as suggested by Vijendar Mukunda.
>
>  sound/soc/amd/ps/acp63.h     | 8 ++++++++
>  sound/soc/amd/ps/pci-ps.c    | 2 ++
>  sound/soc/amd/ps/ps-common.c | 2 ++
>  3 files changed, 12 insertions(+)
>
> diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
> index 62cb6bef1..d0cfbd4da 100644
> --- a/sound/soc/amd/ps/acp63.h
> +++ b/sound/soc/amd/ps/acp63.h
> @@ -294,6 +294,7 @@ struct acp63_dev_data;
>   * struct acp_hw_ops - ACP PCI driver platform specific ops
>   * @acp_init: ACP initialization
>   * @acp_deinit: ACP de-initialization
> + * @disable_interrupts: disable ACP interrupt sources
>   * @acp_get_config: function to read the acp pin configuration
>   * @acp_sdw_dma_irq_thread: ACP SoundWire DMA interrupt thread
>   * acp_suspend: ACP system level suspend callback
> @@ -304,6 +305,7 @@ struct acp63_dev_data;
>  struct acp_hw_ops {
>         int (*acp_init)(void __iomem *acp_base, struct device *dev);
>         int (*acp_deinit)(void __iomem *acp_base, struct device *dev);
> +       void (*disable_interrupts)(void __iomem *acp_base);
>         void (*acp_get_config)(struct pci_dev *pci, struct acp63_dev_data *acp_data);
>         void (*acp_sdw_dma_irq_thread)(struct acp63_dev_data *acp_data);
>         int (*acp_suspend)(struct device *dev);
> @@ -397,6 +399,12 @@ static inline int acp_hw_deinit(struct acp63_dev_data *adata, struct device *dev
>         return -EOPNOTSUPP;
>  }
>
> +static inline void acp_hw_disable_interrupts(struct acp63_dev_data *adata)
> +{
> +       if (adata && adata->hw_ops && adata->hw_ops->disable_interrupts)
> +               ACP_HW_OPS(adata, disable_interrupts)(adata->acp63_base);
> +}
> +
>  static inline void acp_hw_get_config(struct pci_dev *pci, struct acp63_dev_data *adata)
>  {
>         if (adata && adata->hw_ops && adata->hw_ops->acp_get_config)
> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
> index 729f9aaba..b604db494 100644
> --- a/sound/soc/amd/ps/pci-ps.c
> +++ b/sound/soc/amd/ps/pci-ps.c
> @@ -738,6 +738,8 @@ static void snd_acp63_remove(struct pci_dev *pci)
>         int ret;
>
>         adata = pci_get_drvdata(pci);
> +       acp_hw_disable_interrupts(adata);
> +       devm_free_irq(&pci->dev, pci->irq, adata);
>         if (adata->sdw) {
>                 amd_sdw_exit(adata);
>                 platform_device_unregister(adata->sdw_dma_dev);
> diff --git a/sound/soc/amd/ps/ps-common.c b/sound/soc/amd/ps/ps-common.c
> index 7b4966b75..ea71cdf19 100644
> --- a/sound/soc/amd/ps/ps-common.c
> +++ b/sound/soc/amd/ps/ps-common.c
> @@ -248,6 +248,7 @@ void acp63_hw_init_ops(struct acp_hw_ops *hw_ops)
>  {
>         hw_ops->acp_init = acp63_init;
>         hw_ops->acp_deinit = acp63_deinit;
> +       hw_ops->disable_interrupts = acp63_disable_interrupts;
>         hw_ops->acp_get_config = acp63_get_config;
>         hw_ops->acp_sdw_dma_irq_thread = acp63_sdw_dma_irq_thread;
>         hw_ops->acp_suspend = snd_acp63_suspend;
> @@ -484,6 +485,7 @@ void acp70_hw_init_ops(struct acp_hw_ops *hw_ops)
>  {
>         hw_ops->acp_init = acp70_init;
>         hw_ops->acp_deinit = acp70_deinit;
> +       hw_ops->disable_interrupts = acp70_disable_interrupts;
>         hw_ops->acp_get_config = acp70_get_config;
>         hw_ops->acp_sdw_dma_irq_thread = acp70_sdw_dma_irq_thread;
>         hw_ops->acp_suspend = snd_acp70_suspend;
> --
> 2.34.1