[PATCH] tpm: fix crash in tpm_tis

[PATCH] tpm: fix crash in tpm_tis

[Jarkko Sakkinen]

rmmod crashes the driver because tpm_chip_unregister() already sets ops
to NULL. Release ops in tpm_dev_release() so that tpm2_shutdown() can be
cleanly executed and also because it is symmetrical where they are
allocated (in tpmm_chip_alloc()).

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Fixes: 4d3eac5e156a ("tpm: Provide strong locking for device removal")
---
 drivers/char/tpm/tpm-chip.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index f62c851..5241bc4 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -127,6 +127,11 @@ static void tpm_dev_release(struct device *dev)
 	idr_remove(&dev_nums_idr, chip->dev_num);
 	mutex_unlock(&idr_lock);
 
+	/* Make the driver uncallable. */
+	down_write(&chip->ops_sem);
+	chip->ops = NULL;
+	up_write(&chip->ops_sem);
+
 	kfree(chip);
 }
 
@@ -266,11 +271,6 @@ static void tpm_del_char_device(struct tpm_chip *chip)
 	mutex_lock(&idr_lock);
 	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
 	mutex_unlock(&idr_lock);
-
-	/* Make the driver uncallable. */
-	down_write(&chip->ops_sem);
-	chip->ops = NULL;
-	up_write(&chip->ops_sem);
 }
 
 static int tpm1_chip_register(struct tpm_chip *chip)
-- 
1.9.1

Re: [PATCH] tpm: fix crash in tpm_tis

[Jason Gunthorpe]

[-- Attachment #1: Type: text/html, Size: 228 bytes --]

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

------------------------------------------------------------------------------

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

Re: [PATCH] tpm: fix crash in tpm_tis

[Jarkko Sakkinen]

On Thu, Apr 07, 2016 at 07:36:54AM -0700, Jason Gunthorpe wrote:
>    I will have to look closer after the conference, but this does not look
>    right.
> 
>    I vaguely recall commenting on this before. Move the shutdown into the
>    core code to fix it.

This fix that I sent is not the right way to do it.

One example scenario:

1. TIS driver gets detached, which causes tpm_tis_remove() to be called.
2. Some in-kernel subsystem uses TPM, which should not be done since the
   hardware is already unitialized.
3. The devres subsystem sets ops to NULL.

Even though the fix is wrong I feel that it might put the rwsem into
question.

I'm just thinking that maybe there could be a release callback in
tpm_class_ops that could be called by tpm_del_char_device(). There can't
be clients for the chip at that point so no synchronization mechanism
is needed.

>    Jason

/Jarkko

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial! http://pubads.g.doubleclick.net/
gampad/clk?id=1444514301&iu=/ca-pub-7940484522588532

Re: [PATCH] tpm: fix crash in tpm_tis

[Jarkko Sakkinen]

On Mon, Apr 11, 2016 at 11:41:24AM +0300, Jarkko Sakkinen wrote:
> On Thu, Apr 07, 2016 at 07:36:54AM -0700, Jason Gunthorpe wrote:
> >    I will have to look closer after the conference, but this does not look
> >    right.
> > 
> >    I vaguely recall commenting on this before. Move the shutdown into the
> >    core code to fix it.
> 
> This fix that I sent is not the right way to do it.
> 
> One example scenario:
> 
> 1. TIS driver gets detached, which causes tpm_tis_remove() to be called.
> 2. Some in-kernel subsystem uses TPM, which should not be done since the
>    hardware is already unitialized.
> 3. The devres subsystem sets ops to NULL.
> 
> Even though the fix is wrong I feel that it might put the rwsem into
> question.
> 
> I'm just thinking that maybe there could be a release callback in
> tpm_class_ops that could be called by tpm_del_char_device(). There can't
> be clients for the chip at that point so no synchronization mechanism
> is needed.

As a fix for this regression moving shutdown to tmp_chip_unregister() does
make more sense since the patch is already merged to next. Lets not get
stuck into locking discussion...

/Jarkko