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 <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
---
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
Wen Yang <[email protected]> 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 <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
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
Wen Yang <[email protected]> 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
On 2019/11/20 9:06 下午, Alexander Shishkin wrote:
> Wen Yang <[email protected]> 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 <[email protected]>
>> Cc: Alexander Shishkin <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: [email protected]
> 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
On 2019/11/20 9:38 下午, Alexander Shishkin wrote:
> Wen Yang <[email protected]> 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