[alsa-devel] [PATCH] ASoC: dapm: Fix NULL pointer dereference in snd_soc_dapm_new_dai
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);
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.
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.
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.
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
participants (4)
-
Bharadiya,Pankaj -
Mark Brown -
Pankaj Bharadiya
-
Pierre-Louis Bossart