Re: [PATCH v4 07/13] soundwire: amd: fix ctx leak when sdw_amd_startup() fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] soundwire: amd: race and UAF in sdw_amd_probe() error cleanup with active IRQ
--

commit 60bd17e72e211d96b5525ff2d9fb05330bb627b0
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

soundwire: amd: fix ctx leak when sdw_amd_startup() fails

This commit cleans up the SoundWire context and registered platform devices via
sdw_amd_exit() and clears *sdw_ctx if sdw_amd_startup() fails during
sdw_amd_probe().

> diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
> index 8e419ddfa5161..88004d59322ee 100644
> --- a/drivers/soundwire/amd_init.c
> +++ b/drivers/soundwire/amd_init.c
> @@ -166,11 +166,18 @@ 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)
>  {
> +	int ret;
> +
>  	*sdw_ctx = sdw_amd_probe_controller(res);
>  	if (!*sdw_ctx)
>  		return -ENODEV;
>  
> -	return sdw_amd_startup(*sdw_ctx);
> +	ret = sdw_amd_startup(*sdw_ctx);
> +	if (ret) {
> +		sdw_amd_exit(*sdw_ctx);
> +		*sdw_ctx = NULL;
> +	}

[Severity: High]
Could calling sdw_amd_exit(*sdw_ctx) before clearing *sdw_ctx race with the
parent ACP interrupt handler and trigger a use-after-free?

The callers of sdw_amd_probe() (snd_acp63_probe() ->
create_acp63_platform_devs() -> amd_sdw_probe() in sound/soc/amd/ps/pci-ps.c,
and amd_sof_acp_probe() / amd_sof_acp7x_probe() -> amd_sof_sdw_probe() in
sound/soc/sof/amd/acp.c) enable ACP external interrupts and register their
shared hard IRQ handlers (acp63_irq_handler(), acp_irq_handler(), and
acp7x_irq_handler()) before passing &adata->sdw as sdw_ctx to sdw_amd_probe().

In sdw_amd_probe(), *sdw_ctx (adata->sdw) is assigned immediately before
sdw_amd_startup() runs. Inside sdw_amd_startup() -> amd_sdw_manager_start() in
drivers/soundwire/amd_manager.c, interrupts are unmasked before the fallible
manager enable step and before subsequent manager instances in the loop are
started:

drivers/soundwire/amd_manager.c:amd_sdw_manager_start() {
    ...
		amd_enable_sdw_interrupts(amd_manager);
		ret = amd_enable_sdw_manager(amd_manager);
		if (ret)
			return ret;
    ...
}

If amd_enable_sdw_manager() fails, or if manager 0 starts and manager 1 fails
in amd_sdw_manager_start(), sdw_amd_probe() calls sdw_amd_exit(*sdw_ctx) while
the parent PCI IRQ is active and *sdw_ctx (adata->sdw) is still non-NULL:

drivers/soundwire/amd_init.c:sdw_amd_exit() {
	sdw_amd_cleanup(ctx);
	kfree(ctx->peripherals);
	kfree(ctx);
}

Because sdw_amd_cleanup() unregisters and frees ctx->pdev[i] (and its devres
amd_manager) and kfree(ctx) frees the context before *sdw_ctx is set to NULL, a
concurrent hard IRQ handler on another CPU can observe the non-NULL adata->sdw
pointer and dereference freed ctx or ctx->pdev[i]->dev, or schedule
amd_sdw_irq_thread after amd_sdw_manager_remove() has already canceled the work
and freed amd_manager:

sound/soc/sof/amd/acp.c:acp7x_irq_handler() {
    ...
	if (adata->sdw) {
		for (instance = 0; instance < ACP7X_SDW_MAX_MANAGER_COUNT; instance++) {
			if (ext_intr_stat & acp7x_sof_sdw_ext_stat[instance]) {
				sof_acp7x_handle_sdw_manager_irq(sdev, adata, desc, instance,
								 acp7x_sof_sdw_ext_stat[instance]);
				irq_flag = 1;
			}
		}
	}
    ...
}

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);
		if (amd_manager)
			schedule_work(&amd_manager->amd_sdw_irq_thread);
		irq_flag = 1;
	}
    ...
}

Unlike the normal remove paths (which call free_irq() before sdw_amd_exit()),
the IRQ remains active here. Should sdw_amd_probe() hold ctx in a local
variable and only publish *sdw_ctx after sdw_amd_startup() succeeds, while also
ensuring any interrupts enabled during a partial sdw_amd_startup() are masked
and synchronized before sdw_amd_exit() frees the devices and context?

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