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 |
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(-)
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: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(-) >> >> >
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);
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 --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;