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 <[email protected]> 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/[email protected]?part=4
