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@xxxxxxxxxxxx>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@xxxxxxxxxxxxxxx>
Looks good to me. Thank you.
FWIW,
Reviewed-by: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
Tested-by: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
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)
+ 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;
+ 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;
}
}
--
2.31.0
