Skip to content

dma: fix error-path and bounds issues in dw/sdma/acp drivers #10932

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lgirdwood merged 3 commits into from
Jun 18, 2026
Merged

dma: fix error-path and bounds issues in dw/sdma/acp drivers #10932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a03392
acp-dma: clear channel pointer after free on probe failure
lrgirdwo Jun 11, 2026
5a52ae2
dw-dma: commit descriptor count only after successful alloc
lrgirdwo Jun 11, 2026
e51af02
sdma: fix off-by-one in the event bound check
lrgirdwo Jun 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/drivers/amd/common/acp_dma.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,7 @@ static int acp_dma_probe(struct dma *dma)
sizeof(struct acp_dma_chan_data));
if (!acp_dma_chan) {
rfree(dma->chan);
dma->chan = NULL;
tr_err(&acpdma_tr, "acp-dma: %d channel %d private data alloc failed",
dma->plat_data.id, channel);
return -ENOMEM;
Expand Down
15 changes: 10 additions & 5 deletions src/drivers/dw/dma.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,8 +532,6 @@ static int dw_dma_set_config(struct dma_chan_data *channel,
/* do we need to realloc descriptors */
if (config->elem_array.count != channel->desc_count) {

channel->desc_count = config->elem_array.count;

/*
* Allocate descriptors for channel. They must be cache-line
* size aligned to avoid corrupting adjacent memory when
Expand All @@ -542,18 +540,25 @@ static int dw_dma_set_config(struct dma_chan_data *channel,
* allocations on Zephyr to always force cache-line size
* alignment.
*/
if (dw_chan->lli)
rfree(dw_chan->lli);
rfree(dw_chan->lli);

dw_chan->lli = rmalloc(SOF_MEM_FLAG_KERNEL | SOF_MEM_FLAG_COHERENT | SOF_MEM_FLAG_DMA,
sizeof(struct dw_lli) * channel->desc_count);
sizeof(struct dw_lli) * config->elem_array.count);

@lyakh lyakh Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, the if in line 543 / 545 above is redundant

@lgirdwood lgirdwood Jun 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point — dropped the redundant if (dw_chan->lli) guard; rfree(NULL) is a no-op (zephyr returns early on NULL, the library build calls free()), so the unconditional rfree(dw_chan->lli) is fine.

if (!dw_chan->lli) {
tr_err(&dwdma_tr, "dma %d channel %d lli alloc failed",
channel->dma->plat_data.id,
channel->index);
/* allocation failed, so dw_chan->lli is now NULL; reset
* the count to match it so a later config does not
* bzero() a NULL pointer using a stale count
*/

@lyakh lyakh Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather say, that the point is not that it was freed above, but that the allocation has failed and it's now NULL. Maybe it wasn't freed and instead it was never allocated yet.

@lgirdwood lgirdwood Jun 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworded — the comment now says the allocation failed so dw_chan->lli is now NULL (it may never have been allocated), and the count is reset to match, rather than implying it was freed above.

channel->desc_count = 0;
ret = -ENOMEM;
goto out;
}

/* only commit the new count once the buffer is allocated */
channel->desc_count = config->elem_array.count;
}

/* initialise descriptors */
Expand Down
4 changes: 2 additions & 2 deletions src/drivers/imx/sdma.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ static void sdma_enable_event(struct dma_chan_data *channel, int eventnum)

tr_dbg(&sdma_tr, "channel %d, event %d", channel->index, eventnum);

if (eventnum < 0 || eventnum > SDMA_HWEVENTS_COUNT)
if (eventnum < 0 || eventnum >= SDMA_HWEVENTS_COUNT)
return; /* No change if request is invalid */

dma_reg_update_bits(channel->dma, SDMA_CHNENBL(eventnum),
Expand All @@ -461,7 +461,7 @@ static void sdma_disable_event(struct dma_chan_data *channel, int eventnum)
{
tr_dbg(&sdma_tr, "channel %d, event %d", channel->index, eventnum);

if (eventnum < 0 || eventnum > SDMA_HWEVENTS_COUNT)
if (eventnum < 0 || eventnum >= SDMA_HWEVENTS_COUNT)
return; /* No change if request is invalid */

dma_reg_update_bits(channel->dma, SDMA_CHNENBL(eventnum),
Expand Down
Loading