[PATCH] intel_th: avoid double free in error flow

[PATCH] intel_th: avoid double free in error flow

[Wen Yang]

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

Re: [PATCH] intel_th: avoid double free in error flow

[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

Re: [PATCH] intel_th: avoid double free in error flow

[Wen Yang]

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

Re: [PATCH] intel_th: avoid double free in error flow

[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

Re: [PATCH] intel_th: avoid double free in error flow

[Wen Yang]

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