Skip to content

audio: tdfb: Improve robustness for invalid configuration #10877

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

Merged
lgirdwood merged 1 commit into from
Jun 12, 2026
+44 −18
Merged

audio: tdfb: Improve robustness for invalid configuration #10877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 36 additions & 15 deletions src/audio/tdfb/tdfb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,11 @@ static int tdfb_init_coef(struct processing_module *mod, int source_nch,
int i;

/* Sanity checks */
if (config->size != cd->config_size) {
comp_err(dev, "Incorrect configuration blob size");
return -EINVAL;
}
Comment on lines +326 to +329

@singalsu singalsu Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Since the blobs are created with a tool (not manually) the size mismatch is very unlikely. No need for more details.


if (config->num_output_channels > PLATFORM_MAX_CHANNELS ||
!config->num_output_channels) {
comp_err(dev, "invalid num_output_channels %d",
Expand All @@ -342,12 +347,21 @@ static int tdfb_init_coef(struct processing_module *mod, int source_nch,
return -EINVAL;
}

if (config->num_angles > SOF_TDFB_MAX_ANGLES) {
/* In SOF v1.6 - 1.8 based beamformer topologies the multiple angles, mic locations,
* and beam on/off switch were not defined. A most basic supported blob has num_angles
* equal to 1. Mic locations data is optional.
*/
if (config->num_angles == 0 || config->num_angles > SOF_TDFB_MAX_ANGLES) {
comp_err(dev, "invalid num_angles %d",
config->num_angles);
return -EINVAL;
}

if (!config->angle_enum_mult) {
Comment thread
singalsu marked this conversation as resolved.
comp_err(dev, "invalid angle_enum_mult");
return -EINVAL;
}
Comment on lines +360 to +363

@singalsu singalsu Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

With negative multiplier it's possible to generate a descending angle scale. It could be useful, no need to limit the usage. The angles are negative and positive any way via the angle offset parameter.


if (config->beam_off_defined > 1) {
comp_err(dev, "invalid beam_off_defined %d",
config->beam_off_defined);
Expand All @@ -360,15 +374,6 @@ static int tdfb_init_coef(struct processing_module *mod, int source_nch,
return -EINVAL;
}

/* In SOF v1.6 - 1.8 based beamformer topologies the multiple angles, mic locations,
* and beam on/off switch were not defined. Return error if such configuration is seen.
* A most basic blob has num_angles equals 1. Mic locations data is optional.
*/
if (config->num_angles == 0 && config->num_mic_locations == 0) {
comp_err(dev, "ABI version less than 3.19.1 is not supported.");
return -EINVAL;
}

/* Skip filter coefficients */
num_filters = config->num_filters * (config->num_angles + config->beam_off_defined);
coefp = tdfb_filter_seek(config, num_filters);
Expand Down Expand Up @@ -423,6 +428,12 @@ static int tdfb_init_coef(struct processing_module *mod, int source_nch,
cd->filter_angles[min_delta_idx].azimuth, idx);
}

if (idx < 0 || idx + (int)config->num_filters > num_filters) {
comp_err(dev, "invalid filter_index %d for angle %d",
idx, cd->filter_angles[min_delta_idx].azimuth);
return -EINVAL;
}

/* Seek to proper filter for requested angle or beam off configuration */
coefp = tdfb_filter_seek(config, idx);

Expand Down Expand Up @@ -451,6 +462,11 @@ static int tdfb_init_coef(struct processing_module *mod, int source_nch,
for (i = 0; i < config->num_filters; i++) {
if (cd->input_channel_select[i] > max_ch)
max_ch = cd->input_channel_select[i];

if (cd->input_channel_select[i] < 0) {
comp_err(dev, "invalid channel select for filter %d", i);
return -EINVAL;
}
}

/* The stream must contain at least the number of channels that is
Expand Down Expand Up @@ -641,7 +657,12 @@ static int tdfb_process(struct processing_module *mod,

/* Check for changed configuration */
if (comp_is_new_data_blob_available(cd->model_handler)) {
cd->config = comp_get_data_blob(cd->model_handler, NULL, NULL);
cd->config = comp_get_data_blob(cd->model_handler, &cd->config_size, NULL);
if (!cd->config || cd->config_size < sizeof(*cd->config) ||
cd->config_size > SOF_TDFB_MAX_SIZE) {
comp_err(dev, "invalid configuration blob, size %zu", cd->config_size);
return -EINVAL;
}
Comment on lines 659 to +665

@singalsu singalsu Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I propose to add the blob validator later. I have started the work but it won't be ready before summer vacations that this PR targets for.

ret = tdfb_setup(mod, audio_stream_get_channels(source),
audio_stream_get_channels(sink),
audio_stream_get_frm_fmt(source));
Expand Down Expand Up @@ -705,7 +726,6 @@ static int tdfb_prepare(struct processing_module *mod,
struct comp_buffer *sourceb, *sinkb;
struct comp_dev *dev = mod->dev;
enum sof_ipc_frame frame_fmt;
size_t data_size;
int source_channels;
int sink_channels;
int rate;
Expand Down Expand Up @@ -735,9 +755,10 @@ static int tdfb_prepare(struct processing_module *mod,
rate = audio_stream_get_rate(&sourceb->stream);

/* Initialize filter */
cd->config = comp_get_data_blob(cd->model_handler, &data_size, NULL);
if (!cd->config || !data_size) {
comp_err(dev, "Missing a configuration blob.");
cd->config = comp_get_data_blob(cd->model_handler, &cd->config_size, NULL);
if (!cd->config || cd->config_size < sizeof(*cd->config) ||
cd->config_size > SOF_TDFB_MAX_SIZE) {
comp_err(dev, "invalid configuration blob, size %zu", cd->config_size);
ret = -EINVAL;
goto out;
}
Expand Down
5 changes: 3 additions & 2 deletions src/audio/tdfb/tdfb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,17 @@
#ifndef __USER_TDFB_H__
#define __USER_TDFB_H__

#include <sof/platform.h>
#include <stdint.h>

#define SOF_TDFB_NUM_INPUT_PINS 1 /* One source */
#define SOF_TDFB_NUM_OUTPUT_PINS 1 /* One sink */
#define SOF_TDFB_MAX_SIZE 4096 /* Max size for coef data in bytes */
#define SOF_TDFB_MAX_SIZE 16384 /* Max size for coef data in bytes */
#define SOF_TDFB_FIR_MAX_LENGTH 256 /* Max length for individual filter */
#define SOF_TDFB_FIR_MAX_COUNT 16 /* A blob can define max 16 FIR EQs */
#define SOF_TDFB_MAX_STREAMS 8 /* Support 1..8 sinks */
#define SOF_TDFB_MAX_ANGLES 360 /* Up to 1 degree precision for 360 degrees coverage */
#define SOF_TDFB_MAX_MICROPHONES 16 /* Up to 16 microphone locations */
#define SOF_TDFB_MAX_MICROPHONES PLATFORM_MAX_CHANNELS /* Bounded by direction state arrays */

/* The driver assigns running numbers for control index. If there's single control of
* type switch, enum, binary they all have index 0.
Expand Down
1 change: 1 addition & 0 deletions src/audio/tdfb/tdfb_comp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ struct tdfb_comp_data {
int16_t *output_stream_mix; /**< for each FIR define stream */
int16_t az_value; /**< beam steer azimuth as in control enum */
int16_t az_value_estimate; /**< beam steer azimuth as in control enum */
size_t config_size; /**< size of the configuration blob */
size_t fir_delay_size; /**< allocated size */
unsigned int max_frames; /**< max frames to process */
bool direction_updates:1; /**< set true if direction angle control is updated */
Expand Down
2 changes: 1 addition & 1 deletion src/audio/tdfb/tdfb_direction.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ static int16_t distance_from_source(struct tdfb_comp_data *cd, int mic_n,

static void theoretical_time_differences(struct tdfb_comp_data *cd, int16_t az)
{
int16_t d[PLATFORM_MAX_CHANNELS];
int16_t d[SOF_TDFB_MAX_MICROPHONES];
Comment thread
singalsu marked this conversation as resolved.
int16_t src_x;
int16_t src_y;
int16_t sin_az;
Expand Down
3 changes: 3 additions & 0 deletions src/audio/tdfb/tdfb_ipc3.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ static int tdfb_cmd_get_value(struct processing_module *mod, struct sof_ipc_ctrl
{
struct tdfb_comp_data *cd = module_get_private_data(mod);

if (cdata->num_elems == 0 || cdata->num_elems > SOF_IPC_MAX_CHANNELS)
return -EINVAL;

@ujfalusi ujfalusi Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

only bytes controls will have num_elems as 0 as the hos does not know the size, for any other controls the host sets num_elems: data->num_elems = scontrol->num_channels;, the num_channels comes from topology and only checked against SND_SOC_TPLG_MAX_CHAN, no check for 0 anywhere, not even in core, a control is allowed to have 0 channels?

@singalsu singalsu Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm not even sure the num_elems strictly means number of stream channels. It might be e.g. graphical EQ bands number that could be larger than max. channels count, e.g. 10 vs. 8. Can the cdata buffer allocation be too small for the num_elems ever?

@ujfalusi ujfalusi Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it is allocated based on the num_channels:
https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/sof/ipc4-control.c#L214

@singalsu singalsu Jun 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

With current implementation of the control the components generally can decide the usage, the control can be per channel, or as "mono" a control for all channels, or with non-associated to channels something else like the graphical EQ bands controls. In this case with the current IIR implementation, the SOF_IPC_MAX_CHANNELS check isn't limiting any functionality so it can be kept.

@lgirdwood lgirdwood Jun 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

atm, lets keep the validation in the module for valid channel count in kcontrols as they will be different.


switch (cdata->cmd) {
case SOF_CTRL_CMD_ENUM:
comp_dbg(mod->dev, "SOF_CTRL_CMD_ENUM index=%d",
Expand Down
Loading