[PATCH] tpm: fix crash in tpm_tis deinitialization

[PATCH] tpm: fix crash in tpm_tis deinitialization

[Jarkko Sakkinen]

rmmod crashes the driver because tpm_chip_unregister() already sets ops
to NULL. This commit fixes the issue by moving tpm2_shutdown() to
tpm_chip_unregister(). This commit is also cleanup because it removes
duplicate code from tpm_crb and tpm_tis to the core.

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 | 3 +++
 drivers/char/tpm/tpm_crb.c  | 3 ---
 drivers/char/tpm/tpm_tis.c  | 3 ---
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index f62c851..2642cca 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -361,6 +361,9 @@ void tpm_chip_unregister(struct tpm_chip *chip)
 	if (!(chip->flags & TPM_CHIP_FLAG_REGISTERED))
 		return;
 
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		tpm2_shutdown(chip, TPM2_SU_CLEAR);
+
 	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
 		sysfs_remove_link(&chip->dev.parent->kobj, "ppi");
 
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 20155d5..c31b5a7 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -341,9 +341,6 @@ static int crb_acpi_remove(struct acpi_device *device)
 	struct device *dev = &device->dev;
 	struct tpm_chip *chip = dev_get_drvdata(dev);
 
-	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		tpm2_shutdown(chip, TPM2_SU_CLEAR);
-
 	tpm_chip_unregister(chip);
 
 	return 0;
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 1e45e73..a6b2d46 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -681,9 +681,6 @@ static void tpm_tis_remove(struct tpm_chip *chip)
 	struct priv_data *priv = dev_get_drvdata(&chip->dev);
 	void __iomem *reg = priv->iobase + TPM_INT_ENABLE(priv->locality);
 
-	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		tpm2_shutdown(chip, TPM2_SU_CLEAR);
-
 	iowrite32(~TPM_GLOBAL_INT_ENABLE & ioread32(reg), reg);
 	release_locality(chip, priv->locality, 1);
 }
-- 
2.7.4

Re: [PATCH] tpm: fix crash in tpm_tis deinitialization

[Jason Gunthorpe]

On Mon, Apr 11, 2016 at 07:05:20PM +0300, Jarkko Sakkinen wrote:
> rmmod crashes the driver because tpm_chip_unregister() already sets ops
> to NULL. This commit fixes the issue by moving tpm2_shutdown() to
> tpm_chip_unregister(). This commit is also cleanup because it removes
> duplicate code from tpm_crb and tpm_tis to the core.
> 
> 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 | 3 +++
>  drivers/char/tpm/tpm_crb.c  | 3 ---
>  drivers/char/tpm/tpm_tis.c  | 3 ---
>  3 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index f62c851..2642cca 100644
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -361,6 +361,9 @@ void tpm_chip_unregister(struct tpm_chip *chip)
>  	if (!(chip->flags & TPM_CHIP_FLAG_REGISTERED))
>  		return;
>  
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +		tpm2_shutdown(chip, TPM2_SU_CLEAR);
> +
>  	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
>  		sysfs_remove_link(&chip->dev.parent->kobj, "ppi");

This needs to be after ops is fenced, something like this.

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 2642cca05cac..2ea2f1561e59 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -269,6 +269,8 @@ static void tpm_del_char_device(struct tpm_chip *chip)
 
 	/* Make the driver uncallable. */
 	down_write(&chip->ops_sem);
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		tpm2_shutdown(chip, TPM2_SU_CLEAR);
 	chip->ops = NULL;
 	up_write(&chip->ops_sem);
 }
@@ -361,9 +363,6 @@ void tpm_chip_unregister(struct tpm_chip *chip)
 	if (!(chip->flags & TPM_CHIP_FLAG_REGISTERED))
 		return;
 
-	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		tpm2_shutdown(chip, TPM2_SU_CLEAR);
-
 	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
 		sysfs_remove_link(&chip->dev.parent->kobj, "ppi");
 

Re: [PATCH] tpm: fix crash in tpm_tis deinitialization

[Jarkko Sakkinen]

On Mon, Apr 11, 2016 at 11:40:37AM -0600, Jason Gunthorpe wrote:
> On Mon, Apr 11, 2016 at 07:05:20PM +0300, Jarkko Sakkinen wrote:
> > rmmod crashes the driver because tpm_chip_unregister() already sets ops
> > to NULL. This commit fixes the issue by moving tpm2_shutdown() to
> > tpm_chip_unregister(). This commit is also cleanup because it removes
> > duplicate code from tpm_crb and tpm_tis to the core.
> > 
> > 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 | 3 +++
> >  drivers/char/tpm/tpm_crb.c  | 3 ---
> >  drivers/char/tpm/tpm_tis.c  | 3 ---
> >  3 files changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index f62c851..2642cca 100644
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -361,6 +361,9 @@ void tpm_chip_unregister(struct tpm_chip *chip)
> >  	if (!(chip->flags & TPM_CHIP_FLAG_REGISTERED))
> >  		return;
> >  
> > +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> > +		tpm2_shutdown(chip, TPM2_SU_CLEAR);
> > +
> >  	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
> >  		sysfs_remove_link(&chip->dev.parent->kobj, "ppi");
> 
> This needs to be after ops is fenced, something like this.

