Re: [PATCH 1/4] ASoC: amd: ps: guard adata->sdw dereferences with NULL check in IRQ handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ASoC: amd: ps: NULL deref of adata->sdw->pdev[i] on
  single-link SDW configs
- [Low] ASoC: amd: ps: inaccurate commit message teardown claim and
  Fixes tag
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] soundwire: amd: use-after-free in amd_sdw_manager_remove()
  [1]
--

commit 929ea19624010c51ae72f2eb65f2e1a41edc5971
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

> ASoC: amd: ps: guard adata->sdw dereferences with NULL check in IRQ handler
>
> The ACP external IRQ handler is registered before sdw_amd_probe() assigns
> adata->sdw, so a stray interrupt in the probe window could reach the
> adata->sdw->pdev[i] dereference with adata->sdw still NULL.
>
> The same window exists on the teardown side: after sdw_amd_exit() frees
> the context but before adata->sdw is cleared by the caller.
>
> Add NULL checks for adata->sdw in acp63_irq_handler() before dereferencing
> ->pdev[0] and ->pdev[1], and in handle_acp70_sdw_wake_event() for the same
> reason.  The interrupt-status register write that acknowledges the IRQ is
> kept outside the guard so the hardware bit is always cleared regardless of
> whether the SoundWire context is live.
>
> Fixes: e1cb350610ce ("ASoC: amd: ps: handle SoundWire interrupts in acp pci driver")
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

[Severity: Low]
Is the teardown description and Fixes tag accurate here?

In amd_sdw_exit(), sdw_amd_exit(acp_data->sdw) frees the context while
acp_data->sdw is still non-NULL, and acp_data->sdw = NULL only runs after
sdw_amd_exit() returns, so an if (adata->sdw) check in that window would
see a dangling non-NULL pointer rather than NULL (snd_acp63_remove()
instead avoids this by calling acp_hw_disable_interrupts() and
devm_free_irq() before amd_sdw_exit()).

Also, e1cb350610ce ("ASoC: amd: ps: handle SoundWire interrupts in acp pci
driver") predates adata->sdw.  The adata->sdw dereferences in
acp63_irq_handler() were introduced by eaf825037d6d ("ASoC: amd: ps:
refactor acp child platform device creation code"), and
handle_acp70_sdw_wake_event() was added by 3898b189079c ("ASoC: amd: ps:
add soundwire wake interrupt handling").

> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
> index b604db4942dd6..d0b66df101579 100644
> --- a/sound/soc/amd/ps/pci-ps.c
> +++ b/sound/soc/amd/ps/pci-ps.c
> @@ -26,16 +26,20 @@ static void handle_acp70_sdw_wake_event(struct acp63_dev_data *adata)
>  	struct amd_sdw_manager *amd_manager;
>  
>  	if (adata->acp70_sdw0_wake_event) {
> -		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
> -		if (amd_manager)
> -			pm_request_resume(amd_manager->dev);
> +		if (adata->sdw) {
> +			amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
> +			if (amd_manager)
> +				pm_request_resume(amd_manager->dev);
> +		}
>  		adata->acp70_sdw0_wake_event = 0;
>  	}
>  
>  	if (adata->acp70_sdw1_wake_event) {
> -		amd_manager = dev_get_drvdata(&adata->sdw->pdev[1]->dev);
> -		if (amd_manager)
> -			pm_request_resume(amd_manager->dev);
> +		if (adata->sdw) {
> +			amd_manager = dev_get_drvdata(&adata->sdw->pdev[1]->dev);
> +			if (amd_manager)
> +				pm_request_resume(amd_manager->dev);
> +		}
>  		adata->acp70_sdw1_wake_event = 0;
>  	}
>  }
> @@ -209,18 +213,22 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
>  	ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>  	if (ext_intr_stat & ACP_SDW0_STAT) {
>  		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
> -		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
> -		if (amd_manager)
> -			schedule_work(&amd_manager->amd_sdw_irq_thread);
> +		if (adata->sdw) {
> +			amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
> +			if (amd_manager)
> +				schedule_work(&amd_manager->amd_sdw_irq_thread);
> +		}
>  		irq_flag = 1;
>  	}
>  
>  	ext_intr_stat1 = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
>  	if (ext_intr_stat1 & ACP_SDW1_STAT) {
>  		writel(ACP_SDW1_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
> -		amd_manager = dev_get_drvdata(&adata->sdw->pdev[1]->dev);
> -		if (amd_manager)
> -			schedule_work(&amd_manager->amd_sdw_irq_thread);
> +		if (adata->sdw) {
> +			amd_manager = dev_get_drvdata(&adata->sdw->pdev[1]->dev);
> +			if (amd_manager)
> +				schedule_work(&amd_manager->amd_sdw_irq_thread);
> +		}
>  		irq_flag = 1;
>  	}

[Severity: Medium]
Can adata->sdw->pdev[0] or adata->sdw->pdev[1] still be NULL when
adata->sdw is non-NULL?

In sdw_amd_probe_controller(), ctx->pdev[index] is only registered for
links set in link_mask:

drivers/soundwire/amd_init.c:sdw_amd_probe_controller() {
    ...
	for (index = 0; index < count; index++) {
		if (!(link_mask & BIT(index)))
			continue;
    ...
		ctx->pdev[index] = platform_device_register_full(&pdevinfo[index]);
}

On single-link configurations (where link_mask is BIT(0) or BIT(1)),
adata->sdw is non-NULL while the unused link's pdev[i] remains NULL.  A
stray interrupt or wake event for that link in acp63_irq_handler() or
handle_acp70_sdw_wake_event() will then pass &adata->sdw->pdev[i]->dev to
dev_get_drvdata() and cause a NULL pointer dereference, as
acp70_enable_sdw_host_wake_interrupts() unmasks ACP70_SDW_HOST_WAKE_MASK
for both links whenever either link enables host wake.

Should these checks also verify adata->sdw->pdev[0] and
adata->sdw->pdev[1] (like handle_acp63_sdw_pme_event() does)?

[Severity: Critical]
This problem wasn't introduced by this patch, but can unbinding the child
SoundWire manager device race with acp63_irq_handler() and cause a
use-after-free of amd_manager [1]?

CPU0                                CPU1
acp63_irq_handler()
  amd_manager = dev_get_drvdata()
                                    amd_sdw_manager_remove()
                                      cancel_work_sync(...)
                                      amd_disable_sdw_interrupts()
                                    <driver core frees amd_manager>
  if (amd_manager)
    schedule_work(&amd_manager->amd_sdw_irq_thread);  <-- use-after-free

amd_sdw_manager_remove() masks SoundWire interrupts and cancels
amd_sdw_work, but does not synchronize with the parent ACP IRQ handler or
cancel amd_sdw_irq_thread before devres frees amd_manager.  If
acp63_irq_handler() reads amd_manager before removal finishes and calls
schedule_work() afterwards, it queues work on the freed amd_manager.

[1] https://sashiko.dev/#/bug/linux-ab458b2e-f505-4a41-8238-a93ceb5d9c77

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