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

[PATCH v2] 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,

changes since v1:
- adjust the payload size for non bytes controls by subtracting the size of the
  sof_ipc_ctrl_data struct, plus add comment to note this

Regards,
Peter

 sound/soc/sof/ipc3-topology.c | 40 ++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/sound/soc/sof/ipc3-topology.c b/sound/soc/sof/ipc3-topology.c
index 572bcbfdb356..f9f37e01edfa 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,29 @@ 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 {
+			/* 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 (cdata->cmd) {
@@ -876,10 +890,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 v2] ASoC: SOF: ipc3-topology: Correct get_control_data for non bytes payload

[Sergey Senozhatsky]

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

Re: [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@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;
>  		}
>  	}
>  

-- 
Péter

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

[Sergey Senozhatsky]

On (22/04/27 14:33), Péter Ujfalusi wrote:
> > @@ -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*?

clang appears to be unhappy otherwise.

	error: comparison of array 'cdata->data' equal to a null pointer is always false

Changing this into `if (!cdata->data)` is a little bit better as now
'always false' becomes 'always true'

	error: address of array 'cdata->data' will always evaluate to 'true'

> > +				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.

Oh... good catch!

I used `wdata[i].control->control_data->data` for tests, converted this
to cdata the very last moment.

So something like this then

---
 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..60b4b0053e98 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->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

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

[Péter Ujfalusi]

On 27/04/2022 14:54, Sergey Senozhatsky wrote:
> On (22/04/27 14:33), Péter Ujfalusi wrote:
>>> @@ -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*?
> 
> clang appears to be unhappy otherwise.
> 
> 	error: comparison of array 'cdata->data' equal to a null pointer is always false
> 
> Changing this into `if (!cdata->data)` is a little bit better as now
> 'always false' becomes 'always true'
> 
> 	error: address of array 'cdata->data' will always evaluate to 'true'

Hrm, uhm. clang is right. The check is (and was) bogus...

cdata->data is a pointer (to cdata->data[0]) which is always:
cdata + sizeof(struct sof_ipc_ctrl_data).
Checking if it is NULL or not is irrelevant and wrong. If we do not have
additional data then cdata->data points to memory which is outside of
the struct and it can be random data (might be 0, might not be).

I think we can just drop this check as we would not be here if
additional data was not allocated for the payload prior?

> 
>>> +				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.
> 
> Oh... good catch!
> 
> I used `wdata[i].control->control_data->data` for tests, converted this
> to cdata the very last moment.
> 
> So something like this then
> 
> ---
>  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..60b4b0053e98 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->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;
>  		}
>  	}
>  

-- 
Péter

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

[Sergey Senozhatsky]

On (22/04/27 15:08), Péter Ujfalusi wrote:
> > clang appears to be unhappy otherwise.
> > 
> > 	error: comparison of array 'cdata->data' equal to a null pointer is always false
> > 
> > Changing this into `if (!cdata->data)` is a little bit better as now
> > 'always false' becomes 'always true'
> > 
> > 	error: address of array 'cdata->data' will always evaluate to 'true'
> 
> Hrm, uhm. clang is right. The check is (and was) bogus...
> 
> cdata->data is a pointer (to cdata->data[0]) which is always:
> cdata + sizeof(struct sof_ipc_ctrl_data).
> Checking if it is NULL or not is irrelevant and wrong. If we do not have
> additional data then cdata->data points to memory which is outside of
> the struct and it can be random data (might be 0, might not be).

Yeah to be honest that's what I'm thinking too.

Does sof_ipc_ctrl_data have to be a var-sized structure? Or can that union
hold pointers that are allocated separately?

	scontrol->data = kzalloc(sizeof sof_ipc_ctrl_data);
	scontrol->data->chan = kzalloc(sizeof chan * mc->num_channels)

> I think we can just drop this check as we would not be here if
> additional data was not allocated for the payload prior?

I don't have enough knowledge of this code. ->data check doesn't do what
it is expected to do so removing it shouldn't do harm.

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

[Péter Ujfalusi]

On 27/04/2022 15:31, Sergey Senozhatsky wrote:
> On (22/04/27 15:08), Péter Ujfalusi wrote:
>>> clang appears to be unhappy otherwise.
>>>
>>> 	error: comparison of array 'cdata->data' equal to a null pointer is always false
>>>
>>> Changing this into `if (!cdata->data)` is a little bit better as now
>>> 'always false' becomes 'always true'
>>>
>>> 	error: address of array 'cdata->data' will always evaluate to 'true'
>>
>> Hrm, uhm. clang is right. The check is (and was) bogus...
>>
>> cdata->data is a pointer (to cdata->data[0]) which is always:
>> cdata + sizeof(struct sof_ipc_ctrl_data).
>> Checking if it is NULL or not is irrelevant and wrong. If we do not have
>> additional data then cdata->data points to memory which is outside of
>> the struct and it can be random data (might be 0, might not be).
> 
> Yeah to be honest that's what I'm thinking too.
> 
> Does sof_ipc_ctrl_data have to be a var-sized structure? Or can that union
> hold pointers that are allocated separately?
> 
> 	scontrol->data = kzalloc(sizeof sof_ipc_ctrl_data);
> 	scontrol->data->chan = kzalloc(sizeof chan * mc->num_channels)

Unfortunately no, the data/chanv/compv needs to be flexible array as it
is the IPC message itself.

> 
>> I think we can just drop this check as we would not be here if
>> additional data was not allocated for the payload prior?
> 
> I don't have enough knowledge of this code. ->data check doesn't do what
> it is expected to do so removing it shouldn't do harm.

Let me quickly send v3 with dropped cdata->data check.

-- 
Péter

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

[Sergey Senozhatsky]

On (22/04/27 15:35), Péter Ujfalusi wrote:
> >> Hrm, uhm. clang is right. The check is (and was) bogus...
> >>
> >> cdata->data is a pointer (to cdata->data[0]) which is always:
> >> cdata + sizeof(struct sof_ipc_ctrl_data).
> >> Checking if it is NULL or not is irrelevant and wrong. If we do not have
> >> additional data then cdata->data points to memory which is outside of
> >> the struct and it can be random data (might be 0, might not be).
> > 
> > Yeah to be honest that's what I'm thinking too.
> > 
> > Does sof_ipc_ctrl_data have to be a var-sized structure? Or can that union
> > hold pointers that are allocated separately?
> > 
> > 	scontrol->data = kzalloc(sizeof sof_ipc_ctrl_data);
> > 	scontrol->data->chan = kzalloc(sizeof chan * mc->num_channels)
> 
> Unfortunately no, the data/chanv/compv needs to be flexible array as it
> is the IPC message itself.

That's what I suspected.

> >> I think we can just drop this check as we would not be here if
> >> additional data was not allocated for the payload prior?
> > 
> > I don't have enough knowledge of this code. ->data check doesn't do what
> > it is expected to do so removing it shouldn't do harm.
> 
> Let me quickly send v3 with dropped cdata->data check.

OK. I'll remove if from the backport, run another test and will call it a
day. As you can guess I was puzzled by that ->data check but it's another
very long day in the office for me and in the end I just dropped the ball
and decide to suppress clang warning instead. Very smart! (NO). My bad. (YES).