linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] intel_th: avoid double free in error flow
@ 2019-11-19 17:34 Wen Yang
  2019-11-20 13:06 ` Alexander Shishkin
  0 siblings, 1 reply; 5+ messages in thread
From: Wen Yang @ 2019-11-19 17:34 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: zhiche.yy, xlpang, Wen Yang, Greg Kroah-Hartman, linux-kernel

There is a possible double free issue in intel_th_subdevice_alloc:

651         err = intel_th_device_add_resources(thdev, res, subdev->nres);
652         if (err) {
653                 put_device(&thdev->dev);
654                 goto fail_put_device;     ---> freed
655         }
...
687 fail_put_device:
688         put_device(&thdev->dev);          ---> double freed
689

This patch fix it by removing the unnecessary put_device().

Fixes: a753bfcfdb1f ("intel_th: Make the switch allocate its subdevices")
Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org
---
 drivers/hwtracing/intel_th/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/hwtracing/intel_th/core.c b/drivers/hwtracing/intel_th/core.c
index d5c1821..98d195c 100644
--- a/drivers/hwtracing/intel_th/core.c
+++ b/drivers/hwtracing/intel_th/core.c
@@ -649,10 +649,8 @@ static inline void intel_th_request_hub_module_flush(struct intel_th *th)
 	}
 
 	err = intel_th_device_add_resources(thdev, res, subdev->nres);
-	if (err) {
-		put_device(&thdev->dev);
+	if (err)
 		goto fail_put_device;
-	}
 
 	if (subdev->type == INTEL_TH_OUTPUT) {
 		if (subdev->mknode)
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] intel_th: avoid double free in error flow
  2019-11-19 17:34 [PATCH] intel_th: avoid double free in error flow Wen Yang
@ 2019-11-20 13:06 ` Alexander Shishkin
  2019-11-20 13:31   ` Wen Yang
  0 siblings, 1 reply; 5+ messages in thread
From: Alexander Shishkin @ 2019-11-20 13:06 UTC (permalink / raw)
  To: Wen Yang
  Cc: zhiche.yy, xlpang, Wen Yang, Greg Kroah-Hartman, linux-kernel,
	alexander.shishkin

Wen Yang <wenyang@linux.alibaba.com> writes:

> There is a possible double free issue in intel_th_subdevice_alloc:
>
> 651         err = intel_th_device_add_resources(thdev, res, subdev->nres);
> 652         if (err) {
> 653                 put_device(&thdev->dev);
> 654                 goto fail_put_device;     ---> freed
> 655         }
> ...
> 687 fail_put_device:
> 688         put_device(&thdev->dev);          ---> double freed
> 689
>
> This patch fix it by removing the unnecessary put_device().

Unnecessary is a too generous term here.

> Fixes: a753bfcfdb1f ("intel_th: Make the switch allocate its subdevices")
> Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-kernel@vger.kernel.org

Cc: stable@ is missing.

> ---
>  drivers/hwtracing/intel_th/core.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/hwtracing/intel_th/core.c b/drivers/hwtracing/intel_th/core.c
> index d5c1821..98d195c 100644
> --- a/drivers/hwtracing/intel_th/core.c
> +++ b/drivers/hwtracing/intel_th/core.c
> @@ -649,10 +649,8 @@ static inline void intel_th_request_hub_module_flush(struct intel_th *th)
>  	}
>  
>  	err = intel_th_device_add_resources(thdev, res, subdev->nres);
> -	if (err) {
> -		put_device(&thdev->dev);
> +	if (err)
>  		goto fail_put_device;
> -	}

What about the second instance of the same problem a few lines lower?

Thanks,
--
Alex

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] intel_th: avoid double free in error flow
  2019-11-20 13:06 ` Alexander Shishkin
@ 2019-11-20 13:31   ` Wen Yang
  2019-11-20 13:38     ` Alexander Shishkin
  0 siblings, 1 reply; 5+ messages in thread
From: Wen Yang @ 2019-11-20 13:31 UTC (permalink / raw)
  To: Alexander Shishkin; +Cc: zhiche.yy, xlpang, Greg Kroah-Hartman, linux-kernel


