* [PATCH][RFC] tpm: Rework open/close/shutdown to avoid races
@ 2020-12-04 10:18 Sergey Temerkhanov
2020-12-04 12:58 ` Jason Gunthorpe
2020-12-08 11:00 ` Jarkko Sakkinen
0 siblings, 2 replies; 3+ messages in thread
From: Sergey Temerkhanov @ 2020-12-04 10:18 UTC (permalink / raw)
To: Peter Huewe, Jarkko Sakkinen, Jason Gunthorpe, Arnd Bergmann,
Greg Kroah-Hartman, Sergey Temerkhanov, Jerry Snitselaar,
linux-integrity, linux-kernel
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH][RFC] tpm: Rework open/close/shutdown to avoid races
2020-12-04 10:18 [PATCH][RFC] tpm: Rework open/close/shutdown to avoid races Sergey Temerkhanov
@ 2020-12-04 12:58 ` Jason Gunthorpe
2020-12-08 11:00 ` Jarkko Sakkinen
1 sibling, 0 replies; 3+ messages in thread
From: Jason Gunthorpe @ 2020-12-04 12:58 UTC (permalink / raw)
To: Sergey Temerkhanov
Cc: Peter Huewe, Jarkko Sakkinen, Arnd Bergmann, Greg Kroah-Hartman,
Jerry Snitselaar, linux-integrity, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH][RFC] tpm: Rework open/close/shutdown to avoid races
2020-12-04 10:18 [PATCH][RFC] tpm: Rework open/close/shutdown to avoid races Sergey Temerkhanov
2020-12-04 12:58 ` Jason Gunthorpe
@ 2020-12-08 11:00 ` Jarkko Sakkinen
1 sibling, 0 replies; 3+ messages in thread
From: Jarkko Sakkinen @ 2020-12-08 11:00 UTC (permalink / raw)
To: Sergey Temerkhanov
Cc: Peter Huewe, Jason Gunthorpe, Arnd Bergmann, Greg Kroah-Hartman,
Jerry Snitselaar, linux-integrity, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-12-08 11:02 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-04 10:18 [PATCH][RFC] tpm: Rework open/close/shutdown to avoid races Sergey Temerkhanov
2020-12-04 12:58 ` Jason Gunthorpe
2020-12-08 11:00 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).