[PATCH v2] ASoC: SOF: ipc3-topology: Correct get_control_data for non bytes payload

[Péter Ujfalusi]

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 at chromium.org>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi at linux.intel.com>
> 
> Looks good to me. Thank you.
> FWIW,
> Reviewed-by: Sergey Senozhatsky <senozhatsky at chromium.org>
> Tested-by: Sergey Senozhatsky <senozhatsky at 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;
>  		}
>  	}
>  

-- 
Péter