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

[Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxx>
> Link: https://github.com/thesofproject/avs-topology-xml/issues/22#issuecomment-2127892605
> Reviewed-by: Cezary Rojewski <cezary.rojewski@xxxxxxxxx>
> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>
> ---

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@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxx>
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@xxxxxxxxxx>
    Link:
https://github.com/thesofproject/avs-topology-xml/issues/22#issuecomment-2127892605
    Reviewed-by: Cezary Rojewski <cezary.rojewski@xxxxxxxxx>
    Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>
    Link:
https://lore.kernel.org/r/20240603102818.36165-2-amadeuszx.slawinski@xxxxxxxxxxxxxxx
    Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>

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