[PATCH][RFC] tpm: Rework open/close/shutdown to avoid races

[PATCH][RFC] tpm: Rework open/close/shutdown to avoid races

[Sergey Temerkhanov]

Avoid race condition at shutdown by shutting downn the TPM 2.0
devices synchronously. This eliminates the condition when the
shutdown sequence sets chip->ops to NULL leading to the following:

[ 1586.593561][ T8669] tpm2_del_space+0x28/0x73
[ 1586.598718][ T8669] tpmrm_release+0x27/0x33wq
[ 1586.603774][ T8669] __fput+0x109/0x1d
[ 1586.608380][ T8669] task_work_run+0x7c/0x90
[ 1586.613414][ T8669] prepare_exit_to_usermode+0xb8/0x128
[ 1586.619522][ T8669] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 1586.626068][ T8669] RIP: 0033:0x4cb4bb

Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
---
 drivers/char/tpm/tpm-chip.c  | 27 ---------------------------
 drivers/char/tpm/tpm-dev.c   | 11 ++++++-----
 drivers/char/tpm/tpmrm-dev.c |  7 +++++++
 include/linux/tpm.h          |  2 +-
 4 files changed, 14 insertions(+), 33 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 1838039b0333..ede7f4790c5e 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -282,32 +282,6 @@ static void tpm_devs_release(struct device *dev)
 	put_device(&chip->dev);
 }
 
-/**
- * tpm_class_shutdown() - prepare the TPM device for loss of power.
- * @dev: device to which the chip is associated.
- *
- * Issues a TPM2_Shutdown command prior to loss of power, as required by the
- * TPM 2.0 spec. Then, calls bus- and device- specific shutdown code.
- *
- * Return: always 0 (i.e. success)
- */
-static int tpm_class_shutdown(struct device *dev)
-{
-	struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev);
-
-	down_write(&chip->ops_sem);
-	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-		if (!tpm_chip_start(chip)) {
-			tpm2_shutdown(chip, TPM2_SU_CLEAR);
-			tpm_chip_stop(chip);
-		}
-	}
-	chip->ops = NULL;
-	up_write(&chip->ops_sem);
-
-	return 0;
-}
-
 /**
  * tpm_chip_alloc() - allocate a new struct tpm_chip instance
  * @pdev: device to which the chip is associated
@@ -347,7 +321,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 	device_initialize(&chip->devs);
 
 	chip->dev.class = tpm_class;
-	chip->dev.class->shutdown_pre = tpm_class_shutdown;
 	chip->dev.release = tpm_dev_release;
 	chip->dev.parent = pdev;
 	chip->dev.groups = chip->groups;
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index e2c0baa69fef..e04f3d47c64e 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -23,9 +23,9 @@ static int tpm_open(struct inode *inode, struct file *file)
 	chip = container_of(inode->i_cdev, struct tpm_chip, cdev);
 
 	/* It's assured that the chip will be opened just once,
-	 * by the check of is_open variable, which is protected
-	 * by driver_lock. */
-	if (test_and_set_bit(0, &chip->is_open)) {
+	 * by the check of is_open variable
+	 */
+	if (atomic_fetch_or(1, &chip->is_open)) {
 		dev_dbg(&chip->dev, "Another process owns this TPM\n");
 		return -EBUSY;
 	}
@@ -39,7 +39,7 @@ static int tpm_open(struct inode *inode, struct file *file)
 	return 0;
 
  out:
-	clear_bit(0, &chip->is_open);
+	atomic_set(&chip->is_open, 0);
 	return -ENOMEM;
 }
 
@@ -49,9 +49,10 @@ static int tpm_open(struct inode *inode, struct file *file)
 static int tpm_release(struct inode *inode, struct file *file)
 {
 	struct file_priv *priv = file->private_data;
+	struct tpm_chip *chip = priv->chip;
 
 	tpm_common_release(file, priv);
-	clear_bit(0, &priv->chip->is_open);
+	atomic_set(&chip->is_open, 0);
 	kfree(priv);
 
 	return 0;
diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index eef0fb06ea83..ec83ca8105b8 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -27,6 +27,8 @@ static int tpmrm_open(struct inode *inode, struct file *file)
 		return -ENOMEM;
 	}
 
+	atomic_inc(&chip->is_open);
+
 	tpm_common_open(file, chip, &priv->priv, &priv->space);
 
 	return 0;
@@ -39,6 +41,11 @@ static int tpmrm_release(struct inode *inode, struct file *file)
 
 	tpm_common_release(file, fpriv);
 	tpm2_del_space(fpriv->chip, &priv->space);
