| Message ID | 20240603102818.36165-2-amadeuszx.slawinski@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | 97ab304ecd95c0b1703ff8c8c3956dc6e2afe8e1 |
| Headers | show |
| Series | ASoC: topology: Fix route memory corruption | expand |
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(-) > >
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);
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;