Jason Gunthorpe <[email protected]> wrote on 02/12/2016
04:15:38 PM:
> From: Jason Gunthorpe <[email protected]>
> To: Stefan Berger/Watson/IBM@IBMUS
> Cc: [email protected], [email protected], Jarkko Sakkinen
> <[email protected]>, [email protected]
> 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(struct vtpm_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 void vtpm_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
>
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
tpmdd-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
