Skip to content

ipc4: base_fw: fix OOB reads and input validation in LargeConfig handlers #10757

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

Open
tmleman wants to merge 3 commits into
base: main
Choose a base branch
from
+67 −14
Open

ipc4: base_fw: fix OOB reads and input validation in LargeConfig handlers #10757

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
d7ac1e4
base_fw: libraries_info_get: fix sizeof(pointer) and index confusion
tmleman May 8, 2026
b378052
base_fw: validate IPC payload size before dereference in set handlers
tmleman May 8, 2026
d61abab
ipc4: basefw_set_fw_config: validate TLV size
tmleman May 8, 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
47 changes: 37 additions & 10 deletions src/audio/base_fw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,11 @@ __cold static int basefw_register_kcps(bool first_block, bool last_block,
return IPC4_ERROR_INVALID_PARAM;

#if CONFIG_KCPS_DYNAMIC_CLOCK_CONTROL
if (data_offset_or_size < sizeof(int32_t)) {
tr_err(&ipc_tr, "basefw_register_kcps: payload too small: %u", data_offset_or_size);
return IPC4_ERROR_INVALID_PARAM;
}

/* value of kcps to request on core 0. Can be negative */
if (core_kcps_adjust(0, *(int32_t *)data))
return IPC4_ERROR_INVALID_PARAM;
Expand Down Expand Up @@ -353,6 +358,12 @@ __cold static int basefw_resource_allocation_request(bool first_block, bool last
if (!(first_block && last_block))
return IPC4_ERROR_INVALID_PARAM;

if (data_offset_or_size < sizeof(struct ipc4_resource_request)) {
tr_err(&ipc_tr, "basefw_resource_allocation_request: payload too small: %u < %zu",
data_offset_or_size, sizeof(struct ipc4_resource_request));
return IPC4_ERROR_INVALID_PARAM;
}

request = (struct ipc4_resource_request *)data;

switch (request->ra_type) {
Expand Down Expand Up @@ -420,7 +431,7 @@ __cold static int basefw_libraries_info_get(uint32_t *data_offset, char *data)
if (!desc)
continue;

libs_info->libraries[lib_id].id = lib_id;
libs_info->libraries[lib_counter].id = lib_id;
memcpy_s(libs_info->libraries[lib_counter].name,
SOF_MAN_FW_HDR_FW_NAME_LEN, desc->header.name, sizeof(desc->header.name));
libs_info->libraries[lib_counter].major_version =
Expand All @@ -439,7 +450,7 @@ __cold static int basefw_libraries_info_get(uint32_t *data_offset, char *data)

libs_info->library_count = lib_counter;
*data_offset =
sizeof(libs_info) + libs_info->library_count * sizeof(libs_info->libraries[0]);
sizeof(*libs_info) + libs_info->library_count * sizeof(libs_info->libraries[0]);
Copy link
Copy Markdown
Collaborator

@lyakh lyakh May 11, 2026

Choose a reason for hiding this comment

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

I suppose this IPC4_LIBRARIES_INFO_GET IPC has never been used until now


return IPC4_SUCCESS;
}
Expand Down Expand Up @@ -518,12 +529,17 @@ __cold static int basefw_pipeline_list_info_get(uint32_t *data_offset, char *dat
return IPC4_SUCCESS;
}

__cold int set_perf_meas_state(const char *data)
__cold int set_perf_meas_state(uint32_t data_size, const char *data)
Copy link
Copy Markdown
Collaborator

@lyakh lyakh May 11, 2026

Choose a reason for hiding this comment

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

maybe change the type in these functions to const uint8_t *

{
assert_can_be_cold();

#ifdef CONFIG_SOF_TELEMETRY
enum ipc4_perf_measurements_state_set state = *data;
if (data_size < sizeof(uint8_t)) {
Copy link
Copy Markdown
Collaborator

@lyakh lyakh May 11, 2026

Choose a reason for hiding this comment

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

and then use sizeof(*data) or sizeof(data[0])

tr_err(&ipc_tr, "set_perf_meas_state: payload too small: %u", data_size);
return IPC4_ERROR_INVALID_PARAM;
}

enum ipc4_perf_measurements_state_set state = *(const uint8_t *)data;
Copy link
Copy Markdown
Collaborator

@kv2019i kv2019i May 11, 2026

Choose a reason for hiding this comment

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

And is this right, forced cast to uint8_t and then enum? OTOH, the old code is suspect too, this is assuming stuff about "enum" that I don't think can be assumed.


switch (state) {
case IPC4_PERF_MEASUREMENTS_DISABLED:
Expand Down Expand Up @@ -624,12 +640,17 @@ __cold static int io_global_perf_data_get(uint32_t *data_off_size, char *data)
#endif
}

__cold static int io_perf_monitor_state_set(const char *data)
__cold static int io_perf_monitor_state_set(uint32_t data_size, const char *data)
{
assert_can_be_cold();

#ifdef CONFIG_SOF_TELEMETRY_IO_PERFORMANCE_MEASUREMENTS
return io_perf_monitor_set_state((enum ipc4_perf_measurements_state_set)*data);
if (data_size < sizeof(uint8_t)) {
tr_err(&ipc_tr, "io_perf_monitor_state_set: payload too small: %u", data_size);
return IPC4_ERROR_INVALID_PARAM;
}

return io_perf_monitor_set_state((enum ipc4_perf_measurements_state_set)*(const uint8_t *)data);
#else
return IPC4_UNAVAILABLE;
#endif
Expand Down Expand Up @@ -697,12 +718,18 @@ __cold static int basefw_get_large_config(struct comp_dev *dev, uint32_t param_i
data_offset, data);
};

__cold static int basefw_notification_mask_info(const void *data)
__cold static int basefw_notification_mask_info(uint32_t data_size, const void *data)
{
const struct ipc4_notification_mask_info *mask_info = data;

assert_can_be_cold();

if (data_size < sizeof(struct ipc4_notification_mask_info)) {
tr_err(&ipc_tr, "basefw_notification_mask_info: payload too small: %u < %zu",
data_size, sizeof(struct ipc4_notification_mask_info));
return IPC4_ERROR_INVALID_PARAM;
}

ipc4_update_notification_mask(mask_info->ntfy_mask, mask_info->enabled_mask);

return IPC4_SUCCESS;
Expand Down Expand Up @@ -770,15 +797,15 @@ __cold static int basefw_set_large_config(struct comp_dev *dev, uint32_t param_i

switch (param_id) {
case IPC4_NOTIFICATION_MASK:
return basefw_notification_mask_info(data);
return basefw_notification_mask_info(data_offset, data);
case IPC4_ASTATE_TABLE:
return basefw_astate_table();
case IPC4_DMA_CONTROL:
return basefw_dma_control(first_block, last_block, data_offset, data);
case IPC4_PERF_MEASUREMENTS_STATE:
return set_perf_meas_state(data);
return set_perf_meas_state(data_offset, data);
case IPC4_IO_PERF_MEASUREMENTS_STATE:
return io_perf_monitor_state_set(data);
return io_perf_monitor_state_set(data_offset, data);
case IPC4_SYSTEM_TIME:
return basefw_set_system_time(param_id, first_block,
last_block, data_offset, data);
Expand Down
34 changes: 30 additions & 4 deletions src/audio/base_fw_intel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,17 +470,37 @@ __cold static int fw_config_set_force_l1_exit(const struct sof_tlv *tlv)
__cold static int basefw_set_fw_config(bool first_block, bool last_block,
uint32_t data_offset, const char *data)
{
assert_can_be_cold();

/* Validate minimum TLV header (type + length fields) is present */
if (data_offset < sizeof(struct sof_tlv)) {
tr_err(&basefw_comp_tr, "FW_CONFIG payload too small: %u < %zu",
data_offset, sizeof(struct sof_tlv));
return IPC4_INVALID_CONFIG_DATA_LEN;
}

const struct sof_tlv *tlv = (const struct sof_tlv *)data;

assert_can_be_cold();
/* Validate the TLV value payload fits within the reported buffer size */
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);
Comment on lines +485 to +487
return IPC4_INVALID_CONFIG_DATA_LEN;
}

switch (tlv->type) {
case IPC4_DMI_FORCE_L1_EXIT:
if (tlv->length < sizeof(uint32_t)) {
tr_err(&basefw_comp_tr, "DMI_FORCE_L1_EXIT value too small: %u",
tlv->length);
return IPC4_INVALID_CONFIG_DATA_LEN;
}
return fw_config_set_force_l1_exit(tlv);
default:
break;
}
tr_warn(&basefw_comp_tr, "returning success for Set FW_CONFIG without handling it");

tr_warn(&basefw_comp_tr, "Set FW_CONFIG: no handler for type %u", tlv->type);
Copy link
Copy Markdown
Collaborator

@kv2019i kv2019i May 11, 2026

Choose a reason for hiding this comment

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

If you add a linefeed above, I'd keep one empty line between function body and return to conform with codebase style.

return 0;
}

Expand All @@ -502,9 +522,15 @@ static int basefw_mic_priv_state_changed(bool first_block,
const char *data)
{
#if CONFIG_INTEL_ADSP_MIC_PRIVACY
tr_info(&basefw_comp_tr, "state changed to %d", *data);
if (data_offset_or_size < sizeof(uint8_t)) {
tr_err(&basefw_comp_tr, "mic_priv_state_changed: payload too small: %u",
data_offset_or_size);
return IPC4_ERROR_INVALID_PARAM;
}

uint32_t mic_disable_status = (uint32_t)(uint8_t)(*data);

uint32_t mic_disable_status = (uint32_t)(*data);
tr_info(&basefw_comp_tr, "state changed to %u", mic_disable_status);
struct mic_privacy_settings settings;

mic_privacy_fill_settings(&settings, mic_disable_status);
Expand Down
Loading