On 2019/11/20 9:06 下午, Alexander Shishkin wrote:
> Wen Yang <wenyang@linux.alibaba.com> writes:
>
>> There is a possible double free issue in intel_th_subdevice_alloc:
>>
>> 651         err = intel_th_device_add_resources(thdev, res, subdev->nres);
>> 652         if (err) {
>> 653                 put_device(&thdev->dev);
>> 654                 goto fail_put_device;     ---> freed
>> 655         }
>> ...
>> 687 fail_put_device:
>> 688         put_device(&thdev->dev);          ---> double freed
>> 689
>>
>> This patch fix it by removing the unnecessary put_device().
> Unnecessary is a too generous term here.
>
>> Fixes: a753bfcfdb1f ("intel_th: Make the switch allocate its subdevices")
>> Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: linux-kernel@vger.kernel.org
> Cc: stable@ is missing.
>
>> ---
>>   drivers/hwtracing/intel_th/core.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/hwtracing/intel_th/core.c b/drivers/hwtracing/intel_th/core.c
>> index d5c1821..98d195c 100644
>> --- a/drivers/hwtracing/intel_th/core.c
>> +++ b/drivers/hwtracing/intel_th/core.c
>> @@ -649,10 +649,8 @@ static inline void intel_th_request_hub_module_flush(struct intel_th *th)
>>   	}
>>   
>>   	err = intel_th_device_add_resources(thdev, res, subdev->nres);
>> -	if (err) {
>> -		put_device(&thdev->dev);
>> +	if (err)
>>   		goto fail_put_device;
>> -	}
> What about the second instance of the same problem a few lines lower?
> Thanks,
> --
> Alex

Hi Alex,

Thank you for your comments.

Another example after a few lines lower:

         err = device_add(&thdev->dev);

         if (err) {
                  put_device(&thdev->dev);
                  goto fail_free_res;

          }

device_add() has increased the reference count,

so when it returns an error, an additional call to put_device()

is needed here to reduce the reference count.

So the code in this place is correct.


--

Regards,

Wen




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] intel_th: avoid double free in error flow
  2019-11-20 13:31   ` Wen Yang
@ 2019-11-20 13:38     ` Alexander Shishkin
  2019-11-20 13:46       ` Wen Yang
  0 siblings, 1 reply; 5+ messages in thread
From: Alexander Shishkin @ 2019-11-20 13:38 UTC (permalink / raw)
  To: Wen Yang
  Cc: zhiche.yy, xlpang, Greg Kroah-Hartman, linux-kernel, alexander.shishkin

Wen Yang <wenyang@linux.alibaba.com> writes:

> Another example after a few lines lower:
>
>          err = device_add(&thdev->dev);
>
>          if (err) {
>                   put_device(&thdev->dev);
>                   goto fail_free_res;
>
>           }
>
> device_add() has increased the reference count,
>
> so when it returns an error, an additional call to put_device()
>
> is needed here to reduce the reference count.
>
> So the code in this place is correct.

No, device_add() drops its own extra reference in case of error (as it
should), so in "if (err) ..." branch we still only have just one
reference before it goes free.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] intel_th: avoid double free in error flow
  2019-11-20 13:38     ` Alexander Shishkin
@ 2019-11-20 13:46       ` Wen Yang
  0 siblings, 0 replies; 5+ messages in thread
From: Wen Yang @ 2019-11-20 13:46 UTC (permalink / raw)
  To: Alexander Shishkin; +Cc: zhiche.yy, xlpang, Greg Kroah-Hartman, linux-kernel


On 2019/11/20 9:38 下午, Alexander Shishkin wrote:
> Wen Yang <wenyang@linux.alibaba.com> writes:
>
>> Another example after a few lines lower:
>>
>>           err = device_add(&thdev->dev);
>>
>>           if (err) {
>>                    put_device(&thdev->dev);
>>                    goto fail_free_res;
>>
>>            }
>>
>> device_add() has increased the reference count,
>>
>> so when it returns an error, an additional call to put_device()
>>
>> is needed here to reduce the reference count.
>>
>> So the code in this place is correct.
> No, device_add() drops its own extra reference in case of error (as it
> should), so in "if (err) ..." branch we still only have just one
> reference before it goes free.
>
> Regards,
> --
> Alex



Well, ok, you are right.

We just checked the code and device_add() does release the reference count.

Thank you.


--

Regards,

Wen





^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-11-20 13:46 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-19 17:34 [PATCH] intel_th: avoid double free in error flow Wen Yang
2019-11-20 13:06 ` Alexander Shishkin
2019-11-20 13:31   ` Wen Yang
2019-11-20 13:38     ` Alexander Shishkin
2019-11-20 13:46       ` Wen Yang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).