diff mbox series

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

Message ID 20240603102818.36165-2-amadeuszx.slawinski@linux.intel.com (mailing list archive)
State Superseded
Commit 97ab304ecd95c0b1703ff8c8c3956dc6e2afe8e1
Headers show
Series ASoC: topology: Fix route memory corruption | expand

Commit Message

Amadeusz Sławiński June 3, 2024, 10:28 a.m. UTC 
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(-)

Comments

Pierre-Louis Bossart June 13, 2024, 5:58 a.m. UTC | #1 
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(-)
Péter Ujfalusi June 13, 2024, 6:27 a.m. UTC | #2 
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 Ujfalusi June 13, 2024, 6:29 a.m. UTC | #3 
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 Ujfalusi June 13, 2024, 6:44 a.m. UTC | #4 
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);
Amadeusz Sławiński June 13, 2024, 7:31 a.m. UTC | #5 
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.
diff mbox series

Patch

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;