ipc4: helper: bound host TLV length in DMA config walk#10915
ipc4: helper: bound host TLV length in DMA config walk#10915abonislawski wants to merge 1 commit into
Conversation
Reject a gateway-config TLV whose length overruns the buffer so the uintptr_t walk cannot wrap past the low address space. Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>
abonislawski
requested review from
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
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.
Hardens the DMA config TLV walk against malformed host TLVs by bounding TLV length checks so pointer arithmetic can’t wrap and walk out of the provided buffer.
Changes:
- Switches end-address arithmetic and comparisons to
uintptr_tfor correct pointer-width handling. - Adds a per-TLV bounds check to reject TLVs whose declared length would overrun the buffer and potentially break
tlv_next().
| uint32_t size, uint32_t device_id, int dma_cfg_idx) | ||
| { | ||
| uint32_t end_addr = (uint32_t)data_buffer + size; | ||
| uintptr_t end_addr = (uintptr_t)data_buffer + size; |
There was a problem hiding this comment.
I do not really follow what Copilot is trying to suggest here. If the end_addr wraps, then the loop bellow will end immediately and no harm is done. The code of course does not work and function would return an error, but that is correct behaviour since the arguments are invalid.
There was a problem hiding this comment.
You are probably right. Theoretically, if sizeof(uint32_t) > sizeof(uintptr_t), the the operation could wrap but still yield a value greater than the buffer's beginning. But this doesn't explain why Copilot thinks that skipping the loop won't return an error.
There was a problem hiding this comment.
Btw. Perhaps it would be better to size_t instead of uint32_t for pointer arithmetic.
| uint32_t size, uint32_t device_id, int dma_cfg_idx) | ||
| { | ||
| uint32_t end_addr = (uint32_t)data_buffer + size; | ||
| uintptr_t end_addr = (uintptr_t)data_buffer + size; |
There was a problem hiding this comment.
I do not really follow what Copilot is trying to suggest here. If the end_addr wraps, then the loop bellow will end immediately and no harm is done. The code of course does not work and function would return an error, but that is correct behaviour since the arguments are invalid.
Reject a gateway-config TLV whose length overruns the buffer so the uintptr_t walk cannot wrap past the low address space.