+
+	if (!atomic_dec_return(&fpriv->chip->is_open)) {
+		tpm2_shutdown(fpriv->chip, TPM2_SU_CLEAR);
+		tpm_chip_stop(fpriv->chip);
+	}
 	kfree(priv);
 
 	return 0;
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 77fdc988c610..26e070198a15 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -126,7 +126,7 @@ struct tpm_chip {
 	unsigned int flags;
 
 	int dev_num;		/* /dev/tpm# */
-	unsigned long is_open;	/* only one allowed */
+	atomic_t is_open;	/* only one allowed */
 
 	char hwrng_name[64];
 	struct hwrng hwrng;
-- 
2.25.1

Re: [PATCH][RFC] tpm: Rework open/close/shutdown to avoid races

[Jason Gunthorpe]

On Fri, Dec 04, 2020 at 01:18:05PM +0300, Sergey Temerkhanov wrote:
> Avoid race condition at shutdown by shutting downn the TPM 2.0
> devices synchronously. This eliminates the condition when the
> shutdown sequence sets chip->ops to NULL leading to the following:
> 
> [ 1586.593561][ T8669] tpm2_del_space+0x28/0x73
> [ 1586.598718][ T8669] tpmrm_release+0x27/0x33wq
> [ 1586.603774][ T8669] __fput+0x109/0x1d
> [ 1586.608380][ T8669] task_work_run+0x7c/0x90
> [ 1586.613414][ T8669] prepare_exit_to_usermode+0xb8/0x128
> [ 1586.619522][ T8669] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 1586.626068][ T8669] RIP: 0033:0x4cb4bb
> 
> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>
>  drivers/char/tpm/tpm-chip.c  | 27 ---------------------------
>  drivers/char/tpm/tpm-dev.c   | 11 ++++++-----
>  drivers/char/tpm/tpmrm-dev.c |  7 +++++++
>  include/linux/tpm.h          |  2 +-
>  4 files changed, 14 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 1838039b0333..ede7f4790c5e 100644
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -282,32 +282,6 @@ static void tpm_devs_release(struct device *dev)
>  	put_device(&chip->dev);
>  }
>  
> -/**
> - * tpm_class_shutdown() - prepare the TPM device for loss of power.
> - * @dev: device to which the chip is associated.
> - *
> - * Issues a TPM2_Shutdown command prior to loss of power, as required by the
> - * TPM 2.0 spec. Then, calls bus- and device- specific shutdown code.
> - *
> - * Return: always 0 (i.e. success)
> - */
> -static int tpm_class_shutdown(struct device *dev)
> -{
> -	struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev);
> -
> -	down_write(&chip->ops_sem);
> -	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> -		if (!tpm_chip_start(chip)) {
> -			tpm2_shutdown(chip, TPM2_SU_CLEAR);
> -			tpm_chip_stop(chip);
> -		}
> -	}
> -	chip->ops = NULL;
> -	up_write(&chip->ops_sem);
> -
> -	return 0;
> -}

This does more than just call tpm2_shutdown, it exists to prevent use
after free situations because the chip point can exist in other parts
of the system beyond the lifetime of the driver.

You can't call into the driver at all past shutdown, so moving
tpm2_shutdown here:

> @@ -39,6 +41,11 @@ static int tpmrm_release(struct inode *inode, struct file *file)
>  
>  	tpm_common_release(file, fpriv);
>  	tpm2_del_space(fpriv->chip, &priv->space);
> +
> +	if (!atomic_dec_return(&fpriv->chip->is_open)) {
> +		tpm2_shutdown(fpriv->chip, TPM2_SU_CLEAR);
> +		tpm_chip_stop(fpriv->chip);
> +	}

Is just wrong.

Your bug is because tpmrm_release is not following the lifetime rules
for chip, it probably needs to do a get on the pointer to be able to
access the ops.

Jason

Re: [PATCH][RFC] tpm: Rework open/close/shutdown to avoid races

[Jarkko Sakkinen]

On Fri, Dec 04, 2020 at 01:18:05PM +0300, Sergey Temerkhanov wrote:
> Avoid race condition at shutdown by shutting downn the TPM 2.0
> devices synchronously. This eliminates the condition when the
> shutdown sequence sets chip->ops to NULL leading to the following:
> 
> [ 1586.593561][ T8669] tpm2_del_space+0x28/0x73
> [ 1586.598718][ T8669] tpmrm_release+0x27/0x33wq
> [ 1586.603774][ T8669] __fput+0x109/0x1d
> [ 1586.608380][ T8669] task_work_run+0x7c/0x90
> [ 1586.613414][ T8669] prepare_exit_to_usermode+0xb8/0x128
> [ 1586.619522][ T8669] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 1586.626068][ T8669] RIP: 0033:0x4cb4bb
> 
> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@gmail.com>

bitops is an atomic API. I don't understand why you want to convert
to "atomic_t". You are also removing tpm_class_shutdown() without
any explanation. Finally, there is no bug report.

/Jarkko


