| Message ID | 20220427085011.21805-1-peter.ujfalusi@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: SOF: ipc3-topology: Correct get_control_data for non bytes payload | expand |
On (22/04/27 11:50), Peter Ujfalusi wrote: > @@ -784,16 +785,23 @@ static int sof_get_control_data(struct snd_soc_component *scomp, > } > > cdata = wdata[i].control->ipc_control_data; > - wdata[i].pdata = cdata->data; > - if (!wdata[i].pdata) > - return -EINVAL; > > /* make sure data is valid - data can be updated at runtime */ > - if (widget->dobj.widget.kcontrol_type[i] == SND_SOC_TPLG_TYPE_BYTES && > - wdata[i].pdata->magic != SOF_ABI_MAGIC) > - return -EINVAL; > + if (widget->dobj.widget.kcontrol_type[i] == SND_SOC_TPLG_TYPE_BYTES) { > + if (!cdata->data) > + return -EINVAL; > + > + if (cdata->data->magic != SOF_ABI_MAGIC) > + return -EINVAL; > + > + wdata[i].pdata = cdata->data->data; > + wdata[i].pdata_size = cdata->data->size; > + } else { > + wdata[i].pdata = cdata->chanv; /* points to the control data union */ > + wdata[i].pdata_size = wdata[i].control->size; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ A question, so here wdata[i].control->size is scontrol->size = struct_size(scontrol->control_data, chanv, le32_to_cpu(mc->num_channels)); Right? If so, then is this really what we memcpy()? We memcpy() control->data->chanv and its size is `sizeof(chanv) * le32_to_cpu(mc->num_channels)`, isn't it? [..] > if (ipc_data_size) { > for (i = 0; i < widget->num_kcontrols; i++) { > + if (!wdata[i].pdata_size) > + continue; > + > + memcpy(&process->data[offset], wdata[i].pdata, > + wdata[i].pdata_size); > + offset += wdata[i].pdata_size; > } > } So should sof_get_control_data() have instead of this wdata[i].pdata_size = wdata[i].control->size; something like this wdata[i].pdata_size = wdata[i].control->size - sizeof(struct sof_ipc_ctrl_data); So that we will copy payload data, which is a bunch of chanv structs 8 bytes each.
On 27/04/2022 13:36, Sergey Senozhatsky wrote: > On (22/04/27 11:50), Peter Ujfalusi wrote: >> @@ -784,16 +785,23 @@ static int sof_get_control_data(struct snd_soc_component *scomp, >> } >> >> cdata = wdata[i].control->ipc_control_data; >> - wdata[i].pdata = cdata->data; >> - if (!wdata[i].pdata) >> - return -EINVAL; >> >> /* make sure data is valid - data can be updated at runtime */ >> - if (widget->dobj.widget.kcontrol_type[i] == SND_SOC_TPLG_TYPE_BYTES && >> - wdata[i].pdata->magic != SOF_ABI_MAGIC) >> - return -EINVAL; >> + if (widget->dobj.widget.kcontrol_type[i] == SND_SOC_TPLG_TYPE_BYTES) { >> + if (!cdata->data) >> + return -EINVAL; >> + >> + if (cdata->data->magic != SOF_ABI_MAGIC) >> + return -EINVAL; >> + >> + wdata[i].pdata = cdata->data->data; >> + wdata[i].pdata_size = cdata->data->size; >> + } else { >> + wdata[i].pdata = cdata->chanv; /* points to the control data union */ >> + wdata[i].pdata_size = wdata[i].control->size; > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > A question, so here wdata[i].control->size is > > scontrol->size = struct_size(scontrol->control_data, chanv, > le32_to_cpu(mc->num_channels)); > > Right? > > If so, then is this really what we memcpy()? We memcpy() control->data->chanv > and its size is `sizeof(chanv) * le32_to_cpu(mc->num_channels)`, isn't it? > > [..] >> if (ipc_data_size) { >> for (i = 0; i < widget->num_kcontrols; i++) { >> + if (!wdata[i].pdata_size) >> + continue; >> + >> + memcpy(&process->data[offset], wdata[i].pdata, >> + wdata[i].pdata_size); >> + offset += wdata[i].pdata_size; >> } >> } > > So should sof_get_control_data() have instead of this > > wdata[i].pdata_size = wdata[i].control->size; > > something like this > > wdata[i].pdata_size = wdata[i].control->size - sizeof(struct sof_ipc_ctrl_data); Yes, you are right. > > So that we will copy payload data, which is a bunch of chanv structs 8 > bytes each.
diff --git a/sound/soc/sof/ipc3-topology.c b/sound/soc/sof/ipc3-topology.c index 572bcbfdb356..8a083fd547ba 100644 --- a/sound/soc/sof/ipc3-topology.c +++ b/sound/soc/sof/ipc3-topology.c @@ -20,7 +20,8 @@ struct sof_widget_data { int ctrl_type; int ipc_cmd; - struct sof_abi_hdr *pdata; + void *pdata; + size_t pdata_size; struct snd_sof_control *control; }; @@ -784,16 +785,23 @@ static int sof_get_control_data(struct snd_soc_component *scomp, } cdata = wdata[i].control->ipc_control_data; - wdata[i].pdata = cdata->data; - if (!wdata[i].pdata) - return -EINVAL; /* make sure data is valid - data can be updated at runtime */ - if (widget->dobj.widget.kcontrol_type[i] == SND_SOC_TPLG_TYPE_BYTES && - wdata[i].pdata->magic != SOF_ABI_MAGIC) - return -EINVAL; + if (widget->dobj.widget.kcontrol_type[i] == SND_SOC_TPLG_TYPE_BYTES) { + if (!cdata->data) + return -EINVAL; + + if (cdata->data->magic != SOF_ABI_MAGIC) + return -EINVAL; + + wdata[i].pdata = cdata->data->data; + wdata[i].pdata_size = cdata->data->size; + } else { + wdata[i].pdata = cdata->chanv; /* points to the control data union */ + wdata[i].pdata_size = wdata[i].control->size; + } - *size += wdata[i].pdata->size; + *size += wdata[i].pdata_size; /* get data type */ switch (cdata->cmd) { @@ -876,10 +884,12 @@ static int sof_process_load(struct snd_soc_component *scomp, */ if (ipc_data_size) { for (i = 0; i < widget->num_kcontrols; i++) { - memcpy(&process->data[offset], - wdata[i].pdata->data, - wdata[i].pdata->size); - offset += wdata[i].pdata->size; + if (!wdata[i].pdata_size) + continue; + + memcpy(&process->data[offset], wdata[i].pdata, + wdata[i].pdata_size); + offset += wdata[i].pdata_size; } }
It is possible to craft a topology where sof_get_control_data() would do out of bounds access because it expects that it is only called when the payload is bytes type. Confusingly it also handles other types of controls, but the payload parsing implementation is only valid for bytes. Fix the code to count the non bytes controls and instead of storing a pointer to sof_abi_hdr in sof_widget_data (which is only valid for bytes), store the pointer to the data itself and add a new member to save the size of the data. In case of non bytes controls we store the pointer to the chanv itself, which is just an array of values at the end. Reported-by: Sergey Senozhatsky <senozhatsky@chromium.org> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> --- Hi, This should fix the out of bounds access, but I'm not sure what would be expected in case of non bytes type of controls, so this patch will consider their size(s) into the counting and copying. Sergey, can you check if it is easy to backport for 5.10.111? There were changes around this (code move to ipc3-topology.c and renames) but the logic remained the same. Regards, Peter sound/soc/sof/ipc3-topology.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-)