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

[Wen Yang]

On 2019/11/20 9:06 äå, Alexander Shishkin wrote:
> Wen Yang <wenyang@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx>
> > Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> 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