I would appreciate a supporting argument.

I guess the argument here is that this will prevent user space from
issuing TPM commands after the shutdown command has been sent?

/Jarkko

> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 2642cca05cac..2ea2f1561e59 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -269,6 +269,8 @@ static void tpm_del_char_device(struct tpm_chip *chip)
>  
>  	/* Make the driver uncallable. */
>  	down_write(&chip->ops_sem);
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +		tpm2_shutdown(chip, TPM2_SU_CLEAR);
>  	chip->ops = NULL;
>  	up_write(&chip->ops_sem);
>  }
> @@ -361,9 +363,6 @@ void tpm_chip_unregister(struct tpm_chip *chip)
>  	if (!(chip->flags & TPM_CHIP_FLAG_REGISTERED))
>  		return;
>  
> -	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> -		tpm2_shutdown(chip, TPM2_SU_CLEAR);
> -
>  	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
>  		sysfs_remove_link(&chip->dev.parent->kobj, "ppi");
>  

Re: [PATCH] tpm: fix crash in tpm_tis deinitialization

[Jason Gunthorpe]

On Tue, Apr 12, 2016 at 07:26:27AM +0300, Jarkko Sakkinen wrote:
> > This needs to be after ops is fenced, something like this.
> 
> I would appreciate a supporting argument.
> 
> I guess the argument here is that this will prevent user space from
> issuing TPM commands after the shutdown command has been sent?

It prevents everything including the kernel from issuing a command
after shutdown. The shutdown is sent as the last command and no other
commands can follow it.

It doesn't make any sense to allow commands to follow shutdown.

Jason

Re: [PATCH] tpm: fix crash in tpm_tis deinitialization

[Jarkko Sakkinen]

On Tue, Apr 12, 2016 at 11:26:55AM -0600, Jason Gunthorpe wrote:
> On Tue, Apr 12, 2016 at 07:26:27AM +0300, Jarkko Sakkinen wrote:
> > > This needs to be after ops is fenced, something like this.
> > 
> > I would appreciate a supporting argument.
> > 
> > I guess the argument here is that this will prevent user space from
> > issuing TPM commands after the shutdown command has been sent?
> 
> It prevents everything including the kernel from issuing a command
                         ^^^^^^^^^^^^^^^^^^^^

That lock is not used to exclude kernel access. Read lock is only taken
for the user space device in tpm-dev.c.

> after shutdown. The shutdown is sent as the last command and no other
> commands can follow it.
> 
> It doesn't make any sense to allow commands to follow shutdown.

I agree with this.

> Jason

/Jarkko

Re: [PATCH] tpm: fix crash in tpm_tis deinitialization

[Jarkko Sakkinen]

On Wed, Apr 13, 2016 at 09:16:51AM +0300, Jarkko Sakkinen wrote:
> On Tue, Apr 12, 2016 at 11:26:55AM -0600, Jason Gunthorpe wrote:
> > On Tue, Apr 12, 2016 at 07:26:27AM +0300, Jarkko Sakkinen wrote:
> > > > This needs to be after ops is fenced, something like this.
> > > 
> > > I would appreciate a supporting argument.
> > > 
> > > I guess the argument here is that this will prevent user space from
> > > issuing TPM commands after the shutdown command has been sent?
> > 
> > It prevents everything including the kernel from issuing a command
>                          ^^^^^^^^^^^^^^^^^^^^
> 
> That lock is not used to exclude kernel access. Read lock is only taken
> for the user space device in tpm-dev.c.

My bad. For the in-kernel API it is taken through tpm_find_get().

I'll update the fix.

/Jarkko

Re: [PATCH] tpm: fix crash in tpm_tis deinitialization

[Jason Gunthorpe]

On Wed, Apr 13, 2016 at 09:58:05AM +0300, Jarkko Sakkinen wrote:
> > > It prevents everything including the kernel from issuing a command
> >                          ^^^^^^^^^^^^^^^^^^^^
> > 
> > That lock is not used to exclude kernel access. Read lock is only taken
> > for the user space device in tpm-dev.c.
> 
> My bad. For the in-kernel API it is taken through tpm_find_get().

Right. At this point any thing that can run in-kernel concurrently
with the write exclusion is a kernel bug. Those places will need to
grab the read side of the lock.

Jason