ipc4: base_fw: fix OOB reads and input validation in LargeConfig handlers#10757
Open
tmleman wants to merge 3 commits intothesofproject:mainfrom
Open
ipc4: base_fw: fix OOB reads and input validation in LargeConfig handlers#10757tmleman wants to merge 3 commits intothesofproject:mainfrom
tmleman wants to merge 3 commits intothesofproject:mainfrom
Conversation
Two bugs in basefw_libraries_info_get() are fixed: 1. sizeof(pointer) misuse (CWE-467): The reply size was computed with sizeof(libs_info) where libs_info is a pointer to struct ipc4_libraries_info. On 32-bit Xtensa targets the pointer and the struct header are both 4 bytes, so the bug was latent. On 64-bit builds (host simulation, future platforms) sizeof(pointer)=8 while sizeof(struct ipc4_libraries_info)=4, causing the reported reply size to be overstated by 4 bytes and potentially leaking uninitialized buffer content to the host. Fix: use sizeof(*libs_info) to always dereference to the struct size. 2. Mixed-index confusion causing uninitialized field writes: The .id field was written using libs_info->libraries[lib_id] while all other fields (name, version, num_module_entries) were written using libs_info->libraries[lib_counter]. When any intermediate library slot is absent (desc == NULL), lib_id advances past lib_counter, so the .id write lands in a different slot than the remaining fields. The result is a reply entry with an uninitialized .id (stale buffer content returned to the host) and an orphaned write into a slot beyond library_count. Fix: use lib_counter consistently for all field writes inside the loop. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
tmleman
requested review from
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens IPC4 base firmware LargeConfig handling by fixing incorrect size calculations/indexing and adding input-size validation to prevent out-of-bounds reads when parsing host-supplied payloads.
Changes:
- Add minimum payload-size guards for several LargeConfigSet handlers before dereferencing incoming data buffers.
- Fix library info reply population (indexing bug) and correct a
sizeof(pointer)misuse in response sizing. - Add TLV header/value length validation in
FW_CONFIGhandling and improve the “unhandled type” warning message.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/audio/base_fw.c | Adds payload-size validation to multiple set handlers; fixes library info indexing and response size calculation. |
| src/audio/base_fw_intel.c | Adds TLV size validation for FW_CONFIG and input-size validation for mic privacy state change; improves warning message text. |
Comment on lines
+485
to
+487
| if (data_offset < sizeof(struct sof_tlv) + tlv->length) { | ||
| tr_err(&basefw_comp_tr, "FW_CONFIG TLV value truncated: need %zu, have %u", | ||
| sizeof(struct sof_tlv) + tlv->length, data_offset); |
Multiple LargeConfigSet handlers for module_id=0 dereferenced the payload pointer without checking the host-supplied data_off_size. When data_off_size=0 the dcache invalidation is a no-op and handlers read stale mailbox content from the previous transaction. The fix is the same pattern across all affected handlers: add a minimum-size guard against the reported payload length before the first memory access, and return IPC4_ERROR_INVALID_PARAM on undersize input. Three handlers (set_perf_meas_state, io_perf_monitor_state_set, basefw_notification_mask_info) also had data_offset dropped by the dispatcher; those call sites are updated to pass it through. An additional sign-extension defect in basefw_mic_priv_state_changed() is fixed by casting *data through uint8_t before widening to uint32_t. Affected handlers and their minimum required sizes: set_perf_meas_state 1 byte (enum state) io_perf_monitor_state_set 1 byte (enum state) basefw_register_kcps 4 bytes (int32_t kcps) basefw_notification_mask_info 8 bytes (ipc4_notification_mask_info) basefw_resource_allocation_request 8 bytes (ipc4_resource_request) basefw_mic_priv_state_changed 1 byte (uint8_t, mic privacy only) Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
basefw_set_fw_config() cast the IPC payload directly to struct sof_tlv* and read tlv->type without checking that the reported payload length (data_offset) was sufficient. A zero-length or undersized LargeConfigSet with param_id=IPC4_FW_CONFIG could cause the handler to access memory beyond the valid payload (CWE-125 / CWE-20). Three guards are added before any TLV field access: 1. data_offset < sizeof(struct sof_tlv) Rejects payloads too short to hold the type+length header (8 B). 2. data_offset < sizeof(struct sof_tlv) + tlv->length Rejects payloads where the declared value length exceeds the actual buffer, preventing OOB reads of tlv->value[]. 3. tlv->length < sizeof(uint32_t) for IPC4_DMI_FORCE_L1_EXIT Rejects a TLV whose value field is too small to contain the force flag read by fw_config_set_force_l1_exit(). The TLV pointer cast is moved to after the header-size check so it is never formed against an undersized buffer. The warning log for unhandled types is updated to include the type value to aid diagnostics. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
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.
Three bugs in the IPC4 base firmware LargeConfig handlers found during a static analysis audit (CodeQL, cppcheck, semgrep).
The first commit fixes a sizeof(pointer) misuse (CWE-467) and a mixed-index bug causing uninitialized fields in the library info reply.
The second commit adds minimum payload size guards to six LargeConfigSet handlers that dereferenced the IPC payload without checking the host-supplied data_off_size (CWE-125 / CWE-20).
The third commit validates the TLV header and value length in basefw_set_fw_config() before accessing any TLV fields (CWE-125).