[PATCH] of: overlay: fix memory leak of ovcs on error exit path

[PATCH] of: overlay: fix memory leak of ovcs on error exit path

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

Currently if the call to of_resolve_phandles fails then then ovcs
is not kfree'd on the error exit path.  Rather than try and make
the clean up exit path more convoluted, fix this by just kfree'ing
ovcs at the point of error detection and exit via the same exit
path.

Detected by CoverityScan, CID#1462296 ("Resource Leak")

Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/of/overlay.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 53bc9e3f0b98..6c8efe7d8cbb 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
 	of_overlay_mutex_lock();
 
 	ret = of_resolve_phandles(tree);
-	if (ret)
+	if (ret) {
+		kfree(ovcs);
 		goto err_overlay_unlock;
+	}
 
 	mutex_lock(&of_mutex);
 
-- 
2.14.1

Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path

[Frank Rowand]

On 11/29/17 14:17, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently if the call to of_resolve_phandles fails then then ovcs
> is not kfree'd on the error exit path.  Rather than try and make
> the clean up exit path more convoluted, fix this by just kfree'ing
> ovcs at the point of error detection and exit via the same exit
> path.
> 
> Detected by CoverityScan, CID#1462296 ("Resource Leak")
> 
> Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/of/overlay.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 53bc9e3f0b98..6c8efe7d8cbb 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>  	of_overlay_mutex_lock();
>  
>  	ret = of_resolve_phandles(tree);
> -	if (ret)
> +	if (ret) {
> +		kfree(ovcs);
>  		goto err_overlay_unlock;
> +	}
>  
>  	mutex_lock(&of_mutex);
>  
> 

False coverity warning.  ovcs is freed in free_overlay_changeset().

Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path

[Colin Ian King]

On 30/11/17 12:14, Frank Rowand wrote:
> On 11/29/17 14:17, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Currently if the call to of_resolve_phandles fails then then ovcs
>> is not kfree'd on the error exit path.  Rather than try and make
>> the clean up exit path more convoluted, fix this by just kfree'ing
>> ovcs at the point of error detection and exit via the same exit
>> path.
>>
>> Detected by CoverityScan, CID#1462296 ("Resource Leak")
>>
>> Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  drivers/of/overlay.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 53bc9e3f0b98..6c8efe7d8cbb 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>>  	of_overlay_mutex_lock();
>>  
>>  	ret = of_resolve_phandles(tree);
>> -	if (ret)
>> +	if (ret) {
>> +		kfree(ovcs);
>>  		goto err_overlay_unlock;
>> +	}
>>  
>>  	mutex_lock(&of_mutex);
>>  
>>
> 
> False coverity warning.  ovcs is freed in free_overlay_changeset().
> 

The error exit path is via err_overlay_unlock:

err_overlay_unlock:
        of_overlay_mutex_unlock();

out:
        pr_debug("%s() err=%d\n", __func__, ret);

        return ret;

..so there is no call to free_overlay_changeset there.

Colin

Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path

[Frank Rowand]

Hi Colin, Rob,

On 11/30/17 07:18, Colin Ian King wrote:
> On 30/11/17 12:14, Frank Rowand wrote:
>> On 11/29/17 14:17, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> Currently if the call to of_resolve_phandles fails then then ovcs
>>> is not kfree'd on the error exit path.  Rather than try and make
>>> the clean up exit path more convoluted, fix this by just kfree'ing
>>> ovcs at the point of error detection and exit via the same exit
>>> path.
>>>
>>> Detected by CoverityScan, CID#1462296 ("Resource Leak")
>>>
>>> Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> ---
>>>  drivers/of/overlay.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>> index 53bc9e3f0b98..6c8efe7d8cbb 100644
>>> --- a/drivers/of/overlay.c
>>> +++ b/drivers/of/overlay.c
>>> @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>>>  	of_overlay_mutex_lock();
>>>  
>>>  	ret = of_resolve_phandles(tree);
>>> -	if (ret)
>>> +	if (ret) {
>>> +		kfree(ovcs);
>>>  		goto err_overlay_unlock;
>>> +	}
>>>  
>>>  	mutex_lock(&of_mutex);
>>>  
>>>
>>
>> False coverity warning.  ovcs is freed in free_overlay_changeset().
>>
> 
> The error exit path is via err_overlay_unlock:
> 
> err_overlay_unlock:
>         of_overlay_mutex_unlock();
> 
> out:
>         pr_debug("%s() err=%d\n", __func__, ret);
> 
>         return ret;
> 
> ..so there is no call to free_overlay_changeset there.
> 
> Colin
> 

