Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

["Stefan Berger" <stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3150 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote on 02/12/2016 
05:46:42 PM:

> 
> On Fri, Feb 12, 2016 at 05:23:16PM -0500, Stefan Berger wrote:
> 
> >    > I don't see how that can happen. Looking at the tpm cdev, it is
> 
> >    Well, following my tests, it does happen.
> 
> How are you testing for use-after-free bugs? Did you test the kapi
> interface? Did you get it in *exactly* the right configuration to
> cause this?

I think there are two cases that matter for the final unwind:

1) server side closes last
2) client side closes last

Any other cases?

Here's what happens in these cases:

1) we unwind with vtpm_delete_device_pair + vtpm_delete_vtpm_dev [see 
below]. The chip an vtpm_dev are freed, in that order by just these 
functions.

2) we unwind in tpm_release and the following kicks it off:

http://lxr.free-electrons.com/source/drivers/char/tpm/tpm-dev.c#L169

169         put_device(priv->chip->pdev);
170         kfree(priv);
171         return 0;
172 }
173 

The above put_device (in the front-end) releases vtpm_dev first and then 
after tpm_release finishes the chip is released (following the cdev's 
end). 

Once tpm_release happened, we closed the file descriptor and there's no 
more code doing other business in either the front or backend drivers 
other than unwinding.

   Stefan

> 
> >    > 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.
> 
> device_unregister will put_device(vtpmt_dev) which will kfree it since
> no refs are left.
> 
> tpm_chip is still alive because the cdev/kapi are still holding a ref
> on it.
> 
> The cdev/kapi can still call vtpm_tpm_op_send which will then deref
> chip->priv which is the free'd vtpm_dev.
> 
> Any attempt to test for this scenario is very likely to hit the
> STATE_OPENED_BIT test and exit without crashing. However that is just
> a user-after free getting lucky. Testing is not a substitute for
> proper analysis.
> 
> Show me an analysis of what in cdev/kapi is holding the kref on
> vtpm_dev if you still think this can't happen.
> 
> As I said, the only kref the tpm core takes on vtpm_dev is dropped
> by tpm_chip_unregister.
> 
> Jason
> 



[-- Attachment #1.2: Type: text/html, Size: 6115 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

------------------------------------------------------------------------------
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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel