Jason Gunthorpe <[email protected]> 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
>
------------------------------------------------------------------------------
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
