Re: [PATCH 2/4] ASoC: SOF: amd: 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: SOF: amd: NULL dereference of adata->sdw->pdev[i] in
  acp_irq_handler()
- [Low] ASoC: SOF: amd: inaccurate teardown claim and missing Fixes tag
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] soundwire: amd: memory and platform device leaks in
  sdw_amd_probe() [1]
--

commit 920e1ff1deb60367eb67bae9d4d4b5880a64d07f
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

> ASoC: SOF: amd: 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 acp_irq_handler() before dereferencing
> ->pdev[0] and ->pdev[1], and in amd_sof_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: 96eb81851012 ("ASoC: SOF: amd: add interrupt handling for SoundWire manager devices")
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

[Severity: Low]
Is the teardown window description in the second paragraph accurate?  In
amd_sof_sdw_exit(), sdw_amd_exit(acp_data->sdw) frees the context before
acp_data->sdw = NULL runs, so adata->sdw is still non-NULL during that
interval and an if (adata->sdw) check cannot guard it; teardown is safe
instead because callers invoke free_irq(sdev->ipc_irq, sdev) before calling
amd_sof_sdw_exit().

Also, since the unguarded adata->sdw dereferences in
amd_sof_handle_acp70_sdw_wake_event() were introduced by a later commit,
should this include:

  Fixes: 1f41bf67bfe3 ("ASoC: SOF: amd: add soundwire wake irq handling")

> diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
> index 86e2402b8ffd2..623e016f38b83 100644
> --- a/sound/soc/sof/amd/acp.c
> +++ b/sound/soc/sof/amd/acp.c
> @@ -478,16 +478,20 @@ static void amd_sof_handle_acp70_sdw_wake_event(struct acp_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;
>  	}
>  }
> @@ -579,10 +583,12 @@ static irqreturn_t acp_irq_handler(int irq, void *dev_id)
>  
>  	val = snd_sof_dsp_read(sdev, ACP_DSP_BAR, desc->ext_intr_stat);
>  	if (val & ACP_SDW0_IRQ_MASK) {
> -		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
>  		snd_sof_dsp_write(sdev, ACP_DSP_BAR, desc->ext_intr_stat, ACP_SDW0_IRQ_MASK);
> -		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;
>  	}
>  
> @@ -608,11 +614,13 @@ static irqreturn_t acp_irq_handler(int irq, void *dev_id)
>  	if (desc->ext_intr_stat1) {
>  		val = snd_sof_dsp_read(sdev, ACP_DSP_BAR, desc->ext_intr_stat1);
>  		if (val & ACP_SDW1_IRQ_MASK) {
> -			amd_manager = dev_get_drvdata(&adata->sdw->pdev[1]->dev);
>  			snd_sof_dsp_write(sdev, ACP_DSP_BAR, desc->ext_intr_stat1,
>  					  ACP_SDW1_IRQ_MASK);
> -			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 here when
adata->sdw is non-NULL?

In sdw_amd_probe_controller(), ctx->pdev[index] is only populated 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 only one SoundWire manager is active,
the unused link's pdev[i] remains NULL while adata->sdw is non-NULL.
Passing &adata->sdw->pdev[i]->dev to dev_get_drvdata() on a stray interrupt
or wake event for the unpopulated link will cause a NULL pointer dereference
in hard IRQ context, unlike the sibling ACP7x handlers in this file (such
as sof_acp7x_handle_sdw_manager_irq()) which check adata->sdw->pdev[instance]
before calling dev_get_drvdata().

[Severity: Medium]
This problem wasn't introduced by this patch, but in the same SoundWire
probe path [1], does a failure in sdw_amd_startup() leak the
sdw_amd_ctx memory and platform devices allocated by
sdw_amd_probe_controller(), as well as adata->dmic_dev in
amd_sof_acp_probe()?

drivers/soundwire/amd_init.c:sdw_amd_probe() {
	*sdw_ctx = sdw_amd_probe_controller(res);
	if (!*sdw_ctx)
		return -ENODEV;

	return sdw_amd_startup(*sdw_ctx);
}

sound/soc/sof/amd/acp.c:amd_sof_acp_probe() {
	...
	adata->dmic_dev = platform_device_register_data(sdev->dev, "dmic-codec",
							PLATFORM_DEVID_NONE, NULL, 0);
	...
	ret = amd_sof_sdw_probe(sdev);
	if (ret < 0) {
		dev_err(sdev->dev, "error: SoundWire probe error\n");
		free_irq(sdev->ipc_irq, sdev);
		return ret;
	}
	...
unregister_dev:
	platform_device_unregister(adata->dmic_dev);
	return ret;
}

When sdw_amd_startup() fails, sdw_amd_probe() returns without unregistering
the ctx->pdev[index] devices or freeing ctx, and amd_sof_acp_probe()
returns directly after free_irq() instead of jumping to unregister_dev to
unregister adata->dmic_dev.

[1] https://sashiko.dev/#/bug/linux-3af16781-2451-4877-8594-c12800a7bba5

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