[PATCH] ASoC: dapm: Fix NULL pointer dereference in snd_soc_dapm_new_dai

[PATCH] ASoC: dapm: Fix NULL pointer dereference in snd_soc_dapm_new_dai

[Pankaj 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

Re: [PATCH] ASoC: dapm: Fix NULL pointer dereference in snd_soc_dapm_new_dai

[Mark Brown]

[-- 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 --]

Re: [PATCH] ASoC: dapm: Fix NULL pointer dereference in snd_soc_dapm_new_dai

[Pierre-Louis Bossart]

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.

Re: [alsa-devel] [PATCH] ASoC: dapm: Fix NULL pointer dereference in snd_soc_dapm_new_dai

[Mark Brown]

[-- 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 --]

Re: [alsa-devel] [PATCH] ASoC: dapm: Fix NULL pointer dereference in snd_soc_dapm_new_dai

[Bharadiya,Pankaj]

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