* [PATCH] ASoC: dapm: Fix NULL pointer dereference in snd_soc_dapm_new_dai
@ 2019-03-21 10:11 Pankaj Bharadiya
2019-03-21 12:31 ` Mark Brown
0 siblings, 1 reply; 5+ messages in thread
From: Pankaj Bharadiya @ 2019-03-21 10:11 UTC (permalink / raw)
To: lgirdwood, broonie, perex, tiwai, alsa-devel
Cc: linux-kernel, pankaj.laxminarayan.bharadiya
In case of single config, w_param_text is NULL.
In snd_soc_dapm_new_control_unlocked() call failure case, it will end
up calling snd_soc_dapm_free_kcontrol() unconditionally and result in
NULL pointer dereference.
Signed-off-by: Pankaj Bharadiya <pankaj.laxminarayan.bharadiya@intel.com>
---
sound/soc/soc-dapm.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 1ec06ef..ba6cb37 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -4094,8 +4094,9 @@ snd_soc_dapm_new_dai(struct snd_soc_card *card, struct snd_soc_pcm_runtime *rtd,
outfree_kcontrol_news:
devm_kfree(card->dev, (void *)template.kcontrol_news);
- snd_soc_dapm_free_kcontrol(card, &private_value,
- rtd->dai_link->num_params, w_param_text);
+ if (w_param_text)
+ snd_soc_dapm_free_kcontrol(card, &private_value,
+ rtd->dai_link->num_params, w_param_text);
param_fail:
devm_kfree(card->dev, link_name);
return ERR_PTR(ret);
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ASoC: dapm: Fix NULL pointer dereference in snd_soc_dapm_new_dai
2019-03-21 10:11 [PATCH] ASoC: dapm: Fix NULL pointer dereference in snd_soc_dapm_new_dai Pankaj Bharadiya
@ 2019-03-21 12:31 ` Mark Brown
2019-03-21 13:59 ` Pierre-Louis Bossart
0 siblings, 1 reply; 5+ messages in thread
From: Mark Brown @ 2019-03-21 12:31 UTC (permalink / raw)
To: Pankaj Bharadiya; +Cc: lgirdwood, perex, tiwai, alsa-devel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 680 bytes --]
On Thu, Mar 21, 2019 at 03:41:25PM +0530, Pankaj Bharadiya wrote:
> outfree_kcontrol_news:
> devm_kfree(card->dev, (void *)template.kcontrol_news);
> - snd_soc_dapm_free_kcontrol(card, &private_value,
> - rtd->dai_link->num_params, w_param_text);
> + if (w_param_text)
> + snd_soc_dapm_free_kcontrol(card, &private_value,
> + rtd->dai_link->num_params, w_param_text);
This is very non-obvious - it's not at all clear why we'd need the text
to free controls. If there is an issue here it seems like it'd be
better to make sure that snd_soc_dapm_free_kcontrol() can cope with that
being NULL, that will be clearer and also avoid potential issues with
other callers.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ASoC: dapm: Fix NULL pointer dereference in snd_soc_dapm_new_dai
2019-03-21 12:31 ` Mark Brown
@ 2019-03-21 13:59 ` Pierre-Louis Bossart
2019-03-21 14:09 ` [alsa-devel] " Mark Brown
0 siblings, 1 reply; 5+ messages in thread
From: Pierre-Louis Bossart @ 2019-03-21 13:59 UTC (permalink / raw)
To: Mark Brown, Pankaj Bharadiya; +Cc: lgirdwood, alsa-devel, linux-kernel, tiwai
On 3/21/19 7:31 AM, Mark Brown wrote:
> On Thu, Mar 21, 2019 at 03:41:25PM +0530, Pankaj Bharadiya wrote:
>
>> outfree_kcontrol_news:
>> devm_kfree(card->dev, (void *)template.kcontrol_news);
>> - snd_soc_dapm_free_kcontrol(card, &private_value,
>> - rtd->dai_link->num_params, w_param_text);
>> + if (w_param_text)
>> + snd_soc_dapm_free_kcontrol(card, &private_value,
>> + rtd->dai_link->num_params, w_param_text);
> This is very non-obvious - it's not at all clear why we'd need the text
> to free controls. If there is an issue here it seems like it'd be
> better to make sure that snd_soc_dapm_free_kcontrol() can cope with that
> being NULL, that will be clearer and also avoid potential issues with
> other callers.
I believe the issue is real, but you need to look at the entire code to figure it out
/* allocate memory for control, only in case of multiple configs */
if (rtd->dai_link->num_params > 1) {
w_param_text = devm_kcalloc(card->dev,
rtd->dai_link->num_params,
sizeof(char *), GFP_KERNEL);
if (!w_param_text) {
ret = -ENOMEM;
goto param_fail;
}
template.num_kcontrols = 1;
template.kcontrol_news =
snd_soc_dapm_alloc_kcontrol(card,
link_name,
rtd->dai_link->params,
rtd->dai_link->num_params,
w_param_text, &private_value);
if (!template.kcontrol_news) {
ret = -ENOMEM;
goto param_fail;
}
} else {
w_param_text = NULL; <<<< this is set when there is a single config
}
dev_dbg(card->dev, "ASoC: adding %s widget\n", link_name);
w = snd_soc_dapm_new_control_unlocked(&card->dapm, &template);
if (IS_ERR(w)) {
ret = PTR_ERR(w);
goto outfree_kcontrol_news; <<< the control creation failed
}
w->priv = rtd;
return w;
outfree_kcontrol_news:
devm_kfree(card->dev, (void *)template.kcontrol_news);
<<< and in the function below we try to access w_param_text and private_value which haven't been allocated.
snd_soc_dapm_free_kcontrol(card, &private_value,
rtd->dai_link->num_params, w_param_text);
That said I agree with Mark that it's better to change snd_soc_dapm_free_kcontrol directly.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: dapm: Fix NULL pointer dereference in snd_soc_dapm_new_dai
2019-03-21 13:59 ` Pierre-Louis Bossart
@ 2019-03-21 14:09 ` Mark Brown
2019-03-21 16:31 ` Bharadiya,Pankaj
0 siblings, 1 reply; 5+ messages in thread
From: Mark Brown @ 2019-03-21 14:09 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: Pankaj Bharadiya, linux-kernel, alsa-devel, tiwai, lgirdwood
[-- Attachment #1: Type: text/plain, Size: 694 bytes --]
On Thu, Mar 21, 2019 at 08:59:55AM -0500, Pierre-Louis Bossart wrote:
> On 3/21/19 7:31 AM, Mark Brown wrote:
> > On Thu, Mar 21, 2019 at 03:41:25PM +0530, Pankaj Bharadiya wrote:
> > This is very non-obvious - it's not at all clear why we'd need the text
> > to free controls. If there is an issue here it seems like it'd be
> > better to make sure that snd_soc_dapm_free_kcontrol() can cope with that
> > being NULL, that will be clearer and also avoid potential issues with
> > other callers.
> I believe the issue is real, but you need to look at the entire code to figure it out
Yeah, I'm fairly sure there's an actual issue here - it's just that the
fix is obscure and feels fragile.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: dapm: Fix NULL pointer dereference in snd_soc_dapm_new_dai
2019-03-21 14:09 ` [alsa-devel] " Mark Brown
@ 2019-03-21 16:31 ` Bharadiya,Pankaj
0 siblings, 0 replies; 5+ messages in thread
From: Bharadiya,Pankaj @ 2019-03-21 16:31 UTC (permalink / raw)
To: Mark Brown
Cc: Pierre-Louis Bossart, linux-kernel, alsa-devel, tiwai, lgirdwood
On Thu, Mar 21, 2019 at 02:09:48PM +0000, Mark Brown wrote:
> On Thu, Mar 21, 2019 at 08:59:55AM -0500, Pierre-Louis Bossart wrote:
> > On 3/21/19 7:31 AM, Mark Brown wrote:
> > > On Thu, Mar 21, 2019 at 03:41:25PM +0530, Pankaj Bharadiya wrote:
>
> > > This is very non-obvious - it's not at all clear why we'd need the text
> > > to free controls. If there is an issue here it seems like it'd be
> > > better to make sure that snd_soc_dapm_free_kcontrol() can cope with that
> > > being NULL, that will be clearer and also avoid potential issues with
> > > other callers.
>
> > I believe the issue is real, but you need to look at the entire code to figure it out
>
> Yeah, I'm fairly sure there's an actual issue here - it's just that the
> fix is obscure and feels fragile.
Will fix it in snd_soc_dapm_free_kcontrol() and submit a new patch..
Thanks,
Pankaj
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-21 16:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-21 10:11 [PATCH] ASoC: dapm: Fix NULL pointer dereference in snd_soc_dapm_new_dai Pankaj Bharadiya
2019-03-21 12:31 ` Mark Brown
2019-03-21 13:59 ` Pierre-Louis Bossart
2019-03-21 14:09 ` [alsa-devel] " Mark Brown
2019-03-21 16:31 ` Bharadiya,Pankaj
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.