[PATCH v2] ASoC: Intel: Skylake: prevent memory leak in snd_skl_parse_uuids

[PATCH v2] ASoC: Intel: Skylake: prevent memory leak in snd_skl_parse_uuids

[Navid Emamdoost]

In snd_skl_parse_uuids if allocation for module->instance_id fails, the
allocated memory for module shoulde be released. I changes the
allocation for module to use devm_kzalloc to be resource_managed
allocation and avoid the release in error path.

Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
Changes in v2:
	- Changed the allocation for module from kzalloc to devm_kzalloc
---
 sound/soc/intel/skylake/skl-sst-utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/intel/skylake/skl-sst-utils.c b/sound/soc/intel/skylake/skl-sst-utils.c
index d43cbf4a71ef..ac37f04b0eea 100644
--- a/sound/soc/intel/skylake/skl-sst-utils.c
+++ b/sound/soc/intel/skylake/skl-sst-utils.c
@@ -284,7 +284,7 @@ int snd_skl_parse_uuids(struct sst_dsp *ctx, const struct firmware *fw,
 	 */
 
 	for (i = 0; i < num_entry; i++, mod_entry++) {
-		module = kzalloc(sizeof(*module), GFP_KERNEL);
+		module = devm_kzalloc(ctx->dev, sizeof(*module), GFP_KERNEL);
 		if (!module) {
 			ret = -ENOMEM;
 			goto free_uuid_list;
-- 
2.17.1

Re: [PATCH v2] ASoC: Intel: Skylake: prevent memory leak in snd_skl_parse_uuids

[Pierre-Louis Bossart]

On 9/25/19 11:19 AM, Navid Emamdoost wrote:
> In snd_skl_parse_uuids if allocation for module->instance_id fails, the
> allocated memory for module shoulde be released. I changes the
> allocation for module to use devm_kzalloc to be resource_managed
> allocation and avoid the release in error path.

if you use devm_, don't you need to fix the error path as well then, I 
see a kfree(uuid) in skl_freeup_uuid_list().

I am not very familiar with this code but the error seems to be that the 
list_add_tail() is called after the module->instance_id is allocated, so 
there is a risk that the module allocated earlier is not freed (since 
it's not yet added to the list). Freeing the module as done in patch 1 
works, using devm_ without fixing the error path does not seem correct 
to me.

> 
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
> Changes in v2:
> 	- Changed the allocation for module from kzalloc to devm_kzalloc
> ---
>   sound/soc/intel/skylake/skl-sst-utils.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/intel/skylake/skl-sst-utils.c b/sound/soc/intel/skylake/skl-sst-utils.c
> index d43cbf4a71ef..ac37f04b0eea 100644
> --- a/sound/soc/intel/skylake/skl-sst-utils.c
> +++ b/sound/soc/intel/skylake/skl-sst-utils.c
> @@ -284,7 +284,7 @@ int snd_skl_parse_uuids(struct sst_dsp *ctx, const struct firmware *fw,
>   	 */
>   
>   	for (i = 0; i < num_entry; i++, mod_entry++) {
> -		module = kzalloc(sizeof(*module), GFP_KERNEL);
> +		module = devm_kzalloc(ctx->dev, sizeof(*module), GFP_KERNEL);
>   		if (!module) {
>   			ret = -ENOMEM;
>   			goto free_uuid_list;
> 

Re: [PATCH v2] ASoC: Intel: Skylake: prevent memory leak in snd_skl_parse_uuids

[Navid Emamdoost]

On Wed, Sep 25, 2019 at 12:05:28PM -0500, Pierre-Louis Bossart wrote:
> On 9/25/19 11:19 AM, Navid Emamdoost wrote:
> > In snd_skl_parse_uuids if allocation for module->instance_id fails, the
> > allocated memory for module shoulde be released. I changes the
> > allocation for module to use devm_kzalloc to be resource_managed
> > allocation and avoid the release in error path.
> 
> if you use devm_, don't you need to fix the error path as well then, I see a
> kfree(uuid) in skl_freeup_uuid_list().
> 
> I am not very familiar with this code but the error seems to be that the
> list_add_tail() is called after the module->instance_id is allocated, so
> there is a risk that the module allocated earlier is not freed (since it's
> not yet added to the list). Freeing the module as done in patch 1 works,
> using devm_ without fixing the error path does not seem correct to me.
> 
Thanks for the feedback, then it's your call if you can accept patch 1 as
fix.

Navid.
> > 
> > Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> > ---
> > Changes in v2:
> > 	- Changed the allocation for module from kzalloc to devm_kzalloc
> > ---
> >   sound/soc/intel/skylake/skl-sst-utils.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/sound/soc/intel/skylake/skl-sst-utils.c b/sound/soc/intel/skylake/skl-sst-utils.c
> > index d43cbf4a71ef..ac37f04b0eea 100644
> > --- a/sound/soc/intel/skylake/skl-sst-utils.c
> > +++ b/sound/soc/intel/skylake/skl-sst-utils.c
> > @@ -284,7 +284,7 @@ int snd_skl_parse_uuids(struct sst_dsp *ctx, const struct firmware *fw,
> >   	 */
> >   	for (i = 0; i < num_entry; i++, mod_entry++) {
> > -		module = kzalloc(sizeof(*module), GFP_KERNEL);
> > +		module = devm_kzalloc(ctx->dev, sizeof(*module), GFP_KERNEL);
> >   		if (!module) {
> >   			ret = -ENOMEM;
> >   			goto free_uuid_list;
> > 
> 

Re: [PATCH v2] ASoC: Intel: Skylake: prevent memory leak in snd_skl_parse_uuids

[Pierre-Louis Bossart]

On 9/26/19 9:55 PM, Navid Emamdoost wrote:
> On Wed, Sep 25, 2019 at 12:05:28PM -0500, Pierre-Louis Bossart wrote:
>> On 9/25/19 11:19 AM, Navid Emamdoost wrote:
>>> In snd_skl_parse_uuids if allocation for module->instance_id fails, the
>>> allocated memory for module shoulde be released. I changes the
>>> allocation for module to use devm_kzalloc to be resource_managed
>>> allocation and avoid the release in error path.
>>
>> if you use devm_, don't you need to fix the error path as well then, I see a
>> kfree(uuid) in skl_freeup_uuid_list().
>>
>> I am not very familiar with this code but the error seems to be that the
>> list_add_tail() is called after the module->instance_id is allocated, so
>> there is a risk that the module allocated earlier is not freed (since it's
>> not yet added to the list). Freeing the module as done in patch 1 works,
>> using devm_ without fixing the error path does not seem correct to me.
>>
> Thanks for the feedback, then it's your call if you can accept patch 1 as
> fix.

Cezary, it's really your call.

> Navid.
>>>
>>> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
>>> ---
>>> Changes in v2:
>>> 	- Changed the allocation for module from kzalloc to devm_kzalloc
>>> ---
>>>    sound/soc/intel/skylake/skl-sst-utils.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/sound/soc/intel/skylake/skl-sst-utils.c b/sound/soc/intel/skylake/skl-sst-utils.c
>>> index d43cbf4a71ef..ac37f04b0eea 100644
>>> --- a/sound/soc/intel/skylake/skl-sst-utils.c
>>> +++ b/sound/soc/intel/skylake/skl-sst-utils.c
>>> @@ -284,7 +284,7 @@ int snd_skl_parse_uuids(struct sst_dsp *ctx, const struct firmware *fw,
>>>    	 */
>>>    	for (i = 0; i < num_entry; i++, mod_entry++) {
>>> -		module = kzalloc(sizeof(*module), GFP_KERNEL);
>>> +		module = devm_kzalloc(ctx->dev, sizeof(*module), GFP_KERNEL);
>>>    		if (!module) {
>>>    			ret = -ENOMEM;
>>>    			goto free_uuid_list;
>>>
>>

Re: [PATCH v2] ASoC: Intel: Skylake: prevent memory leak in snd_skl_parse_uuids

[Cezary Rojewski]

On 2019-09-27 15:14, Pierre-Louis Bossart wrote:
> On 9/26/19 9:55 PM, Navid Emamdoost wrote:
>> On Wed, Sep 25, 2019 at 12:05:28PM -0500, Pierre-Louis Bossart wrote:
>>> On 9/25/19 11:19 AM, Navid Emamdoost wrote:
>>>> In snd_skl_parse_uuids if allocation for module->instance_id fails, the
>>>> allocated memory for module shoulde be released. I changes the
>>>> allocation for module to use devm_kzalloc to be resource_managed
>>>> allocation and avoid the release in error path.
>>>
>>> if you use devm_, don't you need to fix the error path as well then, 
>>> I see a
>>> kfree(uuid) in skl_freeup_uuid_list().
>>>
>>> I am not very familiar with this code but the error seems to be that the
>>> list_add_tail() is called after the module->instance_id is allocated, so
>>> there is a risk that the module allocated earlier is not freed (since 
>>> it's
>>> not yet added to the list). Freeing the module as done in patch 1 works,
>>> using devm_ without fixing the error path does not seem correct to me.
>>>

Good catch, Pierre.

>> Thanks for the feedback, then it's your call if you can accept patch 1 as
>> fix.
> 
> Cezary, it's really your call.
> 

Actually, not the best person to ask about "objective decisions" here as 
my vision is clouded by changes done internally. This code no longer 
exists in our internal repo. It's better for host to send MODULE_INFO 
request rather than understanding firmware binary structure and parse it 
directly.

I'm fine with solution #1 as I guess asking to wait for refactor is not 
an option. Code deployment is delayed due to range of administrative 
decisions, some of which should be uncovered on alsa-devel soon enough.

Czarek

Re: [PATCH v2] ASoC: Intel: Skylake: prevent memory leak in snd_skl_parse_uuids

[Andy Shevchenko]

On Fri, Sep 27, 2019 at 05:10:18PM +0200, Cezary Rojewski wrote:

> I'm fine with solution #1 as I guess asking to wait for refactor is not an
> option. Code deployment is delayed due to range of administrative decisions,
> some of which should be uncovered on alsa-devel soon enough.

The problem with solution #1 is freeing orphaned pointer. It will work,
but it's simple is not okay from object life time prospective.

-- 
With Best Regards,
Andy Shevchenko

Re: [alsa-devel] [PATCH v2] ASoC: Intel: Skylake: prevent memory leak in snd_skl_parse_uuids

[Pierre-Louis Bossart]

> The problem with solution #1 is freeing orphaned pointer. It will work,
> but it's simple is not okay from object life time prospective.

?? I don't get your point at all Andy.
Two allocations happens in a loop and if the second fails, you free the 
first and then jump to free everything allocated in the previous 
iterations. what am I missing?

Re: [alsa-devel] [PATCH v2] ASoC: Intel: Skylake: prevent memory leak in snd_skl_parse_uuids

[Andy Shevchenko]

On Fri, Sep 27, 2019 at 7:39 PM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
> > The problem with solution #1 is freeing orphaned pointer. It will work,
> > but it's simple is not okay from object life time prospective.
>
> ?? I don't get your point at all Andy.
> Two allocations happens in a loop and if the second fails, you free the
> first and then jump to free everything allocated in the previous
> iterations. what am I missing?

Two things:
 - one allocation is done with kzalloc(), while the other one with
devm_kcalloc()
 - due to above the ordering of resources is reversed

-- 
With Best Regards,
Andy Shevchenko

Re: [alsa-devel] [PATCH v2] ASoC: Intel: Skylake: prevent memory leak in snd_skl_parse_uuids

[Pierre-Louis Bossart]

On 9/27/19 3:39 PM, Andy Shevchenko wrote:
> On Fri, Sep 27, 2019 at 7:39 PM Pierre-Louis Bossart
> <pierre-louis.bossart@linux.intel.com> wrote:
>>> The problem with solution #1 is freeing orphaned pointer. It will work,
>>> but it's simple is not okay from object life time prospective.
>>
>> ?? I don't get your point at all Andy.
>> Two allocations happens in a loop and if the second fails, you free the
>> first and then jump to free everything allocated in the previous
>> iterations. what am I missing?
> 
> Two things:
>   - one allocation is done with kzalloc(), while the other one with
> devm_kcalloc()
>   - due to above the ordering of resources is reversed

Ah yes, I see your point now, sorry for being thick.
Indeed it'd make sense to use devm_ for both allocations, but then the 
kfree needs to be removed in the error handling.