SoC: SOF: Fix control data bounds checking and TOCTOU vulnerabilities#5772
Open
ujfalusi wants to merge 5 commits into
Open
SoC: SOF: Fix control data bounds checking and TOCTOU vulnerabilities#5772ujfalusi wants to merge 5 commits into
ujfalusi wants to merge 5 commits into
Conversation
In sof_ipc4_bytes_put(), the copy size is derived from the old data->size in the buffer rather than the incoming new data's size field from ucontrol. If the new data has a different size, the copy uses the wrong length: it may truncate valid data or copy stale bytes. Fix by validating and using the incoming data's sof_abi_hdr.size from ucontrol before copying. Fixes: a062c88 ("ASoC: SOF: ipc4-control: Add support for bytes control get and put") Signed-off-by: Peter Ujfalusi <[email protected]>
In sof_ipc3_control_update(), the expected_size calculation uses firmware-provided cdata->num_elems in arithmetic that could overflow on 32-bit platforms, wrapping to a small value. This would allow the cdata->rhdr.hdr.size comparison to pass with mismatched sizes, potentially leading to out-of-bounds access in snd_sof_update_control. Use check_mul_overflow() and check_add_overflow() to detect and reject overflowed size calculations. Fixes: 10f461d ("ASoC: SOF: Add IPC3 topology control ops") Signed-off-by: Peter Ujfalusi <[email protected]>
In snd_sof_update_control(), firmware-provided cdata->num_elems is checked against local_cdata->data->size but never against the actual allocation size. If local_cdata->data->size was previously set to an inconsistent value, the memcpy could write past the allocated buffer. Add a bounds check to ensure num_elems fits within the available space in the ipc_control_data allocation before copying. Fixes: 10f461d ("ASoC: SOF: Add IPC3 topology control ops") Signed-off-by: Peter Ujfalusi <[email protected]>
In sof_ipc3_bytes_put(), the size used for the memcpy is derived from the old data->size already in the buffer, not the incoming new data's size field. If the new data has a different size, the copy length is wrong: it may truncate valid data or copy stale bytes. Similarly, sof_ipc3_bytes_get() checks data->size against max_size without accounting for the sizeof(struct sof_ipc_ctrl_data) offset of the flex array within the allocation. Fix bytes_put to validate and use the incoming data's sof_abi_hdr.size from ucontrol before copying. Fix bytes_get to subtract sizeof(*cdata) from the bounds check to match the actual available space. Fixes: 544ac88 ("ASoC: SOF: Add bytes_get/put control IPC ops for IPC3") Signed-off-by: Peter Ujfalusi <[email protected]>
The ipc_control_data buffer is allocated as kzalloc(max_size), where max_size covers the entire struct sof_ipc_ctrl_data including its flexible array payload. However, the bounds checks in bytes_ext_put and _bytes_ext_get compared user data lengths against max_size directly, ignoring that cdata->data sits at an offset of sizeof(struct sof_ipc_ctrl_data) bytes into the allocation. This allowed writing up to sizeof(struct sof_ipc_ctrl_data) bytes past the end of the heap buffer from unprivileged userspace via the ALSA TLV kcontrol interface, and similarly allowed over-reading adjacent heap data on the get path. Fix all bounds checks to subtract sizeof(*cdata) from max_size so they reflect the actual space available at the cdata->data offset. Also fix the error-path restore in bytes_ext_put which wrote to cdata->data instead of cdata, causing the same overflow. Fixes: 67ec2a0 ("ASoC: SOF: Add bytes_ext control IPC ops for IPC3") Signed-off-by: Peter Ujfalusi <[email protected]>
ujfalusi
requested review from
bardliao,
dbaluta,
kv2019i,
lgirdwood,
plbossart and
ranj063
as code owners
Collaborator
Author
|
this PR contains the patch from the now closed #5769 |
There was a problem hiding this comment.
Pull request overview
This PR hardens the SOF IPC3 and IPC4 bytes/bytes_ext kcontrol put/get paths by fixing size/bounds validations to prevent heap overflows and TOCTOU-style issues when handling control binary payloads from userspace and firmware notifications.
Changes:
- Validate incoming userspace ABI header sizes (use the new header, and account for
struct sof_ipc_ctrl_dataallocation offsets in IPC3). - Tighten TLV bytes_ext size checks (including minimum ABI header size) and fix restore-path memcpy destination.
- Add overflow-safe expected-size computation for IPC3 control notifications and add additional bounds checks for binary updates.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sound/soc/sof/ipc4-control.c | Uses the incoming ABI header for size validation before copying/sending bytes control data (IPC4). |
| sound/soc/sof/ipc3-control.c | Adjusts bounds checks to reflect actual allocations, hardens TLV bytes_ext handling, fixes restore copy target, and adds overflow-aware notification sizing and additional bounds checks (IPC3). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
446
to
450
| /* be->max has been verified to be >= sizeof(struct sof_abi_hdr) */ | ||
| if (cdata->data->size > scontrol->max_size - sizeof(struct sof_abi_hdr)) { | ||
| if (cdata->data->size > scontrol->max_size - sizeof(*cdata) - sizeof(struct sof_abi_hdr)) { | ||
| dev_err_ratelimited(scomp->dev, "Mismatch in ABI data size (truncated?)\n"); | ||
| goto err_restore; | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
OK, I will change the check to this.
Comment on lines
+564
to
568
| return; | ||
| } | ||
|
|
||
| /* copy the new binary data */ | ||
| memcpy(local_cdata->data, cdata->data, cdata->num_elems); |
Collaborator
Author
There was a problem hiding this comment.
Incorrect. In the binary path, cdata->num_elems is the total binary blob size copied from cdata->data (the sof_abi_hdr start). The existing num_elems == local_cdata->data->size check is how the original code has always worked — num_elems includes the ABI header bytes in this context.
lgirdwood
approved these changes
May 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This series fixes several security issues in the SOF IPC3 and IPC4
control data handling paths, found during a security audit of the
bytes/bytes_ext kcontrol put/get operations.
The most critical issue is a heap buffer overflow reachable from
unprivileged userspace via the ALSA TLV kcontrol interface: bounds
checks in bytes_ext_put/get compared user data lengths against
max_size without accounting for the sizeof(struct sof_ipc_ctrl_data)
offset of the flex array within the allocation.
The remaining patches fix TOCTOU issues where the copy size was derived
from stale buffer contents rather than the incoming data, missing
bounds validation of firmware-provided num_elems against the actual
allocation size, and potential integer overflows in size calculations
on 32-bit platforms.