Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/12/2016 04:15:38 PM:

> From: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> To: Stefan Berger/Watson/IBM@IBMUS
> Cc: dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org, Jarkko Sakkinen
> <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>, tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> Date: 02/12/2016 04:15 PM
> Subject: Re: [tpmdd-devel] [PATCH v5 4/5] Initialize TPM and get
> durations and timeouts
>
> On Fri, Feb 12, 2016 at 03:44:14PM -0500, Stefan Berger wrote:
> >    What I observed is that both tpm_chip and vtpm_dev structures are freed
> >    once the last one of two sides (/dev/tpmX or server side file
> >    descriptor) closes.
>
> Hmmm...
>
> I don't see how that can happen. Looking at the tpm cdev, it is

Well, following my tests, it does happen.

> continues to exist even after tpm_unregister returns (cdev_del does
> not force close existing opens). Certainly the kAPI (ie
> tpm_chip_find_get) will continue to use the chip without blocking
> tpm_unregister.
>
> I see no mechanism for the cdev/kAPI to continue to hold a kref on the
> vtpm struct.
>
> The obvious one would be because the vtpm struct is a parent of the
> chip, but that kref is let go during device_del.
>
> So, we have a situation where tpm_unregister can return, release the
> kref on the vtpm and still have outstanding users, which will result
> in a use after-free.

Maybe you should give it a try ... I don't see these problems and any change seems like ill-fixing it. What will be accessed after tpm_unregister? The only case we have tpm_unregister is here:

static void vtpm_delete_device_pair(structvtpm_dev *vtpm_dev)
{
        tpm_chip_unregister(vtpm_dev->chip);

        vtpm_fops_undo_open(vtpm_dev);

        vtpm_delete_vtpm_dev(vtpm_dev);
}

followed by:

static inline voidvtpm_delete_vtpm_dev(struct vtpm_dev *vtpm_dev)
{
        put_device(&vtpm_dev->chip->dev); /* frees chip */
        device_unregister(&vtpm_dev->dev); /* does put_device */
}

I don't see a problem with that.

>
> [BTW, your lastest vtpm on github still has a problem with error
>  unwind. Move the put_device(&vtpm_dev->chip->dev);  from
>  vtpm_delete_vtpm_dev() and put it in vtpm_dev_release() with a NULL
>  test.

I tested all the error paths. The vtpm_delete_vtpm_dev is the counter-part to vtpm_create_vtpm_dev and only ever gets called if vtpm_create_vtpm_dev ran successfully and is the function to call to unwind vtpm_create_vtpm_dev. So if we unwind there in reverse order then we should be ok. If not, why not ?

>
>  The put_device is missing after the tpm_chip_unregister call, the
>  above is the safest way to fix it.

No, it's the vtpm_delete_vtpm_dev function that unwinds it. I tested this and structures get freed properly.

>  
>  This is why you shouldn't wrapper put_device, anything but naked
>  put_device is probably wrong]
>
> [Also, err_kfree should not exist in vtpm_create_vtpm_dev, always
>  put_device after device_initialize returns, the comment near the device_add
>  is wrong, it is using the get_device done implicitly by
>  device_initialize]

Fixed.
>
> [Don't forget to error check dev_set_name]

Fixed. Looks like 80% of the drivers don't check here.

   Stefan
>
> Jason
>