On 27/04/2022 14:20, Sergey Senozhatsky wrote:
On (22/04/27 13:52), Peter Ujfalusi wrote:
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
Looks good to me. Thank you. FWIW, Reviewed-by: Sergey Senozhatsky senozhatsky@chromium.org Tested-by: Sergey Senozhatsky senozhatsky@chromium.org
So below is what I ended up with for 5.10. The original patch does not apply cleanly because -stable is missing a number of patches, so I crafted a backport. If it looks OK to you then we probably can send it to stable folks.
sound/soc/sof/topology.c | 42 +++++++++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 13 deletions(-)
diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c index c1fc7bcf4eb5..2b80dbe427c1 100644 --- a/sound/soc/sof/topology.c +++ b/sound/soc/sof/topology.c @@ -50,7 +50,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;
};
@@ -2100,6 +2101,7 @@ static int sof_get_control_data(struct snd_soc_component *scomp, size_t *size) { const struct snd_kcontrol_new *kc;
- struct sof_ipc_ctrl_data *cdata; struct soc_mixer_control *sm; struct soc_bytes_ext *sbe; struct soc_enum *se;
@@ -2136,16 +2138,28 @@ static int sof_get_control_data(struct snd_soc_component *scomp, return -EINVAL; }
wdata[i].pdata = wdata[i].control->control_data->data;if (!wdata[i].pdata)return -EINVAL;
cdata = wdata[i].control->control_data;if (widget->dobj.widget.kcontrol_type[i] == SND_SOC_TPLG_TYPE_BYTES) {if ((void *)cdata->data == NULL)
Is there a need for casting it to void*?
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 (cdata->data->magic != SOF_ABI_MAGIC)return -EINVAL;wdata[i].pdata = cdata->data;
you want to save the cdata->data->data, so w/o the abi header stuff.
wdata[i].pdata_size = cdata->data->size;} else {/* points to the control data union */wdata[i].pdata = cdata->chanv;/** wdata[i].control->size is calculated with struct_size* and includes the size of struct sof_ipc_ctrl_data*/wdata[i].pdata_size = wdata[i].control->size -sizeof(struct sof_ipc_ctrl_data);}
*size += wdata[i].pdata->size;
*size += wdata[i].pdata_size;/* get data type */ switch (wdata[i].control->cmd) {
@@ -2236,10 +2250,12 @@ static int sof_process_load(struct snd_soc_component *scomp, int index, */ 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;