OK, I was looking at 4.15-rc1.  You must be looking at a later version where
"[PATCH 1/2] of: overlay: Fix cleanup order in of_overlay_apply()" has been
applied.  Thanks for providing the extra details about the exit path so I
could see that.

Rob, I think that the fix for cleanup order was not the best way to fix that
problem.  A better method would have been to move "mutex_lock(&of_mutex);"
up 5 lines, to just before calling of_reserve_phandles().  The problem
found by coverity was caused by the "Fix cleanup order" patch.

I can create that alternate fix if you would like, but I am traveling
right now and don't want to submit a patch without boot testing, so
there will be a slight delay.

-Frank

Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path

[Frank Rowand]

On 11/30/17 08:37, Frank Rowand wrote:
> Hi Colin, Rob,
> 
> On 11/30/17 07:18, Colin Ian King wrote:
>> On 30/11/17 12:14, Frank Rowand wrote:
>>> On 11/29/17 14:17, Colin King wrote:
>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>
>>>> Currently if the call to of_resolve_phandles fails then then ovcs
>>>> is not kfree'd on the error exit path.  Rather than try and make
>>>> the clean up exit path more convoluted, fix this by just kfree'ing
>>>> ovcs at the point of error detection and exit via the same exit
>>>> path.
>>>>
>>>> Detected by CoverityScan, CID#1462296 ("Resource Leak")
>>>>
>>>> Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>>> ---
>>>>  drivers/of/overlay.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>> index 53bc9e3f0b98..6c8efe7d8cbb 100644
>>>> --- a/drivers/of/overlay.c
>>>> +++ b/drivers/of/overlay.c
>>>> @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>>>>  	of_overlay_mutex_lock();
>>>>  
>>>>  	ret = of_resolve_phandles(tree);
>>>> -	if (ret)
>>>> +	if (ret) {
>>>> +		kfree(ovcs);
>>>>  		goto err_overlay_unlock;
>>>> +	}
>>>>  
>>>>  	mutex_lock(&of_mutex);
>>>>  
>>>>
>>>
>>> False coverity warning.  ovcs is freed in free_overlay_changeset().
>>>
>>
>> The error exit path is via err_overlay_unlock:
>>
>> err_overlay_unlock:
>>         of_overlay_mutex_unlock();
>>
>> out:
>>         pr_debug("%s() err=%d\n", __func__, ret);
>>
>>         return ret;
>>
>> ..so there is no call to free_overlay_changeset there.
>>
>> Colin
>>
> 
> OK, I was looking at 4.15-rc1.  You must be looking at a later version where
> "[PATCH 1/2] of: overlay: Fix cleanup order in of_overlay_apply()" has been
> applied.  Thanks for providing the extra details about the exit path so I
> could see that.
> 
> Rob, I think that the fix for cleanup order was not the best way to fix that
> problem.  A better method would have been to move "mutex_lock(&of_mutex);"
> up 5 lines, to just before calling of_reserve_phandles().

It is getting late (midnight my time), so I really should revisit this all
tomorrow.  My last comment ("move ... up 5 lines") is probably wrong. 

I'll look at this after some sleep.


> The problem
> found by coverity was caused by the "Fix cleanup order" patch.
> 
> I can create that alternate fix if you would like, but I am traveling
> right now and don't want to submit a patch without boot testing, so
> there will be a slight delay.
> 
> -Frank

Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path

[Rob Herring]

