[PATCH 0/4] ASoC: topology: Fix route memory corruption

[PATCH 0/4] ASoC: topology: Fix route memory corruption

[Amadeusz Sławiński]

Originally reported here:
https://github.com/thesofproject/avs-topology-xml/issues/22#issuecomment-2127892605
There is various level of failure there, first of all when topology
loads routes, it points directly into FW file, but it may be freed after
topology load. After fixing the above, when avs driver parses topology
it should allocate its own memory, as target strings can be shorter than
needed. Also clean up soc_tplg_dapm_graph_elems_load() a bit.

Amadeusz Sławiński (4):
  ASoC: topology: Fix references to freed memory
  ASoC: Intel: avs: Fix route override
  ASoC: topology: Do not assign fields that are already set
  ASoC: topology: Clean up route loading

 sound/soc/intel/avs/topology.c | 19 ++++++++++++----
 sound/soc/soc-topology.c       | 41 ++++++++++++++++++----------------
 2 files changed, 37 insertions(+), 23 deletions(-)

-- 
2.34.1

[PATCH 1/4] ASoC: topology: Fix references to freed memory

[Amadeusz Sławiński]

Most users after parsing a topology file, release memory used by it, so
having pointer references directly into topology file contents is wrong.
Use devm_kmemdup(), to allocate memory as needed.

Reported-by: Jason Montleon <jmontleo@redhat.com>
Link: https://github.com/thesofproject/avs-topology-xml/issues/22#issuecomment-2127892605
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/soc-topology.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 90ca37e008b32..75d9395a18ed4 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1060,15 +1060,32 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 			break;
 		}
 
