Audio: Module adapter: Fix in IPC4 invalid blob pass to init() #9501
Conversation
src/audio/crossover/crossover.c
Outdated
| @@ -297,7 +297,7 @@ static int crossover_init(struct processing_module *mod) | |||
| struct comp_dev *dev = mod->dev; | |||
There was a problem hiding this comment.
Fix patch title, not just IIR.
There was a problem hiding this comment.
@singalsu instead of modifying all the components, would it make sense to change the cfg->size to subtract the sizeof(struct ipc4_base_module_cfg) here?
dst->size = cfgsz - sizeof(cfg->base_cfg);
There was a problem hiding this comment.
This would be simplest, good idea! But I'm not able to test all the components, there's several without testbench or cmocka. I recall some components use the extended base module config, for pins. Need to check those can work correctly with this.
There was a problem hiding this comment.
I checked init() of every component. It looks like selector and src components need for their init() function compensation of sizeof(cfg->base_cfg) subtract from size but it's smaller than changing every component so I'll do it that way.
singalsu
force-pushed
the
fix_audio_module_blob_ipc4_init
branch
from
f8bff98
to
7839da7
Compare
singalsu
changed the title
The cfg->size is the size of ipc4_base_module_cfg. The cfg->data is NULL but when such is encountered the comp_init_data_blob() allocates a zero bytes of size. Such blob is illegal and e.g. IIR will fail if not another blob is received by prepare(). DC block operates, but applies a rather high cut-off frequency that results to silent audio for e.g. lower voice frequencies. What happens with such illegal blob is component specific. The expected operation for most components is pass-through when a configuration blob is not set. This change should ensure that the component is not initialized with incorrect bytes control data if such is not passed with topology. IPC4 SRC utilizes in init() an ipc4_base_module_cfg variant that adds an uint32_t after it for sink rate. To pass the check for cfg->size the size subtract done in module adapter need to be taken into account. TODO: There is similar size check in selector, need to make similar change there. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
singalsu
force-pushed
the
fix_audio_module_blob_ipc4_init
branch
from
7839da7
to
ccaf68b
Compare
| @@ -48,7 +48,7 @@ int module_adapter_init_data(struct comp_dev *dev, | |||
| return -EINVAL; | |||
|
|
|||
| dst->base_cfg = cfg->base_cfg; | |||
| dst->size = cfgsz; | |||
| dst->size = cfgsz - sizeof(cfg->base_cfg); | |||
There was a problem hiding this comment.
@singalsu it's probably worth while having a generic accessor macro in the module API header so that IPC4 module data can be easily extracted
There was a problem hiding this comment.
I think I'm missing still cases when there's the extended config with pins appended (set in rimage for some components). I'm especially not happy with SRC component code change below. The cfg_size_expect gets value 4, from subtract 44 - 40. Also I'm at the moment testing a way to pass in testbench ipc4 build the UUID to avoid the uuid -> rimage id -> uuid translations with normal firmware. So there could be even more varying init ipc sizes.
Do you have ideas how to do this in a nicer way?
The cfg->size is the size of ipc4_base_module_cfg. The cfg->data is NULL but when such is encountered the comp_init_data_blob() allocates a zero bytes of size.
Such blob is illegal and e.g. IIR will fail if not another blob is received by prepare(). DC block operates,
but applies a rather high cut-off frequency that results to silent audio for e.g. lower voice frequencies. What happens with such illegal blob is component specific.
The expected operation for most components is pass-through when a configuration blob is not set. This change should ensure that the component is not initialized with incorrect bytes control data if such is not passed with topology.