On Thu, Nov 30, 2017 at 9:01 AM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 11/30/17 08:37, Frank Rowand wrote:
>> Hi Colin, Rob,
>>
>> On 11/30/17 07:18, Colin Ian King wrote:
>>> On 30/11/17 12:14, Frank Rowand wrote:
>>>> On 11/29/17 14:17, Colin King wrote:
>>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>>
>>>>> Currently if the call to of_resolve_phandles fails then then ovcs
>>>>> is not kfree'd on the error exit path.  Rather than try and make
>>>>> the clean up exit path more convoluted, fix this by just kfree'ing
>>>>> ovcs at the point of error detection and exit via the same exit
>>>>> path.
>>>>>
>>>>> Detected by CoverityScan, CID#1462296 ("Resource Leak")
>>>>>
>>>>> Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
>>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>>>> ---
>>>>>  drivers/of/overlay.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>>> index 53bc9e3f0b98..6c8efe7d8cbb 100644
>>>>> --- a/drivers/of/overlay.c
>>>>> +++ b/drivers/of/overlay.c
>>>>> @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>>>>>    of_overlay_mutex_lock();
>>>>>
>>>>>    ret = of_resolve_phandles(tree);
>>>>> -  if (ret)
>>>>> +  if (ret) {
>>>>> +          kfree(ovcs);
>>>>>            goto err_overlay_unlock;
>>>>> +  }
>>>>>
>>>>>    mutex_lock(&of_mutex);
>>>>>
>>>>>
>>>>
>>>> False coverity warning.  ovcs is freed in free_overlay_changeset().
>>>>
>>>
>>> The error exit path is via err_overlay_unlock:
>>>
>>> err_overlay_unlock:
>>>         of_overlay_mutex_unlock();
>>>
>>> out:
>>>         pr_debug("%s() err=%d\n", __func__, ret);
>>>
>>>         return ret;
>>>
>>> ..so there is no call to free_overlay_changeset there.
>>>
>>> Colin
>>>
>>
>> OK, I was looking at 4.15-rc1.  You must be looking at a later version where
>> "[PATCH 1/2] of: overlay: Fix cleanup order in of_overlay_apply()" has been
>> applied.  Thanks for providing the extra details about the exit path so I
>> could see that.
>>
>> Rob, I think that the fix for cleanup order was not the best way to fix that
>> problem.  A better method would have been to move "mutex_lock(&of_mutex);"
>> up 5 lines, to just before calling of_reserve_phandles().
>
> It is getting late (midnight my time), so I really should revisit this all
> tomorrow.  My last comment ("move ... up 5 lines") is probably wrong.
>
> I'll look at this after some sleep.

I'm dropping "of: overlay: Fix cleanup order in of_overlay_apply()",
so someone please fix this in the original patch.

Rob

Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path

[Dan Carpenter]

On Thu, Nov 30, 2017 at 07:14:45AM -0500, Frank Rowand wrote:
> On 11/29/17 14:17, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > Currently if the call to of_resolve_phandles fails then then ovcs
> > is not kfree'd on the error exit path.  Rather than try and make
> > the clean up exit path more convoluted, fix this by just kfree'ing
> > ovcs at the point of error detection and exit via the same exit
> > path.
> > 
> > Detected by CoverityScan, CID#1462296 ("Resource Leak")
> > 
> > Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")

The Fixes tag is wrong.  It should be:

Fixes: bd80e2555c5c ("of: overlay: Fix cleanup order in of_overlay_apply()")

> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> >  drivers/of/overlay.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > index 53bc9e3f0b98..6c8efe7d8cbb 100644
> > --- a/drivers/of/overlay.c
> > +++ b/drivers/of/overlay.c
> > @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
> >  	of_overlay_mutex_lock();
> >  
> >  	ret = of_resolve_phandles(tree);
> > -	if (ret)
> > +	if (ret) {
> > +		kfree(ovcs);
> >  		goto err_overlay_unlock;
> > +	}
> >  
> >  	mutex_lock(&of_mutex);
> >  
> > 
> 
> False coverity warning.  ovcs is freed in free_overlay_changeset().

You're looking at an older version of the code and Colin is looking at
linux-next.

regards,
dan carpenter

Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path

[Colin Ian King]

On 30/11/17 12:50, Dan Carpenter wrote:
> On Thu, Nov 30, 2017 at 07:14:45AM -0500, Frank Rowand wrote:
>> On 11/29/17 14:17, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> Currently if the call to of_resolve_phandles fails then then ovcs
>>> is not kfree'd on the error exit path.  Rather than try and make
>>> the clean up exit path more convoluted, fix this by just kfree'ing
>>> ovcs at the point of error detection and exit via the same exit
>>> path.
>>>
>>> Detected by CoverityScan, CID#1462296 ("Resource Leak")
>>>
>>> Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
> 
> The Fixes tag is wrong.  It should be:
> 
> Fixes: bd80e2555c5c ("of: overlay: Fix cleanup order in of_overlay_apply()")

Good catch. I'll send a V2.

Colin

