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@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)
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;
> }
> }
>
--
Péter
