dma: fix error-path and bounds issues in dw/sdma/acp drivers#10932
Conversation
lgirdwood
requested review from
dbaluta,
kv2019i,
lbetlej,
lyakh,
mmaka1 and
plbossart
as code owners
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes error-path and bounds-check issues in multiple DMA drivers to prevent dangling frees, NULL-pointer zeroing, and out-of-range event access.
Changes:
- acp-dma: clear
dma->chanafter freeing on partial-probe failure to avoid double-free on retry. - dw-dma: only commit
desc_countafter successful (re)allocation; reset to 0 on alloc failure to keep count/buffer consistent. - sdma: correct event-id bounds checks from
>to>=in both enable/disable event paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/drivers/imx/sdma.c | Tightens event-id bounds checks to prevent out-of-range register access. |
| src/drivers/dw/dma.c | Fixes descriptor count/buffer consistency on realloc failure to avoid NULL bzero. |
| src/drivers/amd/common/acp_dma.c | Prevents probe-retry double-free by NULLing a freed channel pointer. |
| rfree(dma->chan); | ||
| /* clear the freed pointer so a later probe retry sees no | ||
| * channel context instead of a dangling one to free again | ||
| */ |
There was a problem hiding this comment.
superfluous obvious comment, should not be there
There was a problem hiding this comment.
Removed — dropped the comment.
| /* lli was freed above; keep desc_count consistent with | ||
| * the now-NULL buffer so a later config does not bzero() | ||
| * a NULL pointer using a stale count | ||
| */ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| 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); |
There was a problem hiding this comment.
btw, the if in line 543 / 545 above is redundant
There was a problem hiding this comment.
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.
On a partial channel-init failure the channel array was freed but the pointer was left set, so a later probe retry would free it again. Clear the pointer after freeing. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
The descriptor count was updated before the descriptor buffer was reallocated, so on allocation failure the count was left larger than the now-NULL buffer and a later config zeroed a NULL pointer using the stale count. Update the count only after a successful allocation and reset it on failure. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
The event-number range check used a strict greater-than against the event count, allowing the count value itself to index one past the array. Use greater-or-equal, in both the enable and disable paths. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
lgirdwood
force-pushed
the
fix-dma-drivers
branch
from
45c8c0c to
e51af02
Compare
|
Files not tested by internal CI. |
Error-path and bounds fixes across three DMA drivers:
dma->chanafter freeing it on a partial-probe failure, soa later probe retry doesn't free a dangling pointer
successfully (re)allocated; otherwise a stale count with a NULL buffer
leads to a later
bzero(NULL, count * size)>->>=) in boththe enable and disable paths