-		route->source = elem->source;
-		route->sink = elem->sink;
+		route->source = devm_kmemdup(tplg->dev, elem->source,
+					     min(strlen(elem->source),
+						 SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
+					     GFP_KERNEL);
+		route->sink = devm_kmemdup(tplg->dev, elem->sink,
+					   min(strlen(elem->sink), SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
+					   GFP_KERNEL);
+		if (!route->source || !route->sink) {
+			ret = -ENOMEM;
+			break;
+		}
 
 		/* set to NULL atm for tplg users */
 		route->connected = NULL;
-		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0)
+		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0) {
 			route->control = NULL;
-		else
-			route->control = elem->control;
+		} else {
+			route->control = devm_kmemdup(tplg->dev, elem->control,
+						      min(strlen(elem->control),
+							  SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
+						      GFP_KERNEL);
+			if (!route->control) {
+				ret = -ENOMEM;
+				break;
+			}
+		}
 
 		/* add route dobj to dobj_list */
 		route->dobj.type = SND_SOC_DOBJ_GRAPH;
-- 
2.34.1

Re: [PATCH 1/4] ASoC: topology: Fix references to freed memory

[Pierre-Louis Bossart]

On 6/3/24 12:28, Amadeusz Sławiński wrote:
> Most users after parsing a topology file, release memory used by it, so
> having pointer references directly into topology file contents is wrong.
> Use devm_kmemdup(), to allocate memory as needed.
> 
> Reported-by: Jason Montleon <jmontleo@redhat.com>
> Link: https://github.com/thesofproject/avs-topology-xml/issues/22#issuecomment-2127892605
> Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> ---

This patch breaks the Intel SOF CI in spectacular ways, with the widgets
names completely garbled with noise such as

host-copier.5.playbackpid.socket
host-copier.5.playbackrt@linux.intel.com>
dai-copier.HDA.iDisp3.playbackrun_t:s0
host-copier.31.playback\xff`\x86\xba\x034\x89\xff\xff@N\x83\xb83\x89\xff\xff\x10\x84\xe9\x8b\xff\xff\xff\xffS\x81ی\xff\xff\xff\xff\x0f

https://github.com/thesofproject/linux/pull/5057#issuecomment-2164470192

I am going to revert this patchset in the SOF tree.

>  sound/soc/soc-topology.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index 90ca37e008b32..75d9395a18ed4 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -1060,15 +1060,32 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>  			break;
>  		}
>  
> -		route->source = elem->source;
> -		route->sink = elem->sink;
> +		route->source = devm_kmemdup(tplg->dev, elem->source,
> +					     min(strlen(elem->source),
> +						 SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
> +					     GFP_KERNEL);
> +		route->sink = devm_kmemdup(tplg->dev, elem->sink,
> +					   min(strlen(elem->sink), SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
> +					   GFP_KERNEL);
> +		if (!route->source || !route->sink) {
> +			ret = -ENOMEM;
> +			break;
> +		}
>  
>  		/* set to NULL atm for tplg users */
>  		route->connected = NULL;
> -		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0)
> +		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0) {
>  			route->control = NULL;
> -		else
> -			route->control = elem->control;
> +		} else {
> +			route->control = devm_kmemdup(tplg->dev, elem->control,
> +						      min(strlen(elem->control),
> +							  SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
> +						      GFP_KERNEL);
> +			if (!route->control) {
> +				ret = -ENOMEM;
> +				break;
> +			}
> +		}
>  
>  		/* add route dobj to dobj_list */
>  		route->dobj.type = SND_SOC_DOBJ_GRAPH;

97ab304ecd95c0b1703ff8c8c3956dc6e2afe8e1 is the first bad commit
commit 97ab304ecd95c0b1703ff8c8c3956dc6e2afe8e1
Author: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Date:   Mon Jun 3 12:28:15 2024 +0200

    ASoC: topology: Fix references to freed memory

    Most users after parsing a topology file, release memory used by it, so
    having pointer references directly into topology file contents is wrong.
    Use devm_kmemdup(), to allocate memory as needed.

    Reported-by: Jason Montleon <jmontleo@redhat.com>
    Link:
https://github.com/thesofproject/avs-topology-xml/issues/22#issuecomment-2127892605
    Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
    Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
    Link:
https://lore.kernel.org/r/20240603102818.36165-2-amadeuszx.slawinski@linux.intel.com
    Signed-off-by: Mark Brown <broonie@kernel.org>

 sound/soc/soc-topology.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

Re: [PATCH 1/4] ASoC: topology: Fix references to freed memory

[Péter Ujfalusi]

On 13/06/2024 08:58, Pierre-Louis Bossart wrote:
> 
> 
> On 6/3/24 12:28, Amadeusz Sławiński wrote:
>> Most users after parsing a topology file, release memory used by it, so
>> having pointer references directly into topology file contents is wrong.
>> Use devm_kmemdup(), to allocate memory as needed.
>>
>> Reported-by: Jason Montleon <jmontleo@redhat.com>
>> Link: https://github.com/thesofproject/avs-topology-xml/issues/22#issuecomment-2127892605
>> Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
>> ---
> 
> This patch breaks the Intel SOF CI in spectacular ways, with the widgets
> names completely garbled with noise such as
> 
> host-copier.5.playbackpid.socket
> host-copier.5.playbackrt@linux.intel.com>
> dai-copier.HDA.iDisp3.playbackrun_t:s0
> host-copier.31.playback\xff`\x86\xba\x034\x89\xff\xff@N\x83\xb83\x89\xff\xff\x10\x84\xe9\x8b\xff\xff\xff\xffS\x81ی\xff\xff\xff\xff\x0f
> 
> https://github.com/thesofproject/linux/pull/5057#issuecomment-2164470192
> 
> I am going to revert this patchset in the SOF tree.
> 
>>  sound/soc/soc-topology.c | 27 ++++++++++++++++++++++-----
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
>> index 90ca37e008b32..75d9395a18ed4 100644
>> --- a/sound/soc/soc-topology.c
>> +++ b/sound/soc/soc-topology.c
>> @@ -1060,15 +1060,32 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>>  			break;
>>  		}
>>  
>> -		route->source = elem->source;
>> -		route->sink = elem->sink;
>> +		route->source = devm_kmemdup(tplg->dev, elem->source,
>> +					     min(strlen(elem->source),
>> +						 SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
>> +					     GFP_KERNEL);
>> +		route->sink = devm_kmemdup(tplg->dev, elem->sink,
>> +					   min(strlen(elem->sink), SNDRV_CTL_ELEM_ID_NAME_MAXLEN),

Initially I did not see why this breaks, but then:

The strlen() function calculates the length of the string pointed to by
s, excluding the terminating null byte ('\0').

Likely the fix is as simple as:
min(strlen(elem->sink) + 1, SNDRV_CTL_ELEM_ID_NAME_MAXLEN)

>> +					   GFP_KERNEL);
>> +		if (!route->source || !route->sink) {
>> +			ret = -ENOMEM;
>> +			break;
>> +		}
>>  
>>  		/* set to NULL atm for tplg users */
>>  		route->connected = NULL;
>> -		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0)
>> +		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0) {
>>  			route->control = NULL;
>> -		else
>> -			route->control = elem->control;
>> +		} else {
>> +			route->control = devm_kmemdup(tplg->dev, elem->control,
>> +						      min(strlen(elem->control),
>> +							  SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
>> +						      GFP_KERNEL);
>> +			if (!route->control) {
>> +				ret = -ENOMEM;
>> +				break;
>> +			}
>> +		}
>>  
>>  		/* add route dobj to dobj_list */
>>  		route->dobj.type = SND_SOC_DOBJ_GRAPH;
> 
> 97ab304ecd95c0b1703ff8c8c3956dc6e2afe8e1 is the first bad commit
> commit 97ab304ecd95c0b1703ff8c8c3956dc6e2afe8e1
> Author: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> Date:   Mon Jun 3 12:28:15 2024 +0200
> 
>     ASoC: topology: Fix references to freed memory
> 
>     Most users after parsing a topology file, release memory used by it, so
>     having pointer references directly into topology file contents is wrong.
>     Use devm_kmemdup(), to allocate memory as needed.
> 
>     Reported-by: Jason Montleon <jmontleo@redhat.com>
>     Link:
> https://github.com/thesofproject/avs-topology-xml/issues/22#issuecomment-2127892605
>     Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
>     Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
>     Link:
> https://lore.kernel.org/r/20240603102818.36165-2-amadeuszx.slawinski@linux.intel.com
>     Signed-off-by: Mark Brown <broonie@kernel.org>
> 
>  sound/soc/soc-topology.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> 

-- 
Péter

Re: [PATCH 1/4] ASoC: topology: Fix references to freed memory

[Péter Ujfalusi]

On 13/06/2024 09:27, Péter Ujfalusi wrote:
> 
> 
> On 13/06/2024 08:58, Pierre-Louis Bossart wrote:
>>
>>
>> On 6/3/24 12:28, Amadeusz Sławiński wrote:
>>> Most users after parsing a topology file, release memory used by it, so
>>> having pointer references directly into topology file contents is wrong.
>>> Use devm_kmemdup(), to allocate memory as needed.
>>>
>>> Reported-by: Jason Montleon <jmontleo@redhat.com>
>>> Link: https://github.com/thesofproject/avs-topology-xml/issues/22#issuecomment-2127892605
>>> Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
>>> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
>>> ---
>>
>> This patch breaks the Intel SOF CI in spectacular ways, with the widgets
>> names completely garbled with noise such as
>>
>> host-copier.5.playbackpid.socket
>> host-copier.5.playbackrt@linux.intel.com>
>> dai-copier.HDA.iDisp3.playbackrun_t:s0
>> host-copier.31.playback\xff`\x86\xba\x034\x89\xff\xff@N\x83\xb83\x89\xff\xff\x10\x84\xe9\x8b\xff\xff\xff\xffS\x81ی\xff\xff\xff\xff\x0f
>>
>> https://github.com/thesofproject/linux/pull/5057#issuecomment-2164470192
>>
>> I am going to revert this patchset in the SOF tree.
>>
>>>  sound/soc/soc-topology.c | 27 ++++++++++++++++++++++-----
>>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
>>> index 90ca37e008b32..75d9395a18ed4 100644
>>> --- a/sound/soc/soc-topology.c
>>> +++ b/sound/soc/soc-topology.c
>>> @@ -1060,15 +1060,32 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>>>  			break;
>>>  		}
>>>  
>>> -		route->source = elem->source;
>>> -		route->sink = elem->sink;
>>> +		route->source = devm_kmemdup(tplg->dev, elem->source,
>>> +					     min(strlen(elem->source),
>>> +						 SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
>>> +					     GFP_KERNEL);
>>> +		route->sink = devm_kmemdup(tplg->dev, elem->sink,
>>> +					   min(strlen(elem->sink), SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
> 
> Initially I did not see why this breaks, but then:
> 
> The strlen() function calculates the length of the string pointed to by
> s, excluding the terminating null byte ('\0').
> 
> Likely the fix is as simple as:
> min(strlen(elem->sink) + 1, SNDRV_CTL_ELEM_ID_NAME_MAXLEN)

or better yet:
route->sink = devm_kasprintf(tplg->dev, GFP_KERNEL, "%s", elem->sink);

> 
>>> +					   GFP_KERNEL);
>>> +		if (!route->source || !route->sink) {
>>> +			ret = -ENOMEM;
>>> +			break;
>>> +		}
>>>  
>>>  		/* set to NULL atm for tplg users */
>>>  		route->connected = NULL;
>>> -		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0)
>>> +		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0) {
>>>  			route->control = NULL;
>>> -		else
>>> -			route->control = elem->control;
>>> +		} else {
>>> +			route->control = devm_kmemdup(tplg->dev, elem->control,
>>> +						      min(strlen(elem->control),
>>> +							  SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
>>> +						      GFP_KERNEL);
>>> +			if (!route->control) {
>>> +				ret = -ENOMEM;
>>> +				break;
>>> +			}
>>> +		}
>>>  
>>>  		/* add route dobj to dobj_list */
>>>  		route->dobj.type = SND_SOC_DOBJ_GRAPH;
>>
>> 97ab304ecd95c0b1703ff8c8c3956dc6e2afe8e1 is the first bad commit
>> commit 97ab304ecd95c0b1703ff8c8c3956dc6e2afe8e1
>> Author: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
>> Date:   Mon Jun 3 12:28:15 2024 +0200
>>
>>     ASoC: topology: Fix references to freed memory
>>
>>     Most users after parsing a topology file, release memory used by it, so
>>     having pointer references directly into topology file contents is wrong.
>>     Use devm_kmemdup(), to allocate memory as needed.
>>
>>     Reported-by: Jason Montleon <jmontleo@redhat.com>
>>     Link:
>> https://github.com/thesofproject/avs-topology-xml/issues/22#issuecomment-2127892605
>>     Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
>>     Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
>>     Link:
>> https://lore.kernel.org/r/20240603102818.36165-2-amadeuszx.slawinski@linux.intel.com
>>     Signed-off-by: Mark Brown <broonie@kernel.org>
>>
>>  sound/soc/soc-topology.c | 27 ++++++++++++++++++++++-----
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>
>>
> 

-- 
Péter

Re: [PATCH 1/4] ASoC: topology: Fix references to freed memory

[Péter Ujfalusi]

On 13/06/2024 09:29, Péter Ujfalusi wrote:
>>>> +		route->sink = devm_kmemdup(tplg->dev, elem->sink,
>>>> +					   min(strlen(elem->sink), SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
>>
>> Initially I did not see why this breaks, but then:
>>
>> The strlen() function calculates the length of the string pointed to by
>> s, excluding the terminating null byte ('\0').
>>
>> Likely the fix is as simple as:
>> min(strlen(elem->sink) + 1, SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
> 
> or better yet:
> route->sink = devm_kasprintf(tplg->dev, GFP_KERNEL, "%s", elem->sink);

or even better:
route->sink = devm_kstrdup(tplg->dev, elem->sink, GFP_KERNEL);

-- 
Péter

Re: [PATCH 1/4] ASoC: topology: Fix references to freed memory

[Amadeusz Sławiński]

On 6/13/2024 8:44 AM, Péter Ujfalusi wrote:
> 
> On 13/06/2024 09:29, Péter Ujfalusi wrote:
>>>>> +		route->sink = devm_kmemdup(tplg->dev, elem->sink,
>>>>> +					   min(strlen(elem->sink), SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
>>>
>>> Initially I did not see why this breaks, but then:
>>>
>>> The strlen() function calculates the length of the string pointed to by
>>> s, excluding the terminating null byte ('\0').
>>>
>>> Likely the fix is as simple as:
>>> min(strlen(elem->sink) + 1, SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
>>
>> or better yet:
>> route->sink = devm_kasprintf(tplg->dev, GFP_KERNEL, "%s", elem->sink);
> 
> or even better:
> route->sink = devm_kstrdup(tplg->dev, elem->sink, GFP_KERNEL);
> 

I guess I might have overdid it a bit, let's go with devm_kstrdup for 
all of them, as it should just work unless someone tries to corrupt 
topology file.

[PATCH 2/4] ASoC: Intel: avs: Fix route override

[Amadeusz Sławiński]

Instead of overriding existing memory strings that may be too short,
just allocate needed memory and point the route at it.

Reported-by: Jason Montleon <jmontleo@redhat.com>
Link: https://github.com/thesofproject/avs-topology-xml/issues/22#issuecomment-2127892605
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/intel/avs/topology.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c
index 02bae207f6ece..b6c5d94a15548 100644
--- a/sound/soc/intel/avs/topology.c
+++ b/sound/soc/intel/avs/topology.c
@@ -1545,8 +1545,8 @@ static int avs_route_load(struct snd_soc_component *comp, int index,
 {
 	struct snd_soc_acpi_mach *mach = dev_get_platdata(comp->card->dev);
 	size_t len = SNDRV_CTL_ELEM_ID_NAME_MAXLEN;
-	char buf[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
 	int ssp_port, tdm_slot;
+	char *buf;
 
 	/* See parse_link_formatted_string() for dynamic naming when(s). */
 	if (!avs_mach_singular_ssp(mach))
@@ -1557,13 +1557,24 @@ static int avs_route_load(struct snd_soc_component *comp, int index,
 		return 0;
 	tdm_slot = avs_mach_ssp_tdm(mach, ssp_port);
 
+	buf = devm_kzalloc(comp->card->dev, len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
 	avs_ssp_sprint(buf, len, route->source, ssp_port, tdm_slot);
-	strscpy((char *)route->source, buf, len);
+	route->source = buf;
+
+	buf = devm_kzalloc(comp->card->dev, len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
 	avs_ssp_sprint(buf, len, route->sink, ssp_port, tdm_slot);
-	strscpy((char *)route->sink, buf, len);
+	route->sink = buf;
+
 	if (route->control) {
+		buf = devm_kzalloc(comp->card->dev, len, GFP_KERNEL);
+		if (!buf)
+			return -ENOMEM;
 		avs_ssp_sprint(buf, len, route->control, ssp_port, tdm_slot);
-		strscpy((char *)route->control, buf, len);
+		route->control = buf;
 	}
 
 	return 0;
-- 
2.34.1

[PATCH 3/4] ASoC: topology: Do not assign fields that are already set

[Amadeusz Sławiński]

The routes are allocated with kzalloc(), so all fields are zeroed by
default, skip unnecessary assignments.

Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/soc-topology.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 75d9395a18ed4..1db540aaad451 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1072,11 +1072,7 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 			break;
 		}
 
-		/* set to NULL atm for tplg users */
-		route->connected = NULL;
-		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0) {
-			route->control = NULL;
-		} else {
+		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) != 0) {
 			route->control = devm_kmemdup(tplg->dev, elem->control,
 						      min(strlen(elem->control),
 							  SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
-- 
2.34.1

[PATCH 4/4] ASoC: topology: Clean up route loading

[Amadeusz Sławiński]

Instead of using very long macro name, assign it to shorter variable
and use it instead. While doing that, we can reduce multiple if checks
using this define to one.

Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/soc-topology.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 1db540aaad451..2ac442644ed4f 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1021,6 +1021,7 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 	struct snd_soc_tplg_hdr *hdr)
 {
 	struct snd_soc_dapm_context *dapm = &tplg->comp->dapm;
+	const size_t maxlen = SNDRV_CTL_ELEM_ID_NAME_MAXLEN;
 	struct snd_soc_tplg_dapm_graph_elem *elem;
 	struct snd_soc_dapm_route *route;
 	int count, i;
@@ -1044,38 +1045,27 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 		tplg->pos += sizeof(struct snd_soc_tplg_dapm_graph_elem);
 
 		/* validate routes */
-		if (strnlen(elem->source, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
-			    SNDRV_CTL_ELEM_ID_NAME_MAXLEN) {
-			ret = -EINVAL;
-			break;
-		}
-		if (strnlen(elem->sink, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
-			    SNDRV_CTL_ELEM_ID_NAME_MAXLEN) {
-			ret = -EINVAL;
-			break;
-		}
-		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
-			    SNDRV_CTL_ELEM_ID_NAME_MAXLEN) {
+		if ((strnlen(elem->source, maxlen) == maxlen) ||
+		    (strnlen(elem->sink, maxlen) == maxlen) ||
+		    (strnlen(elem->control, maxlen) == maxlen)) {
 			ret = -EINVAL;
 			break;
 		}
 
 		route->source = devm_kmemdup(tplg->dev, elem->source,
-					     min(strlen(elem->source),
-						 SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
+					     min(strlen(elem->source), maxlen),
 					     GFP_KERNEL);
 		route->sink = devm_kmemdup(tplg->dev, elem->sink,
-					   min(strlen(elem->sink), SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
+					   min(strlen(elem->sink), maxlen),
 					   GFP_KERNEL);
 		if (!route->source || !route->sink) {
 			ret = -ENOMEM;
 			break;
 		}
 
-		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) != 0) {
+		if (strnlen(elem->control, maxlen) != 0) {
 			route->control = devm_kmemdup(tplg->dev, elem->control,
-						      min(strlen(elem->control),
-							  SNDRV_CTL_ELEM_ID_NAME_MAXLEN),
+						      min(strlen(elem->control), maxlen),
 						      GFP_KERNEL);
 			if (!route->control) {
 				ret = -ENOMEM;
-- 
2.34.1

Re: [PATCH 0/4] ASoC: topology: Fix route memory corruption

[Mark Brown]

On Mon, 03 Jun 2024 12:28:14 +0200, Amadeusz Sławiński wrote:
> Originally reported here:
> https://github.com/thesofproject/avs-topology-xml/issues/22#issuecomment-2127892605
> There is various level of failure there, first of all when topology
> loads routes, it points directly into FW file, but it may be freed after
> topology load. After fixing the above, when avs driver parses topology
> it should allocate its own memory, as target strings can be shorter than
> needed. Also clean up soc_tplg_dapm_graph_elems_load() a bit.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/4] ASoC: topology: Fix references to freed memory
      commit: 97ab304ecd95c0b1703ff8c8c3956dc6e2afe8e1
[2/4] ASoC: Intel: avs: Fix route override
      commit: fd660b1bd015e5aa9a558ee04088f2431010548d
[3/4] ASoC: topology: Do not assign fields that are already set
      commit: daf0b99d4720c9f05bdb81c73b2efdb43fa9def3
[4/4] ASoC: topology: Clean up route loading
      commit: e0e7bc2cbee93778c4ad7d9a792d425ffb5af6f7

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark