[patch] ASoC: dapm: using freed pointer in dapm_kcontrol_add_widget()

[patch] ASoC: dapm: using freed pointer in dapm_kcontrol_add_widget()

[Dan Carpenter]

There is a typo here so we end up using the old freed pointer instead of
the newly allocated one.  (If the "n" is zero then the code works,
obviously).

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Only needed in linux-next

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 9abb3b2..d74c356 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -225,13 +225,13 @@ static int dapm_kcontrol_add_widget(struct snd_kcontrol *kcontrol,
 
 	new_data = krealloc(data, sizeof(*data) + sizeof(widget) * n,
 		GFP_KERNEL);
-	if (!data)
+	if (!new_data)
 		return -ENOMEM;
 
-	data->wlist.widgets[n - 1] = widget;
-	data->wlist.num_widgets = n;
+	new_data->wlist.widgets[n - 1] = widget;
+	new_data->wlist.num_widgets = n;
 
-	kcontrol->private_data = data;
+	kcontrol->private_data = new_data;
 
 	return 0;
 }

Re: [patch] ASoC: dapm: using freed pointer in dapm_kcontrol_add_widget()

[Lars-Peter Clausen]

On 07/31/2013 10:52 AM, Dan Carpenter wrote:
> There is a typo here so we end up using the old freed pointer instead of
> the newly allocated one.  (If the "n" is zero then the code works,
> obviously).
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Thanks.

Acked-by: Lars-Peter Clausen <lars@metafoo.de>

Olof, can you check whether this fixes the crash you see?

> ---
> Only needed in linux-next
>
> diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
> index 9abb3b2..d74c356 100644
> --- a/sound/soc/soc-dapm.c
> +++ b/sound/soc/soc-dapm.c
> @@ -225,13 +225,13 @@ static int dapm_kcontrol_add_widget(struct snd_kcontrol *kcontrol,
>
>   	new_data = krealloc(data, sizeof(*data) + sizeof(widget) * n,
>   		GFP_KERNEL);
> -	if (!data)
> +	if (!new_data)
>   		return -ENOMEM;
>
> -	data->wlist.widgets[n - 1] = widget;
> -	data->wlist.num_widgets = n;
> +	new_data->wlist.widgets[n - 1] = widget;
> +	new_data->wlist.num_widgets = n;
>
> -	kcontrol->private_data = data;
> +	kcontrol->private_data = new_data;
>
>   	return 0;
>   }
>

Re: [patch] ASoC: dapm: using freed pointer in dapm_kcontrol_add_widget()

[Olof Johansson]

Hi,

On Wed, Jul 31, 2013 at 2:02 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 07/31/2013 10:52 AM, Dan Carpenter wrote:
>>
>> There is a typo here so we end up using the old freed pointer instead of
>> the newly allocated one.  (If the "n" is zero then the code works,
>> obviously).
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
>
> Thanks.
>
> Acked-by: Lars-Peter Clausen <lars@metafoo.de>
>
> Olof, can you check whether this fixes the crash you see?

Nope.

There's also remaining issues with the code, that patch isn't enough.
The structure that is krealloced() has a list_head in it, but the list
isn't moved from the old head to the new one. There's no safe way to
do that using krealloc, since the old list_head is gone by then, so
it's probably easest to open-code with kzalloc/memcpy/kfree.

But even with that fixed, I still see the same issue. Boggle.

(Btw, I'm testing on top of commit 5106b92f80a2cd37c52cffed80b4f5acfb77ccfd).

-Olof

Re: [patch] ASoC: dapm: using freed pointer in dapm_kcontrol_add_widget()

[Lars-Peter Clausen]

On 07/31/2013 08:17 PM, Olof Johansson wrote:
> Hi,
>
> On Wed, Jul 31, 2013 at 2:02 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 07/31/2013 10:52 AM, Dan Carpenter wrote:
>>>
>>> There is a typo here so we end up using the old freed pointer instead of
>>> the newly allocated one.  (If the "n" is zero then the code works,
>>> obviously).
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>>
>> Thanks.
>>
>> Acked-by: Lars-Peter Clausen <lars@metafoo.de>
>>
>> Olof, can you check whether this fixes the crash you see?
>
> Nope.
>
> There's also remaining issues with the code, that patch isn't enough.
> The structure that is krealloced() has a list_head in it, but the list
> isn't moved from the old head to the new one. There's no safe way to
> do that using krealloc, since the old list_head is gone by then, so
> it's probably easest to open-code with kzalloc/memcpy/kfree.

Hm, right I didn't think of that. Maybe it's better to just keep a the widget 
list in a separate pointer, so none of the other fields of the kcontrol_data 
struct are affected by the krealloc.

- Lars

Re: [patch] ASoC: dapm: using freed pointer in dapm_kcontrol_add_widget()

[Lars-Peter Clausen]

On 07/31/2013 08:33 PM, Lars-Peter Clausen wrote:
> On 07/31/2013 08:17 PM, Olof Johansson wrote:
>> Hi,
>>
>> On Wed, Jul 31, 2013 at 2:02 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>> On 07/31/2013 10:52 AM, Dan Carpenter wrote:
>>>>
>>>> There is a typo here so we end up using the old freed pointer instead of
>>>> the newly allocated one.  (If the "n" is zero then the code works,
>>>> obviously).
>>>>
>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>>
>>> Thanks.
>>>
>>> Acked-by: Lars-Peter Clausen <lars@metafoo.de>
>>>
>>> Olof, can you check whether this fixes the crash you see?
>>
>> Nope.
>>
>> There's also remaining issues with the code, that patch isn't enough.
>> The structure that is krealloced() has a list_head in it, but the list
>> isn't moved from the old head to the new one. There's no safe way to
>> do that using krealloc, since the old list_head is gone by then, so
>> it's probably easest to open-code with kzalloc/memcpy/kfree.
>
> Hm, right I didn't think of that. Maybe it's better to just keep a the widget
> list in a separate pointer, so none of the other fields of the kcontrol_data
> struct are affected by the krealloc.
>

Something along the lines of this:

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index d74c356..ef1db0b 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -177,7 +177,7 @@ static inline struct snd_soc_dapm_widget *dapm_cnew_widget(
  struct dapm_kcontrol_data {
  	unsigned int value;
  	struct list_head paths;
-	struct snd_soc_dapm_widget_list wlist;
+	struct snd_soc_dapm_widget_list *wlist;
  };

  static int dapm_kcontrol_data_alloc(struct snd_soc_dapm_widget *widget,
@@ -185,26 +185,36 @@ static int dapm_kcontrol_data_alloc(struct
  {
  	struct dapm_kcontrol_data *data;

-	data = kzalloc(sizeof(*data) + sizeof(widget), GFP_KERNEL);
-	if (!data) {
-		dev_err(widget->dapm->dev,
-				"ASoC: can't allocate kcontrol data for %s\n",
-				widget->name);
-		return -ENOMEM;
-	}
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		goto err;

-	data->wlist.widgets[0] = widget;
-	data->wlist.num_widgets = 1;
+	data->wlist = kzalloc(sizeof(*data->wlist) + sizeof(widget),
+		GFP_KERNEL);
+	if (!data->wlist)
+		goto err_free;
+
+	data->wlist->widgets[0] = widget;
+	data->wlist->num_widgets = 1;
  	INIT_LIST_HEAD(&data->paths);

  	kcontrol->private_data = data;

  	return 0;
+err_free:
+	kfree(data);
+err:
+	dev_err(widget->dapm->dev,
+			"ASoC: can't allocate kcontrol data for %s\n",
+			widget->name);
+
+	return -ENOMEM;
  }

  static void dapm_kcontrol_free(struct snd_kcontrol *kctl)
  {
  	struct dapm_kcontrol_data *data = snd_kcontrol_chip(kctl);
+	kfree(data->wlist);
  	kfree(data);
  }

@@ -213,25 +223,25 @@ static struct snd_soc_dapm_widget_list
  {
  	struct dapm_kcontrol_data *data = snd_kcontrol_chip(kcontrol);

-	return &data->wlist;
+	return data->wlist;
  }

  static int dapm_kcontrol_add_widget(struct snd_kcontrol *kcontrol,
  	struct snd_soc_dapm_widget *widget)
  {
  	struct dapm_kcontrol_data *data = snd_kcontrol_chip(kcontrol);
-	struct dapm_kcontrol_data *new_data;
-	unsigned int n = data->wlist.num_widgets + 1;
+	struct snd_soc_dapm_widget_list *new_wlist;
+	unsigned int n = data->wlist->num_widgets + 1;

-	new_data = krealloc(data, sizeof(*data) + sizeof(widget) * n,
+	new_wlist = krealloc(data, sizeof(*new_wlist) + sizeof(widget) * n,
  		GFP_KERNEL);
-	if (!new_data)
+	if (!new_wlist)
  		return -ENOMEM;

-	new_data->wlist.widgets[n - 1] = widget;
-	new_data->wlist.num_widgets = n;
+	new_wlist->widgets[n - 1] = widget;
+	new_wlist->num_widgets = n;

-	kcontrol->private_data = new_data;
+	data->wlist = new_wlist;

  	return 0;
  }

Re: [patch] ASoC: dapm: using freed pointer in dapm_kcontrol_add_widget()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 168 bytes --]

On Wed, Jul 31, 2013 at 08:44:17PM +0200, Lars-Peter Clausen wrote:

> Something along the lines of this:

This seems to fix it for me, can you submit properly please?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [patch] ASoC: dapm: using freed pointer in dapm_kcontrol_add_widget()

[Olof Johansson]

On Wed, Jul 31, 2013 at 1:05 PM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Jul 31, 2013 at 08:44:17PM +0200, Lars-Peter Clausen wrote:
>
>> Something along the lines of this:
>
> This seems to fix it for me, can you submit properly please?

Here too. If you submit that as a patch, feel free to add:

Tested-by: Olof Johansson <olof@lixom.net>


-Olof

Re: [patch] ASoC: dapm: using freed pointer in dapm_kcontrol_add_widget()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 239 bytes --]

On Wed, Jul 31, 2013 at 11:52:44AM +0300, Dan Carpenter wrote:
> There is a typo here so we end up using the old freed pointer instead of
> the newly allocated one.  (If the "n" is zero then the code works,
> obviously).

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]