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

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

[Peter Ujfalusi]

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(-)

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;
 		}
 	}
 
-- 
2.36.0

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

[Sergey Senozhatsky]

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.

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

[Péter Ujfalusi]

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.

-- 
Péter