Sanity check available_fmt->input_pin_fmts and available_fmt->output_pin_fmts before using them #4190
Conversation
libinyang
requested review from
ajitkupandey,
bardliao,
dbaluta,
kuanhsuncheng,
kv2019i,
lgirdwood,
plbossart,
ranj063,
ujfalusi and
yaochunhung
as code owners
libinyang
changed the title
sound/soc/sof/ipc4-topology.c
Outdated
There was a problem hiding this comment.
Why not group all the available_fmt->input_pin_fmts under one if? Along with the dev_dbg() few lines up?
sound/soc/sof/ipc4-topology.c
Outdated
There was a problem hiding this comment.
I think I have asked this in the original PR, but the comment got lost @ranj063:
Let's say we have a module with 2 input and 1 output pin.
sof_ipc4_init_audio_fmt() is called with the input_pin_fmts and num_input_formats, we find the second pin_fmt as a match (i=1).
output_pin_fmts is allocated for one pin, no? We access data outside (output_pin_fmts[1])?
There was a problem hiding this comment.
We have 2 set of params: num_input_pins/num_output_pins and num_input_formats/num_output_formats. For WoV, there is no output pin. So it means, the num_output_pins and num_output_formats should be 0. In current WoV tplg, I set num_output_pins = 0 and num_output_formats = 1. This can work without this PR. (num_output_formats = 1)
However, I still think we need this patch for the sanity check in case someone set the num_input_formats or num_output_formats to 0 in tplg, which will cause kernel panic. Kernel should handle such exception.
sound/soc/sof/ipc4-topology.c
Outdated
There was a problem hiding this comment.
@libinyang, if there is no output at all, then obs might be better to set to 0?
There was a problem hiding this comment.
@libinyang, if there is no output at all, then obs might be better to set to 0?
The default value is 0 as we are using kzalloc()
There was a problem hiding this comment.
right, and what if we have 1 input pin and the i is 2? What if we have 1 output pin and the i is 2?
Signed-off-by: Libin Yang <libin.yang@intel.com>
libinyang
force-pushed
the
fixup_check_NULL
branch
from
40cf30b to
6fc388f
Compare
|
PR is updated based on @ujfalusi 's comments. |
| return -EINVAL; | ||
| } | ||
|
|
||
| /* copy input format */ |
There was a problem hiding this comment.
@libinyang, the commit subject and message is not too verbose...
| sof_ipc4_dbg_audio_format(sdev->dev, &available_fmt->input_pin_fmts[i], 1); | ||
| /* set base_cfg obs */ | ||
| if (available_fmt->output_pin_fmts) | ||
| base_config->obs = available_fmt->output_pin_fmts[i].buffer_size; |
There was a problem hiding this comment.
Non existent output_pin_fmts does not warrant similar print as the input_pin_fmts?
| dev_dbg(sdev->dev, "init input audio formats for %s\n", swidget->widget->name); | ||
| sof_ipc4_dbg_audio_format(sdev->dev, &available_fmt->input_pin_fmts[i], 1); | ||
| } else { | ||
| dev_dbg(sdev->dev, "No init input audio formats for %s\n", swidget->widget->name); |
There was a problem hiding this comment.
this is printed when no input pin is defined, sure it means that there is no input format, but...
There was a problem hiding this comment.
I add this printed info in case some one will check the input audio formats. So he can easily use grep "init input audio formats" to get all input formats (also for this reason, I changed "Init" to "init" to print the same keyword in 2 conditions). Or we don't print anything for no input audio format scenario?
| sizeof(struct sof_ipc4_audio_format)); | ||
| if (available_fmt->output_pin_fmts) | ||
| memcpy(&process->output_format, &available_fmt->output_pin_fmts[ret].audio_fmt, | ||
| sizeof(struct sof_ipc4_audio_format)); |
There was a problem hiding this comment.
let's say you got a match with the 2nd input format in sof_ipc4_init_audio_fmt(), that will make ret==1, let's say you have one output pin. available_fmt->output_pin_fmts[1] is out of bound.
There was a problem hiding this comment.
let's say you got a match with the 2nd input format in
sof_ipc4_init_audio_fmt(), that will make ret==1, let's say you have one output pin.available_fmt->output_pin_fmts[1]is out of bound.
Yes, I mentioned a similar question in #4163 (comment). The output is not 1:1 mapping to input. Sometimes input number may be larger than output number. This may cause issue. What I understand is we will limit it in tplg based on @ranj063 's comments?
There was a problem hiding this comment.
As @ujfalusi raise this issue, I still want to recommend that we can add an index in the format structure. Let say:
struct sof_ipc4_pin_format {
u32 pin_index;
+ u32 format_index;
u32 buffer_size;
struct sof_ipc4_audio_format audio_fmt;
};
When there is output format mapped to an input format, they have the same format_index. So we don't use ret as the output fmt idx. This can help us to avoid such out of bound accessing.
|
@libinyang @ujfalusi i've added the fixups in my PR #4185. Lets continue the discussion there |
|
Close this PR as Ranjani merged this fix in PR #4185. |
3 participants
…format count
Sanity check available_fmt->input_pin_fmts and available_fmt->output_pin_fmts before using them as some modules may not have input or output pins.
Signed-off-by: Libin Yang libin.yang@intel.com