Re: [PATCH v5 18/25] soundwire: amd: delay *sdw_ctx publication until after startup succeeds

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] soundwire: amd: NULL adata->sdw dereference in IRQ during
  sdw_amd_startup()
--

commit a9bb217bfdd227aefc49880481f23f5b560ad717
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

soundwire: amd: delay *sdw_ctx publication until after startup succeeds

This commit delays assigning *sdw_ctx in sdw_amd_probe() until
sdw_amd_startup() succeeds, cleaning up the local context with
sdw_amd_exit() on failure.

> diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
> index 032697ea35906..ce40b1e5dbf31 100644
> --- a/drivers/soundwire/amd_init.c
> +++ b/drivers/soundwire/amd_init.c
> @@ -176,11 +176,20 @@ static int sdw_amd_startup(struct sdw_amd_ctx *ctx)
>  
>  int sdw_amd_probe(struct sdw_amd_res *res, struct sdw_amd_ctx **sdw_ctx)
>  {
> -	*sdw_ctx = sdw_amd_probe_controller(res);
> -	if (!*sdw_ctx)
> +	struct sdw_amd_ctx *ctx;
> +	int ret;
> +
> +	ctx = sdw_amd_probe_controller(res);
> +	if (!ctx)
>  		return -ENODEV;
>  
> -	return sdw_amd_startup(*sdw_ctx);
> +	ret = sdw_amd_startup(ctx);
> +	if (ret) {
> +		sdw_amd_exit(ctx);
> +		return ret;
> +	}
> +	*sdw_ctx = ctx;
> +	return 0;
>  }
>  EXPORT_SYMBOL_NS(sdw_amd_probe, "SOUNDWIRE_AMD_INIT");

[Severity: High]
Can delaying the assignment of *sdw_ctx until after sdw_amd_startup()
cause a NULL pointer dereference of adata->sdw in the ACP IRQ handler?

The callers (snd_acp63_probe(), amd_sof_acp_probe(), and
amd_sof_acp7x_probe()) register their IRQ handlers and enable global ACP
external interrupts before calling sdw_amd_probe(&sdw_res, &adata->sdw).
Inside sdw_amd_startup(), amd_sdw_manager_start() unmasks SoundWire
interrupts and enables the bus on each link before *sdw_ctx is populated:

CPU0                              <hardirq>
sdw_amd_probe()
  sdw_amd_startup(ctx)
    amd_sdw_manager_start()
      amd_enable_sdw_interrupts()
      amd_enable_sdw_manager()
                                  acp63_irq_handler()
                                    adata->sdw->pdev[0]  <-- NULL deref
  *sdw_ctx = ctx;

Once the link is enabled and attached peripherals signal attachment while
amd_enable_sdw_manager() polls or while the second link starts up,
acp63_irq_handler() and acp_irq_handler() dereference adata->sdw without a
NULL check:

sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
    ...
	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);
    ...
}

On ACP7x, acp7x_irq_handler() checks if (adata->sdw) and skips
sof_acp7x_handle_sdw_manager_irq() when it is NULL, leaving the SoundWire
interrupt status unacknowledged and returning IRQ_NONE.

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