> ---
>  drivers/char/tpm/tpm-chip.c  | 27 ---------------------------
>  drivers/char/tpm/tpm-dev.c   | 11 ++++++-----
>  drivers/char/tpm/tpmrm-dev.c |  7 +++++++
>  include/linux/tpm.h          |  2 +-
>  4 files changed, 14 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 1838039b0333..ede7f4790c5e 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -282,32 +282,6 @@ static void tpm_devs_release(struct device *dev)
>  	put_device(&chip->dev);
>  }
>  
> -/**
> - * tpm_class_shutdown() - prepare the TPM device for loss of power.
> - * @dev: device to which the chip is associated.
> - *
> - * Issues a TPM2_Shutdown command prior to loss of power, as required by the
> - * TPM 2.0 spec. Then, calls bus- and device- specific shutdown code.
> - *
> - * Return: always 0 (i.e. success)
> - */
> -static int tpm_class_shutdown(struct device *dev)
> -{
> -	struct tpm_chip *chip = container_of(dev, struct tpm_chip, dev);
> -
> -	down_write(&chip->ops_sem);
> -	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> -		if (!tpm_chip_start(chip)) {
> -			tpm2_shutdown(chip, TPM2_SU_CLEAR);
> -			tpm_chip_stop(chip);
> -		}
> -	}
> -	chip->ops = NULL;
> -	up_write(&chip->ops_sem);
> -
> -	return 0;
> -}
> -
>  /**
>   * tpm_chip_alloc() - allocate a new struct tpm_chip instance
>   * @pdev: device to which the chip is associated
> @@ -347,7 +321,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
>  	device_initialize(&chip->devs);
>  
>  	chip->dev.class = tpm_class;
> -	chip->dev.class->shutdown_pre = tpm_class_shutdown;
>  	chip->dev.release = tpm_dev_release;
>  	chip->dev.parent = pdev;
>  	chip->dev.groups = chip->groups;
> diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
> index e2c0baa69fef..e04f3d47c64e 100644
> --- a/drivers/char/tpm/tpm-dev.c
> +++ b/drivers/char/tpm/tpm-dev.c
> @@ -23,9 +23,9 @@ static int tpm_open(struct inode *inode, struct file *file)
>  	chip = container_of(inode->i_cdev, struct tpm_chip, cdev);
>  
>  	/* It's assured that the chip will be opened just once,
> -	 * by the check of is_open variable, which is protected
> -	 * by driver_lock. */
> -	if (test_and_set_bit(0, &chip->is_open)) {
> +	 * by the check of is_open variable
> +	 */
> +	if (atomic_fetch_or(1, &chip->is_open)) {
>  		dev_dbg(&chip->dev, "Another process owns this TPM\n");
>  		return -EBUSY;
>  	}
> @@ -39,7 +39,7 @@ static int tpm_open(struct inode *inode, struct file *file)
>  	return 0;
>  
>   out:
> -	clear_bit(0, &chip->is_open);
> +	atomic_set(&chip->is_open, 0);
>  	return -ENOMEM;
>  }
>  
> @@ -49,9 +49,10 @@ static int tpm_open(struct inode *inode, struct file *file)
>  static int tpm_release(struct inode *inode, struct file *file)
>  {
>  	struct file_priv *priv = file->private_data;
> +	struct tpm_chip *chip = priv->chip;
>  
>  	tpm_common_release(file, priv);
> -	clear_bit(0, &priv->chip->is_open);
> +	atomic_set(&chip->is_open, 0);
>  	kfree(priv);
>  
>  	return 0;
> diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
> index eef0fb06ea83..ec83ca8105b8 100644
> --- a/drivers/char/tpm/tpmrm-dev.c
> +++ b/drivers/char/tpm/tpmrm-dev.c
> @@ -27,6 +27,8 @@ static int tpmrm_open(struct inode *inode, struct file *file)
>  		return -ENOMEM;
>  	}
>  
> +	atomic_inc(&chip->is_open);
> +
>  	tpm_common_open(file, chip, &priv->priv, &priv->space);
>  
>  	return 0;
> @@ -39,6 +41,11 @@ static int tpmrm_release(struct inode *inode, struct file *file)
>  
>  	tpm_common_release(file, fpriv);
>  	tpm2_del_space(fpriv->chip, &priv->space);
> +
> +	if (!atomic_dec_return(&fpriv->chip->is_open)) {
> +		tpm2_shutdown(fpriv->chip, TPM2_SU_CLEAR);
> +		tpm_chip_stop(fpriv->chip);
> +	}
>  	kfree(priv);
>  
>  	return 0;
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 77fdc988c610..26e070198a15 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -126,7 +126,7 @@ struct tpm_chip {
>  	unsigned int flags;
>  
>  	int dev_num;		/* /dev/tpm# */
> -	unsigned long is_open;	/* only one allowed */
> +	atomic_t is_open;	/* only one allowed */
>  
>  	char hwrng_name[64];
>  	struct hwrng hwrng;
> -- 
> 2.25.1
> 

/Jarkko