Re: [PATCH v3 4/5] soundwire: amd: Add BRA/BPT firmware download support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] soundwire: uninitialized pointer dereference of acp_bra_wedged
--

commit bce4ebb14d16cc86a99aa8e0be380048484eab35
Author: Syed Saba Kareem <Syed.SabaKareem@amd.com>

soundwire: amd: Add BRA/BPT firmware download support

This commit adds Bulk Register Access (BRA) and Bulk Payload Transport
(BPT) support for AMD SoundWire platforms. This enables high-speed
firmware download to SoundWire peripherals via DP0, using the ACP BRA
DMA engine.

> diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
> index 8e419ddfa5161..00f4a87da7cbc 100644
> --- a/drivers/soundwire/amd_init.c
> +++ b/drivers/soundwire/amd_init.c
> @@ -120,6 +120,7 @@ static struct sdw_amd_ctx *sdw_amd_probe_controller(struct sdw_amd_res *res)
>  
>  		sdw_pdata[index].instance = index;
>  		sdw_pdata[index].acp_sdw_lock = res->acp_lock;
> +		sdw_pdata[index].acp_bra_lock = res->acp_bra_lock;
>  		sdw_pdata[index].acp_rev = res->acp_rev;
>  		pdevinfo[index].name = "amd_sdw_manager";
>  		pdevinfo[index].id = index;

[Severity: High]
Does this code miss initializing the acp_bra_wedged field in sdw_pdata?

Since sdw_pdata is allocated on the stack and not zero-initialized, this leaves
the acp_bra_wedged pointer containing stack garbage.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index a3316efdf8ac2..078e13b01ac97 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
[ ... ]
> +static u32 amd_sdw_bra_configure_pte(struct amd_sdw_manager *amd_manager,
> +				     dma_addr_t dma_addr, size_t size)
> +{
> +	u32 num_pages = (u32)(PAGE_ALIGN(size) >> PAGE_SHIFT);
> +	u32 low, high, val;
> +	u16 page_idx;
> +	dma_addr_t addr = dma_addr;
> +
> +	if (*amd_manager->acp_bra_wedged) {

[Severity: High]
Can this dereference an uninitialized pointer and cause a kernel oops or panic?

When a BRA firmware download transfer is requested via amd_sdw_bpt_wait(), it
calls amd_sdw_bra_configure_pte() where *amd_manager->acp_bra_wedged is
dereferenced. 

Because the field wasn't initialized in sdw_amd_probe_controller(), this will
dereference an invalid stack address.

> +		dev_err(amd_manager->dev,
> +			"BRA engine previously wedged; refusing to reprogram shared ATU PTEs\n");
> +		return 0;
> +	}
[ ... ]
> @@ -1081,11 +2359,16 @@ static int amd_sdw_manager_probe(struct platform_device *pdev)
>  	amd_manager->mmio = amd_manager->acp_mmio +
>  			    (amd_manager->instance * SDW_MANAGER_REG_OFFSET);
>  	amd_manager->acp_sdw_lock = pdata->acp_sdw_lock;
> +	amd_manager->acp_bra_lock = pdata->acp_bra_lock;
> +	amd_manager->acp_bra_wedged = pdata->acp_bra_wedged;

[Severity: High]
This assigns the uninitialized stack garbage from pdata to
amd_manager->acp_bra_wedged. 

Should sdw_amd_probe_controller() be updated to properly initialize 
sdw_pdata[index].acp_bra_wedged before copying it here?

>  	amd_manager->acp_rev = pdata->acp_rev;
>  	amd_manager->cols_index = sdw_find_col_index(AMD_SDW_DEFAULT_COLUMNS);
>  	amd_manager->rows_index = sdw_find_row_index(AMD_SDW_DEFAULT_ROWS);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005091620.1390916-2-syed.sabakareem@amd.com?part=4