> 
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> ---
>>>  drivers/of/overlay.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>> index 53bc9e3f0b98..6c8efe7d8cbb 100644
>>> --- a/drivers/of/overlay.c
>>> +++ b/drivers/of/overlay.c
>>> @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>>>  	of_overlay_mutex_lock();
>>>  
>>>  	ret = of_resolve_phandles(tree);
>>> -	if (ret)
>>> +	if (ret) {
>>> +		kfree(ovcs);
>>>  		goto err_overlay_unlock;
>>> +	}
>>>  
>>>  	mutex_lock(&of_mutex);
>>>  
>>>
>>
>> False coverity warning.  ovcs is freed in free_overlay_changeset().
> 
> You're looking at an older version of the code and Colin is looking at
> linux-next.
> 
> regards,
> dan carpenter
> 

Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path

[Geert Uytterhoeven]

Hi Dan,

On Thu, Nov 30, 2017 at 1:50 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Thu, Nov 30, 2017 at 07:14:45AM -0500, Frank Rowand wrote:
>> On 11/29/17 14:17, Colin King wrote:
>> > From: Colin Ian King <colin.king@canonical.com>
>> >
>> > Currently if the call to of_resolve_phandles fails then then ovcs
>> > is not kfree'd on the error exit path.  Rather than try and make
>> > the clean up exit path more convoluted, fix this by just kfree'ing
>> > ovcs at the point of error detection and exit via the same exit
>> > path.
>> >
>> > Detected by CoverityScan, CID#1462296 ("Resource Leak")
>> >
>> > Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
>
> The Fixes tag is wrong.  It should be:
>
> Fixes: bd80e2555c5c ("of: overlay: Fix cleanup order in of_overlay_apply()")

Oops, sorry for that.

>> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> > ---
>> >  drivers/of/overlay.c | 4 +++-
>> >  1 file changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> > index 53bc9e3f0b98..6c8efe7d8cbb 100644
>> > --- a/drivers/of/overlay.c
>> > +++ b/drivers/of/overlay.c
>> > @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>> >     of_overlay_mutex_lock();
>> >
>> >     ret = of_resolve_phandles(tree);
>> > -   if (ret)
>> > +   if (ret) {
>> > +           kfree(ovcs);
>> >             goto err_overlay_unlock;
>> > +   }
>> >
>> >     mutex_lock(&of_mutex);
>>
>> False coverity warning.  ovcs is freed in free_overlay_changeset().
>
> You're looking at an older version of the code and Colin is looking at
> linux-next.

BTW, it's a bit confusing that sometimes ovcs must be freed manually, and
sometimes this is taken care of by free_overlay_changeset().

Especially when building on top of that, and adding more error cases in
between err_free_overlay_changeset and err_overlay_unlock.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path

[Geert Uytterhoeven]

Hi Dan,

On Thu, Nov 30, 2017 at 1:50 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Thu, Nov 30, 2017 at 07:14:45AM -0500, Frank Rowand wrote:
>> On 11/29/17 14:17, Colin King wrote:
>> > From: Colin Ian King <colin.king@canonical.com>
>> >
>> > Currently if the call to of_resolve_phandles fails then then ovcs
>> > is not kfree'd on the error exit path.  Rather than try and make
>> > the clean up exit path more convoluted, fix this by just kfree'ing
>> > ovcs at the point of error detection and exit via the same exit
>> > path.
>> >
>> > Detected by CoverityScan, CID#1462296 ("Resource Leak")
>> >
>> > Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
>
> The Fixes tag is wrong.  It should be:
>
> Fixes: bd80e2555c5c ("of: overlay: Fix cleanup order in of_overlay_apply()")
>
>> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> > ---
>> >  drivers/of/overlay.c | 4 +++-
>> >  1 file changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> > index 53bc9e3f0b98..6c8efe7d8cbb 100644
>> > --- a/drivers/of/overlay.c
>> > +++ b/drivers/of/overlay.c
>> > @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>> >     of_overlay_mutex_lock();
>> >
>> >     ret = of_resolve_phandles(tree);
>> > -   if (ret)
>> > +   if (ret) {
>> > +           kfree(ovcs);
>> >             goto err_overlay_unlock;
>> > +   }
>> >
>> >     mutex_lock(&of_mutex);
>> >
>> >
>>
>> False coverity warning.  ovcs is freed in free_overlay_changeset().
>
> You're looking at an older version of the code and Colin is looking at
> linux-next.

Actually Colin was right: the bug was introduced in commit f948d6d8b792:
While free_overlay_changeset() is called in the error path, that function
returns early because of:

        if (!ovcs->cset.entries.next)
                return;

So it never gets to freeing the passed pointer.

I'm fixing it for good (let's hope so ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds