[RFC PATCH 0/4] Re-establish ability for exclusive TPM access to userspace

[RFC PATCH 0/4] Re-establish ability for exclusive TPM access to userspace

[Jonathan McDowell]

I hit a problem last week were ~ 1% of TPM firmware upgrades were
failing. Investigating revealed the issue was that although the upgrade
tool uses /dev/tpm0 this does not actually prevent access via
/dev/tpmrm0, nor internal kernel users. It *does* prevent access to
others via /dev/tpm0

So the upgrade process started, the HW RNG came in to get some
randomness in the middle, did the HMAC context dance, and confused
everything to the point the TPM was no longer visible to the OS even
after a reboot.

Thankfully I've been able to recover those devices, but really what I'd
like is the ability for a userspace tool to exclusively access the TPM
without something coming in behind it. Given the lightweight attempt at
locking that already exists I think this was the original intention.

As an initial approach I propose this patch set; I don't think the first
2 patches are controversial, but the blocking of kernel access + switch
to O_EXCEL in patches 3 + 4 might be. I'm open to alternative
suggestions about how to achieve this.

(I've sent a separate standalone patch that allows the TPM HW RNG to be
disabled at run time, but even with that I think something like this is
a good idea as well.)

Jonathan McDowell (4):
  tpm: Ensure exclusive userspace access when using /dev/tpm<n>
  tpm: Remove tpm_find_get_ops
  tpm: Allow for exclusive TPM access when using /dev/tpm<n>
  tpm: Require O_EXCL for exclusive /dev/tpm access

 drivers/char/tpm/tpm-chip.c       | 90 +++++++++++++++----------------
 drivers/char/tpm/tpm-dev-common.c |  8 +--
 drivers/char/tpm/tpm-dev.c        | 27 +++++++---
 drivers/char/tpm/tpm-dev.h        |  1 +
 drivers/char/tpm/tpm-interface.c  | 20 +++++--
 drivers/char/tpm/tpm.h            |  3 +-
 drivers/char/tpm/tpm2-space.c     |  5 +-
 drivers/char/tpm/tpm_tis_core.c   |  3 +-
 drivers/char/tpm/tpmrm-dev.c      | 20 ++++++-
 include/linux/tpm.h               |  3 +-
 10 files changed, 112 insertions(+), 68 deletions(-)

-- 
2.51.0

[RFC PATCH 1/4] tpm: Ensure exclusive userspace access when using /dev/tpm<n>

[Jonathan McDowell]

From: Jonathan McDowell <noodles@meta.com>

There is an is_open lock on /dev/tpm<n> that dates back to at least
2013, but it only prevents multiple accesses via *this* interface. It is
perfectly possible for userspace to use /dev/tpmrm<n>, or the kernel to
use the internal interfaces, to access the TPM.

This can cause problems with userspace expecting exclusive access and
something changing state underneath it, for example while performing a
TPM firmware upgrade.

Close the userspace loophole by changing the simple bit lock to a full
read/write mutex. Direct /dev/tpm<n> access needs an exclusive write
lock, the resource broker continues to allow concurrent access *except*
when /dev/tpm<n> is open.

Signed-off-by: Jonathan McDowell <noodles@meta.com>
---
 drivers/char/tpm/tpm-chip.c  |  1 +
 drivers/char/tpm/tpm-dev.c   | 14 ++++++++------
 drivers/char/tpm/tpmrm-dev.c | 20 ++++++++++++++++++--
 include/linux/tpm.h          |  3 ++-
 4 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index e25daf2396d3..8c8e9054762a 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -338,6 +338,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 
 	mutex_init(&chip->tpm_mutex);
 	init_rwsem(&chip->ops_sem);
+	init_rwsem(&chip->open_lock);
 
 	chip->ops = ops;
 
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 97c94b5e9340..80c4b3f3ad18 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -22,10 +22,12 @@ 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)) {
+	/*
+	 * Only one client is allowed to have /dev/tpm0 open at a time, so we
+	 * treat it as a write lock. The shared /dev/tpmrm0 is treated as a
+	 * read lock.
+	 */
+	if (!down_write_trylock(&chip->open_lock)) {
 		dev_dbg(&chip->dev, "Another process owns this TPM\n");
 		return -EBUSY;
 	}
@@ -39,7 +41,7 @@ static int tpm_open(struct inode *inode, struct file *file)
 	return 0;
 
  out:
-	clear_bit(0, &chip->is_open);
+	up_write(&chip->open_lock);
 	return -ENOMEM;
 }
 
@@ -51,7 +53,7 @@ static int tpm_release(struct inode *inode, struct file *file)
 	struct file_priv *priv = file->private_data;
 
 	tpm_common_release(file, priv);
-	clear_bit(0, &priv->chip->is_open);
+	up_write(&priv->chip->open_lock);
 	kfree(priv);
 
 	return 0;
diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index c25df7ea064e..40c139a080b6 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -17,19 +17,34 @@ static int tpmrm_open(struct inode *inode, struct file *file)
 	int rc;
 
 	chip = container_of(inode->i_cdev, struct tpm_chip, cdevs);
+
+	/*
+	 * Only one client is allowed to have /dev/tpm0 open at a time, so we
+	 * treat it as a write lock. The shared /dev/tpmrm0 is treated as a
+	 * read lock.
+	 */
+	if (!down_read_trylock(&chip->open_lock)) {
+		dev_dbg(&chip->dev, "Another process owns this TPM\n");
+		return -EBUSY;
+	}
+
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (priv == NULL)
-		return -ENOMEM;
+		goto out;
 
 	rc = tpm2_init_space(&priv->space, TPM2_SPACE_BUFFER_SIZE);
 	if (rc) {
 		kfree(priv);
-		return -ENOMEM;
+		goto out;
 	}
 
 	tpm_common_open(file, chip, &priv->priv, &priv->space);
 
 	return 0;
+
+out:
+	up_read(&chip->open_lock);
+	return -ENOMEM;
 }
 
 static int tpmrm_release(struct inode *inode, struct file *file)
@@ -40,6 +55,7 @@ static int tpmrm_release(struct inode *inode, struct file *file)
 	tpm_common_release(file, fpriv);
 	tpm2_del_space(fpriv->chip, &priv->space);
 	kfree(priv);
+	up_read(&fpriv->chip->open_lock);
 
 	return 0;
 }
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index b0e9eb5ef022..548362d20b32 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -22,6 +22,7 @@
 #include <linux/cdev.h>
 #include <linux/fs.h>
 #include <linux/highmem.h>
+#include <linux/rwsem.h>
 #include <crypto/hash_info.h>
 #include <crypto/aes.h>
 
@@ -168,7 +169,7 @@ struct tpm_chip {
 	unsigned int flags;
 
 	int dev_num;		/* /dev/tpm# */
-	unsigned long is_open;	/* only one allowed */
+	struct rw_semaphore open_lock;
 
 	char hwrng_name[64];
 	struct hwrng hwrng;
-- 
2.51.0

[RFC PATCH 2/4] tpm: Remove tpm_find_get_ops

[Jonathan McDowell]

From: Jonathan McDowell <noodles@meta.com>

tpm_find_get_ops() looks for the first valid TPM if the caller passes in
NULL. All internal users have been converted to either associate
themselves with a TPM directly, or call tpm_default_chip() as part of
their setup. Remove the no longer necessary tpm_find_get_ops().

Signed-off-by: Jonathan McDowell <noodles@meta.com>
---
 drivers/char/tpm/tpm-chip.c      | 36 --------------------------------
 drivers/char/tpm/tpm-interface.c | 20 ++++++++++++++----
 drivers/char/tpm/tpm.h           |  1 -
 drivers/char/tpm/tpm_tis_core.c  |  3 +--
 4 files changed, 17 insertions(+), 43 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 8c8e9054762a..ba906966721a 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -230,42 +230,6 @@ struct tpm_chip *tpm_default_chip(void)
 }
 EXPORT_SYMBOL_GPL(tpm_default_chip);
 
-/**
- * tpm_find_get_ops() - find and reserve a TPM chip
- * @chip:	a &struct tpm_chip instance, %NULL for the default chip
- *
- * Finds a TPM chip and reserves its class device and operations. The chip must
- * be released with tpm_put_ops() after use.
- * This function is for internal use only. It supports existing TPM callers
- * by accepting NULL, but those callers should be converted to pass in a chip
- * directly.
- *
- * Return:
- * A reserved &struct tpm_chip instance.
- * %NULL if a chip is not found.
- * %NULL if the chip is not available.
- */
-struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip)
-{
-	int rc;
-
-	if (chip) {
-		if (!tpm_try_get_ops(chip))
-			return chip;
-		return NULL;
-	}
-
-	chip = tpm_default_chip();
-	if (!chip)
-		return NULL;
-	rc = tpm_try_get_ops(chip);
-	/* release additional reference we got from tpm_default_chip() */
-	put_device(&chip->dev);
-	if (rc)
-		return NULL;
-	return chip;
-}
-
 /**
  * tpm_dev_release() - free chip memory and the device number
  * @dev: the character device for the TPM chip
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index b71725827743..8f65dc06a157 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -313,10 +313,13 @@ int tpm_is_tpm2(struct tpm_chip *chip)
 {
 	int rc;
 
-	chip = tpm_find_get_ops(chip);
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
 	rc = (chip->flags & TPM_CHIP_FLAG_TPM2) != 0;
 
 	tpm_put_ops(chip);
@@ -338,10 +341,13 @@ int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 {
 	int rc;
 
-	chip = tpm_find_get_ops(chip);
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		rc = tpm2_pcr_read(chip, pcr_idx, digest, NULL);
 	else
@@ -369,10 +375,13 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 	int rc;
 	int i;
 
-	chip = tpm_find_get_ops(chip);
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
 	for (i = 0; i < chip->nr_allocated_banks; i++) {
 		if (digests[i].alg_id != chip->allocated_banks[i].alg_id) {
 			rc = -EINVAL;
@@ -492,10 +501,13 @@ int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
 	if (!out || max > TPM_MAX_RNG_DATA)
 		return -EINVAL;
 
-	chip = tpm_find_get_ops(chip);
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		rc = tpm2_get_random(chip, out, max);
 	else
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 7bb87fa5f7a1..9c158c55c05f 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -267,7 +267,6 @@ static inline void tpm_msleep(unsigned int delay_msec)
 int tpm_chip_bootstrap(struct tpm_chip *chip);
 int tpm_chip_start(struct tpm_chip *chip);
 void tpm_chip_stop(struct tpm_chip *chip);
-struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
 
 struct tpm_chip *tpm_chip_alloc(struct device *dev,
 				const struct tpm_class_ops *ops);
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 4b12c4b9da8b..73b94f4daf4b 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -265,8 +265,7 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
 
 			/*
 			 * Dump stack for forensics, as invalid TPM_STS.x could be
-			 * potentially triggered by impaired tpm_try_get_ops() or
-			 * tpm_find_get_ops().
+			 * potentially triggered by impaired tpm_try_get_ops().
 			 */
 			dump_stack();
 		}
-- 
2.51.0

[RFC PATCH 3/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>

[Jonathan McDowell]

From: Jonathan McDowell <noodles@meta.com>

There are situations where userspace might reasonably desire exclusive
access to the TPM, or the kernel's internal context saving + flushing
may cause issues, for example when performing firmware upgrades. Extend
the locking already used for avoiding concurrent userspace access to
prevent internal users of the TPM when /dev/tpm<n> is in use.

The few internal users who already hold the open_lock are changed to use
tpm_internal_(try_get|put)_ops, with the old tpm_(try_get|put)_ops
functions changing to obtain read access to the open_lock.  We return
-EBUSY when another user has exclusive access, rather than adding waits.

Signed-off-by: Jonathan McDowell <noodles@meta.com>
---
 drivers/char/tpm/tpm-chip.c       | 53 +++++++++++++++++++++++++------
 drivers/char/tpm/tpm-dev-common.c |  8 ++---
 drivers/char/tpm/tpm.h            |  2 ++
 drivers/char/tpm/tpm2-space.c     |  5 ++-
 4 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index ba906966721a..3d69ccff4c2a 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -144,7 +144,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
 EXPORT_SYMBOL_GPL(tpm_chip_stop);
 
 /**
- * tpm_try_get_ops() - Get a ref to the tpm_chip
+ * tpm_internal_try_get_ops() - Get a ref to the tpm_chip
  * @chip: Chip to ref
  *
  * The caller must already have some kind of locking to ensure that chip is
@@ -154,7 +154,7 @@ EXPORT_SYMBOL_GPL(tpm_chip_stop);
  *
  * Returns -ERRNO if the chip could not be got.
  */
-int tpm_try_get_ops(struct tpm_chip *chip)
+int tpm_internal_try_get_ops(struct tpm_chip *chip)
 {
 	int rc = -EIO;
 
@@ -185,22 +185,57 @@ int tpm_try_get_ops(struct tpm_chip *chip)
 	put_device(&chip->dev);
 	return rc;
 }
-EXPORT_SYMBOL_GPL(tpm_try_get_ops);
 
 /**
- * tpm_put_ops() - Release a ref to the tpm_chip
+ * tpm_internal_put_ops() - Release a ref to the tpm_chip
  * @chip: Chip to put
  *
- * This is the opposite pair to tpm_try_get_ops(). After this returns chip may
- * be kfree'd.
+ * This is the opposite pair to tpm_internal_try_get_ops(). After this returns
+ * chip may be kfree'd.
  */
-void tpm_put_ops(struct tpm_chip *chip)
+void tpm_internal_put_ops(struct tpm_chip *chip)
 {
 	tpm_chip_stop(chip);
 	mutex_unlock(&chip->tpm_mutex);
 	up_read(&chip->ops_sem);
 	put_device(&chip->dev);
 }
+
+/**
+ * tpm_try_get_ops() - Get a ref to the tpm_chip
+ * @chip: Chip to ref
+ *
+ * The caller must already have some kind of locking to ensure that chip is
+ * valid. This function will attempt to get the open_lock for the chip,
+ * ensuring no other user is expecting exclusive access, before locking the
+ * chip so that the ops member can be accessed safely. The locking prevents
+ * tpm_chip_unregister from completing, so it should not be held for long
+ * periods.
+ *
+ * Returns -ERRNO if the chip could not be got.
+ */
+int tpm_try_get_ops(struct tpm_chip *chip)
+{
+	if (!down_read_trylock(&chip->open_lock))
+		return -EBUSY;
+
+	return tpm_internal_try_get_ops(chip);
+}
+EXPORT_SYMBOL_GPL(tpm_try_get_ops);
+
+/**
+ * tpm_put_ops() - Release a ref to the tpm_chip
+ * @chip: Chip to put
+ *
+ * This is the opposite pair to tpm_try_get_ops(). After this returns
+ * chip may be kfree'd.
+ */
+void tpm_put_ops(struct tpm_chip *chip)
+{
+	tpm_internal_put_ops(chip);
+
+	up_read(&chip->open_lock);
+}
 EXPORT_SYMBOL_GPL(tpm_put_ops);
 
 /**
@@ -644,10 +679,10 @@ void tpm_chip_unregister(struct tpm_chip *chip)
 #ifdef CONFIG_TCG_TPM2_HMAC
 	int rc;
 
-	rc = tpm_try_get_ops(chip);
+	rc = tpm_internal_try_get_ops(chip);
 	if (!rc) {
 		tpm2_end_auth_session(chip);
-		tpm_put_ops(chip);
+		tpm_internal_put_ops(chip);
 	}
 #endif
 
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index f2a5e09257dd..7cd0617844ed 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -65,7 +65,7 @@ static void tpm_dev_async_work(struct work_struct *work)
 
 	mutex_lock(&priv->buffer_mutex);
 	priv->command_enqueued = false;
-	ret = tpm_try_get_ops(priv->chip);
+	ret = tpm_internal_try_get_ops(priv->chip);
 	if (ret) {
 		priv->response_length = ret;
 		goto out;
@@ -73,7 +73,7 @@ static void tpm_dev_async_work(struct work_struct *work)
 
 	ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
 			       sizeof(priv->data_buffer));
-	tpm_put_ops(priv->chip);
+	tpm_internal_put_ops(priv->chip);
 
 	/*
 	 * If ret is > 0 then tpm_dev_transmit returned the size of the
@@ -220,14 +220,14 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
 	 * lock during this period so that the tpm can be unregistered even if
 	 * the char dev is held open.
 	 */
-	if (tpm_try_get_ops(priv->chip)) {
+	if (tpm_internal_try_get_ops(priv->chip)) {
 		ret = -EPIPE;
 		goto out;
 	}
 
 	ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
 			       sizeof(priv->data_buffer));
-	tpm_put_ops(priv->chip);
+	tpm_internal_put_ops(priv->chip);
 
 	if (ret > 0) {
 		priv->response_length = ret;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 9c158c55c05f..1c34c10421c9 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -272,6 +272,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
 				const struct tpm_class_ops *ops);
 struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
 				 const struct tpm_class_ops *ops);
+int tpm_internal_try_get_ops(struct tpm_chip *chip);
+void tpm_internal_put_ops(struct tpm_chip *chip);
 int tpm_chip_register(struct tpm_chip *chip);
 void tpm_chip_unregister(struct tpm_chip *chip);
 
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 60354cd53b5c..853acaf3d10f 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -58,10 +58,9 @@ int tpm2_init_space(struct tpm_space *space, unsigned int buf_size)
 
 void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
 {
-
-	if (tpm_try_get_ops(chip) == 0) {
+	if (tpm_internal_try_get_ops(chip) == 0) {
 		tpm2_flush_sessions(chip, space);
-		tpm_put_ops(chip);
+		tpm_internal_put_ops(chip);
 	}
 
 	kfree(space->context_buf);
-- 
2.51.0

[RFC PATCH 4/4] tpm: Require O_EXCL for exclusive /dev/tpm access

[Jonathan McDowell]

From: Jonathan McDowell <noodles@meta.com>

Given that /dev/tpm has not had exclusive access to the TPM since the
existence of the kernel resource broker and other internal users, stop
defaulted to exclusive access to the first client that opens the device.
Continue to support exclusive access, but only with the use of the
O_EXCL flag on device open.

Signed-off-by: Jonathan McDowell <noodles@meta.com>
---
 drivers/char/tpm/tpm-dev.c | 25 +++++++++++++++++++------
 drivers/char/tpm/tpm-dev.h |  1 +
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 80c4b3f3ad18..8921bbb541c1 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -19,15 +19,21 @@ static int tpm_open(struct inode *inode, struct file *file)
 {
 	struct tpm_chip *chip;
 	struct file_priv *priv;
+	int rc;
 
 	chip = container_of(inode->i_cdev, struct tpm_chip, cdev);
 
 	/*
-	 * Only one client is allowed to have /dev/tpm0 open at a time, so we
-	 * treat it as a write lock. The shared /dev/tpmrm0 is treated as a
-	 * read lock.
+	 * If a client uses the O_EXCL flag then it expects to be the only TPM
+	 * user, so we treat it as a write lock. Otherwise we do as /dev/tpmrm
+	 * and use a read lock.
 	 */
-	if (!down_write_trylock(&chip->open_lock)) {
+	if (file->f_flags & O_EXCL)
+		rc = down_write_trylock(&chip->open_lock);
+	else
+		rc = down_read_trylock(&chip->open_lock);
+
+	if (!rc) {
 		dev_dbg(&chip->dev, "Another process owns this TPM\n");
 		return -EBUSY;
 	}
@@ -35,13 +41,17 @@ static int tpm_open(struct inode *inode, struct file *file)
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (priv == NULL)
 		goto out;
+	priv->exclusive = (file->f_flags & O_EXCL);
 
 	tpm_common_open(file, chip, priv, NULL);
 
 	return 0;
 
  out:
-	up_write(&chip->open_lock);
+	if (file->f_flags & O_EXCL)
+		up_write(&chip->open_lock);
+	else
+		up_read(&chip->open_lock);
 	return -ENOMEM;
 }
 
@@ -53,7 +63,10 @@ static int tpm_release(struct inode *inode, struct file *file)
 	struct file_priv *priv = file->private_data;
 
 	tpm_common_release(file, priv);
-	up_write(&priv->chip->open_lock);
+	if (priv->exclusive)
+		up_write(&priv->chip->open_lock);
+	else
+		up_read(&priv->chip->open_lock);
 	kfree(priv);
 
 	return 0;
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index f3742bcc73e3..0ad8504c73e4 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -17,6 +17,7 @@ struct file_priv {
 	ssize_t response_length;
 	bool response_read;
 	bool command_enqueued;
+	bool exclusive;
 
 	u8 data_buffer[TPM_BUFSIZE];
 };
-- 
2.51.0

Re: [RFC PATCH 1/4] tpm: Ensure exclusive userspace access when using /dev/tpm<n>

[Jarkko Sakkinen]

On Tue, Sep 02, 2025 at 06:26:49PM +0100, Jonathan McDowell wrote:
> From: Jonathan McDowell <noodles@meta.com>
> 
> There is an is_open lock on /dev/tpm<n> that dates back to at least
> 2013, but it only prevents multiple accesses via *this* interface. It is
> perfectly possible for userspace to use /dev/tpmrm<n>, or the kernel to
> use the internal interfaces, to access the TPM.
> 
> This can cause problems with userspace expecting exclusive access and
> something changing state underneath it, for example while performing a
> TPM firmware upgrade.
> 
> Close the userspace loophole by changing the simple bit lock to a full
> read/write mutex. Direct /dev/tpm<n> access needs an exclusive write
> lock, the resource broker continues to allow concurrent access *except*
> when /dev/tpm<n> is open.
> 
> Signed-off-by: Jonathan McDowell <noodles@meta.com>

I think the rationale makes sense to me as they are different view port
for the exact same hardware instance, and /dev/tpmrm0 scales only within
its own virtual universum.

I don't know what would be the best write up but basically I'd cut the
story shorter a bit and explicitly enumerate these anchoring "hard
reasons". Problems in user space is something that I can imagine that
there is a variety problem but it is more abstract side of this
issue. When you have a smoking gun just point your finger to it
exactly.


> ---
>  drivers/char/tpm/tpm-chip.c  |  1 +
>  drivers/char/tpm/tpm-dev.c   | 14 ++++++++------
>  drivers/char/tpm/tpmrm-dev.c | 20 ++++++++++++++++++--
>  include/linux/tpm.h          |  3 ++-
>  4 files changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index e25daf2396d3..8c8e9054762a 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -338,6 +338,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
>  
>  	mutex_init(&chip->tpm_mutex);
>  	init_rwsem(&chip->ops_sem);
> +	init_rwsem(&chip->open_lock);
>  
>  	chip->ops = ops;
>  
> diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
> index 97c94b5e9340..80c4b3f3ad18 100644
> --- a/drivers/char/tpm/tpm-dev.c
> +++ b/drivers/char/tpm/tpm-dev.c
> @@ -22,10 +22,12 @@ 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)) {
> +	/*
> +	 * Only one client is allowed to have /dev/tpm0 open at a time, so we
> +	 * treat it as a write lock. The shared /dev/tpmrm0 is treated as a
> +	 * read lock.
> +	 */
> +	if (!down_write_trylock(&chip->open_lock)) {
>  		dev_dbg(&chip->dev, "Another process owns this TPM\n");
>  		return -EBUSY;
>  	}
> @@ -39,7 +41,7 @@ static int tpm_open(struct inode *inode, struct file *file)
>  	return 0;
>  
>   out:
> -	clear_bit(0, &chip->is_open);
> +	up_write(&chip->open_lock);
>  	return -ENOMEM;
>  }
>  
> @@ -51,7 +53,7 @@ static int tpm_release(struct inode *inode, struct file *file)
>  	struct file_priv *priv = file->private_data;
>  
>  	tpm_common_release(file, priv);
> -	clear_bit(0, &priv->chip->is_open);
> +	up_write(&priv->chip->open_lock);
>  	kfree(priv);
>  
>  	return 0;
> diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
> index c25df7ea064e..40c139a080b6 100644
> --- a/drivers/char/tpm/tpmrm-dev.c
> +++ b/drivers/char/tpm/tpmrm-dev.c
> @@ -17,19 +17,34 @@ static int tpmrm_open(struct inode *inode, struct file *file)
>  	int rc;
>  
>  	chip = container_of(inode->i_cdev, struct tpm_chip, cdevs);
> +
> +	/*
> +	 * Only one client is allowed to have /dev/tpm0 open at a time, so we
> +	 * treat it as a write lock. The shared /dev/tpmrm0 is treated as a
> +	 * read lock.
> +	 */
> +	if (!down_read_trylock(&chip->open_lock)) {
> +		dev_dbg(&chip->dev, "Another process owns this TPM\n");
> +		return -EBUSY;
> +	}
> +
>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>  	if (priv == NULL)
> -		return -ENOMEM;
> +		goto out;
>  
>  	rc = tpm2_init_space(&priv->space, TPM2_SPACE_BUFFER_SIZE);
>  	if (rc) {
>  		kfree(priv);
> -		return -ENOMEM;
> +		goto out;
>  	}
>  
>  	tpm_common_open(file, chip, &priv->priv, &priv->space);
>  
>  	return 0;
> +
> +out:

nit 

err:

as it is purely for error propagation


> +	up_read(&chip->open_lock);
> +	return -ENOMEM;
>  }
>  
>  static int tpmrm_release(struct inode *inode, struct file *file)
> @@ -40,6 +55,7 @@ static int tpmrm_release(struct inode *inode, struct file *file)
>  	tpm_common_release(file, fpriv);
>  	tpm2_del_space(fpriv->chip, &priv->space);
>  	kfree(priv);
> +	up_read(&fpriv->chip->open_lock);
>  
>  	return 0;
>  }
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index b0e9eb5ef022..548362d20b32 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -22,6 +22,7 @@
>  #include <linux/cdev.h>
>  #include <linux/fs.h>
>  #include <linux/highmem.h>
> +#include <linux/rwsem.h>
>  #include <crypto/hash_info.h>
>  #include <crypto/aes.h>
>  
> @@ -168,7 +169,7 @@ struct tpm_chip {
>  	unsigned int flags;
>  
>  	int dev_num;		/* /dev/tpm# */
> -	unsigned long is_open;	/* only one allowed */
> +	struct rw_semaphore open_lock;
>  
>  	char hwrng_name[64];
>  	struct hwrng hwrng;
> -- 
> 2.51.0
> 

BR, Jarkko

Re: [RFC PATCH 2/4] tpm: Remove tpm_find_get_ops

[Jarkko Sakkinen]

On Tue, Sep 02, 2025 at 06:27:03PM +0100, Jonathan McDowell wrote:
> From: Jonathan McDowell <noodles@meta.com>
> 
> tpm_find_get_ops() looks for the first valid TPM if the caller passes in
> NULL. All internal users have been converted to either associate
> themselves with a TPM directly, or call tpm_default_chip() as part of
> their setup. Remove the no longer necessary tpm_find_get_ops().
> 
> Signed-off-by: Jonathan McDowell <noodles@meta.com>

This is a welcome change, thanks.

> ---
>  drivers/char/tpm/tpm-chip.c      | 36 --------------------------------
>  drivers/char/tpm/tpm-interface.c | 20 ++++++++++++++----
>  drivers/char/tpm/tpm.h           |  1 -
>  drivers/char/tpm/tpm_tis_core.c  |  3 +--
>  4 files changed, 17 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 8c8e9054762a..ba906966721a 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -230,42 +230,6 @@ struct tpm_chip *tpm_default_chip(void)
>  }
>  EXPORT_SYMBOL_GPL(tpm_default_chip);
>  
> -/**
> - * tpm_find_get_ops() - find and reserve a TPM chip
> - * @chip:	a &struct tpm_chip instance, %NULL for the default chip
> - *
> - * Finds a TPM chip and reserves its class device and operations. The chip must
> - * be released with tpm_put_ops() after use.
> - * This function is for internal use only. It supports existing TPM callers
> - * by accepting NULL, but those callers should be converted to pass in a chip
> - * directly.
> - *
> - * Return:
> - * A reserved &struct tpm_chip instance.
> - * %NULL if a chip is not found.
> - * %NULL if the chip is not available.
> - */
> -struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip)
> -{
> -	int rc;
> -
> -	if (chip) {
> -		if (!tpm_try_get_ops(chip))
> -			return chip;
> -		return NULL;
> -	}
> -
> -	chip = tpm_default_chip();
> -	if (!chip)
> -		return NULL;
> -	rc = tpm_try_get_ops(chip);
> -	/* release additional reference we got from tpm_default_chip() */
> -	put_device(&chip->dev);
> -	if (rc)
> -		return NULL;
> -	return chip;
> -}
> -
>  /**
>   * tpm_dev_release() - free chip memory and the device number
>   * @dev: the character device for the TPM chip
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index b71725827743..8f65dc06a157 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -313,10 +313,13 @@ int tpm_is_tpm2(struct tpm_chip *chip)
>  {
>  	int rc;
>  
> -	chip = tpm_find_get_ops(chip);
>  	if (!chip)
>  		return -ENODEV;
>  
> +	rc = tpm_try_get_ops(chip);
> +	if (rc)
> +		return rc;
> +
>  	rc = (chip->flags & TPM_CHIP_FLAG_TPM2) != 0;
>  
>  	tpm_put_ops(chip);
> @@ -338,10 +341,13 @@ int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>  {
>  	int rc;
>  
> -	chip = tpm_find_get_ops(chip);
>  	if (!chip)
>  		return -ENODEV;
>  
> +	rc = tpm_try_get_ops(chip);
> +	if (rc)
> +		return rc;
> +
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		rc = tpm2_pcr_read(chip, pcr_idx, digest, NULL);
>  	else
> @@ -369,10 +375,13 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>  	int rc;
>  	int i;
>  
> -	chip = tpm_find_get_ops(chip);
>  	if (!chip)
>  		return -ENODEV;
>  
> +	rc = tpm_try_get_ops(chip);
> +	if (rc)
> +		return rc;
> +
>  	for (i = 0; i < chip->nr_allocated_banks; i++) {
>  		if (digests[i].alg_id != chip->allocated_banks[i].alg_id) {
>  			rc = -EINVAL;
> @@ -492,10 +501,13 @@ int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
>  	if (!out || max > TPM_MAX_RNG_DATA)
>  		return -EINVAL;
>  
> -	chip = tpm_find_get_ops(chip);
>  	if (!chip)
>  		return -ENODEV;
>  
> +	rc = tpm_try_get_ops(chip);
> +	if (rc)
> +		return rc;
> +
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		rc = tpm2_get_random(chip, out, max);
>  	else
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 7bb87fa5f7a1..9c158c55c05f 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -267,7 +267,6 @@ static inline void tpm_msleep(unsigned int delay_msec)
>  int tpm_chip_bootstrap(struct tpm_chip *chip);
>  int tpm_chip_start(struct tpm_chip *chip);
>  void tpm_chip_stop(struct tpm_chip *chip);
> -struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
>  
>  struct tpm_chip *tpm_chip_alloc(struct device *dev,
>  				const struct tpm_class_ops *ops);
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 4b12c4b9da8b..73b94f4daf4b 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -265,8 +265,7 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
>  
>  			/*
>  			 * Dump stack for forensics, as invalid TPM_STS.x could be
> -			 * potentially triggered by impaired tpm_try_get_ops() or
> -			 * tpm_find_get_ops().
> +			 * potentially triggered by impaired tpm_try_get_ops().
>  			 */
>  			dump_stack();

Sorry outside scope of the review but I'll remark something while I
still remember it :-)

Looking at "if  (!test_and_set_bit(TPM_TIS_INVALID_STATUS, &priv->flags)) {"

Despite unfortunately git blame points out to me I don't agree with the
"pr_err + dump_stack" rollback sequence:

1. Stack here is useless noise.
2. This should be fallible situation really, as at it can be affected by
   outside stimuli, not just long-strech malicious device alike case,
   but also it could be like perhaps TPM emulator or something else
   more flakky than a chip.

Improved rollback sequence would be:

1. Print dev_err, exactly as it does now. It's not a kernel bug per se
   but something is definitely acting weirdly.
2. Make '->status' fallible so that rollback can be further propagated
   to 'tpm_transmit'
3. Return some sensible POSIX error code, probably -EIO combined with
   pre-existing dev_err would be fine.

Back to the topic. I agree with the patch. I'll come back on this
once I've tested it with a live kernel in my environment [1].

[1] https://codeberg.org/jarkko/linux-tpmdd-test
    
BR, Jarkko

Re: [RFC PATCH 3/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>

[Jarkko Sakkinen]

On Tue, Sep 02, 2025 at 06:27:10PM +0100, Jonathan McDowell wrote:
> From: Jonathan McDowell <noodles@meta.com>
> 
> There are situations where userspace might reasonably desire exclusive
> access to the TPM, or the kernel's internal context saving + flushing
> may cause issues, for example when performing firmware upgrades. Extend
> the locking already used for avoiding concurrent userspace access to
> prevent internal users of the TPM when /dev/tpm<n> is in use.
> 
> The few internal users who already hold the open_lock are changed to use
> tpm_internal_(try_get|put)_ops, with the old tpm_(try_get|put)_ops
> functions changing to obtain read access to the open_lock.  We return
> -EBUSY when another user has exclusive access, rather than adding waits.
> 
> Signed-off-by: Jonathan McDowell <noodles@meta.com>
> ---
>  drivers/char/tpm/tpm-chip.c       | 53 +++++++++++++++++++++++++------
>  drivers/char/tpm/tpm-dev-common.c |  8 ++---
>  drivers/char/tpm/tpm.h            |  2 ++
>  drivers/char/tpm/tpm2-space.c     |  5 ++-
>  4 files changed, 52 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index ba906966721a..3d69ccff4c2a 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -144,7 +144,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
>  EXPORT_SYMBOL_GPL(tpm_chip_stop);
>  
>  /**
> - * tpm_try_get_ops() - Get a ref to the tpm_chip
> + * tpm_internal_try_get_ops() - Get a ref to the tpm_chip
>   * @chip: Chip to ref
>   *
>   * The caller must already have some kind of locking to ensure that chip is
> @@ -154,7 +154,7 @@ EXPORT_SYMBOL_GPL(tpm_chip_stop);
>   *
>   * Returns -ERRNO if the chip could not be got.
>   */
> -int tpm_try_get_ops(struct tpm_chip *chip)
> +int tpm_internal_try_get_ops(struct tpm_chip *chip)

I'd rather name the new function e.g., tpm_try_get_ops_locked.

Like both from the perspective of more localized patch and also
that "locked" documents more tan "internal".

BR, Jarkko

Re: [RFC PATCH 4/4] tpm: Require O_EXCL for exclusive /dev/tpm access

[Jarkko Sakkinen]

On Tue, Sep 02, 2025 at 06:27:17PM +0100, Jonathan McDowell wrote:
> From: Jonathan McDowell <noodles@meta.com>
> 
> Given that /dev/tpm has not had exclusive access to the TPM since the
> existence of the kernel resource broker and other internal users, stop
> defaulted to exclusive access to the first client that opens the device.
> Continue to support exclusive access, but only with the use of the
> O_EXCL flag on device open.
> 
> Signed-off-by: Jonathan McDowell <noodles@meta.com>
> ---
>  drivers/char/tpm/tpm-dev.c | 25 +++++++++++++++++++------
>  drivers/char/tpm/tpm-dev.h |  1 +
>  2 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
> index 80c4b3f3ad18..8921bbb541c1 100644
> --- a/drivers/char/tpm/tpm-dev.c
> +++ b/drivers/char/tpm/tpm-dev.c
> @@ -19,15 +19,21 @@ static int tpm_open(struct inode *inode, struct file *file)
>  {
>  	struct tpm_chip *chip;
>  	struct file_priv *priv;
> +	int rc;
>  
>  	chip = container_of(inode->i_cdev, struct tpm_chip, cdev);
>  
>  	/*
> -	 * Only one client is allowed to have /dev/tpm0 open at a time, so we
> -	 * treat it as a write lock. The shared /dev/tpmrm0 is treated as a
> -	 * read lock.
> +	 * If a client uses the O_EXCL flag then it expects to be the only TPM
> +	 * user, so we treat it as a write lock. Otherwise we do as /dev/tpmrm
> +	 * and use a read lock.
>  	 */
> -	if (!down_write_trylock(&chip->open_lock)) {
> +	if (file->f_flags & O_EXCL)
> +		rc = down_write_trylock(&chip->open_lock);
> +	else
> +		rc = down_read_trylock(&chip->open_lock);
> +
> +	if (!rc) {
>  		dev_dbg(&chip->dev, "Another process owns this TPM\n");
>  		return -EBUSY;
>  	}
> @@ -35,13 +41,17 @@ static int tpm_open(struct inode *inode, struct file *file)
>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>  	if (priv == NULL)
>  		goto out;
> +	priv->exclusive = (file->f_flags & O_EXCL);
>  
>  	tpm_common_open(file, chip, priv, NULL);
>  
>  	return 0;
>  
>   out:
> -	up_write(&chip->open_lock);
> +	if (file->f_flags & O_EXCL)
> +		up_write(&chip->open_lock);
> +	else
> +		up_read(&chip->open_lock);
>  	return -ENOMEM;
>  }
>  
> @@ -53,7 +63,10 @@ static int tpm_release(struct inode *inode, struct file *file)
>  	struct file_priv *priv = file->private_data;
>  
>  	tpm_common_release(file, priv);
> -	up_write(&priv->chip->open_lock);
> +	if (priv->exclusive)
> +		up_write(&priv->chip->open_lock);
> +	else
> +		up_read(&priv->chip->open_lock);
>  	kfree(priv);
>  
>  	return 0;
> diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
> index f3742bcc73e3..0ad8504c73e4 100644
> --- a/drivers/char/tpm/tpm-dev.h
> +++ b/drivers/char/tpm/tpm-dev.h
> @@ -17,6 +17,7 @@ struct file_priv {
>  	ssize_t response_length;
>  	bool response_read;
>  	bool command_enqueued;
> +	bool exclusive;
>  
>  	u8 data_buffer[TPM_BUFSIZE];
>  };
> -- 
> 2.51.0
> 

I'll hold with testing to +1 version but overall patch set looks good.

BR, Jarkko

[PATCH v2 0/4] Re-establish ability for exclusive TPM access to userspace

[Jonathan McDowell]

I hit a problem last week were ~ 1% of TPM firmware upgrades were
failing. Investigating revealed the issue was that although the upgrade
tool uses /dev/tpm0 this does not actually prevent access via
/dev/tpmrm0, nor internal kernel users. It *does* prevent access to
others via /dev/tpm0

So the upgrade process started, the HW RNG came in to get some
randomness in the middle, did the HMAC context dance, and confused
everything to the point the TPM was no longer visible to the OS even
after a reboot.

Thankfully I've been able to recover those devices, but really what I'd
like is the ability for a userspace tool to exclusively access the TPM
without something coming in behind it. Given the lightweight attempt at
locking that already exists I think this was the original intention.

As an initial approach I propose this patch set; I don't think the first
2 patches are controversial, but the blocking of kernel access + switch
to O_EXCEL in patches 3 + 4 might be. I'm open to alternative
suggestions about how to achieve this.

(I've sent a separate standalone patch that allows the TPM HW RNG to be
disabled at run time, but even with that I think something like this is
a good idea as well.)

Jonathan McDowell (5):
  tpm: Ensure exclusive userspace access when using /dev/tpm<n>
  tpm: Remove tpm_find_get_ops
  tpm: Allow for exclusive TPM access when using /dev/tpm<n>
  tpm: Require O_EXCL for exclusive /dev/tpm access
  hwrng: core - Allow runtime disabling of the HW RNG

 drivers/char/hw_random/core.c     |  9 ++--
 drivers/char/tpm/tpm-chip.c       | 90 +++++++++++++++----------------
 drivers/char/tpm/tpm-dev-common.c |  8 +--
 drivers/char/tpm/tpm-dev.c        | 27 +++++++---
 drivers/char/tpm/tpm-dev.h        |  1 +
 drivers/char/tpm/tpm-interface.c  | 20 +++++--
 drivers/char/tpm/tpm.h            |  3 +-
 drivers/char/tpm/tpm2-space.c     |  5 +-
 drivers/char/tpm/tpm_tis_core.c   |  3 +-
 drivers/char/tpm/tpmrm-dev.c      | 20 ++++++-
 include/linux/tpm.h               |  3 +-
 11 files changed, 118 insertions(+), 71 deletions(-)

-- 
2.51.0

[PATCH v2 1/4] tpm: Ensure exclusive userspace access when using /dev/tpm<n>

[Jonathan McDowell]

From: Jonathan McDowell <noodles@meta.com>

There is an is_open lock on /dev/tpm<n> that dates back to at least
2013, but it only prevents multiple accesses via *this* interface. It is
perfectly possible for userspace to use /dev/tpmrm<n>, or the kernel to
use the internal interfaces, to access the TPM. For tooling expecting
exclusive access, such as firmware updates, this can cause issues.

Close the userspace loophole by changing the simple bit lock to a full
read/write mutex. Direct /dev/tpm<n> access needs an exclusive write
lock, the resource broker continues to allow concurrent access *except*
when /dev/tpm<n> is open.

Signed-off-by: Jonathan McDowell <noodles@meta.com>
---
v2: Change error path label to err instead of out. Rework commit
    message.

 drivers/char/tpm/tpm-chip.c  |  1 +
 drivers/char/tpm/tpm-dev.c   | 14 ++++++++------
 drivers/char/tpm/tpmrm-dev.c | 20 ++++++++++++++++++--
 include/linux/tpm.h          |  3 ++-
 4 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index e25daf2396d3..8c8e9054762a 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -338,6 +338,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 
 	mutex_init(&chip->tpm_mutex);
 	init_rwsem(&chip->ops_sem);
+	init_rwsem(&chip->open_lock);
 
 	chip->ops = ops;
 
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 97c94b5e9340..80c4b3f3ad18 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -22,10 +22,12 @@ 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)) {
+	/*
+	 * Only one client is allowed to have /dev/tpm0 open at a time, so we
+	 * treat it as a write lock. The shared /dev/tpmrm0 is treated as a
+	 * read lock.
+	 */
+	if (!down_write_trylock(&chip->open_lock)) {
 		dev_dbg(&chip->dev, "Another process owns this TPM\n");
 		return -EBUSY;
 	}
@@ -39,7 +41,7 @@ static int tpm_open(struct inode *inode, struct file *file)
 	return 0;
 
  out:
-	clear_bit(0, &chip->is_open);
+	up_write(&chip->open_lock);
 	return -ENOMEM;
 }
 
@@ -51,7 +53,7 @@ static int tpm_release(struct inode *inode, struct file *file)
 	struct file_priv *priv = file->private_data;
 
 	tpm_common_release(file, priv);
-	clear_bit(0, &priv->chip->is_open);
+	up_write(&priv->chip->open_lock);
 	kfree(priv);
 
 	return 0;
diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index c25df7ea064e..13322dd9ac9e 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -17,19 +17,34 @@ static int tpmrm_open(struct inode *inode, struct file *file)
 	int rc;
 
 	chip = container_of(inode->i_cdev, struct tpm_chip, cdevs);
+
+	/*
+	 * Only one client is allowed to have /dev/tpm0 open at a time, so we
+	 * treat it as a write lock. The shared /dev/tpmrm0 is treated as a
+	 * read lock.
+	 */
+	if (!down_read_trylock(&chip->open_lock)) {
+		dev_dbg(&chip->dev, "Another process owns this TPM\n");
+		return -EBUSY;
+	}
+
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (priv == NULL)
-		return -ENOMEM;
+		goto err;
 
 	rc = tpm2_init_space(&priv->space, TPM2_SPACE_BUFFER_SIZE);
 	if (rc) {
 		kfree(priv);
-		return -ENOMEM;
+		goto err;
 	}
 
 	tpm_common_open(file, chip, &priv->priv, &priv->space);
 
 	return 0;
+
+err:
+	up_read(&chip->open_lock);
+	return -ENOMEM;
 }
 
 static int tpmrm_release(struct inode *inode, struct file *file)
@@ -40,6 +55,7 @@ static int tpmrm_release(struct inode *inode, struct file *file)
 	tpm_common_release(file, fpriv);
 	tpm2_del_space(fpriv->chip, &priv->space);
 	kfree(priv);
+	up_read(&fpriv->chip->open_lock);
 
 	return 0;
 }
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index b0e9eb5ef022..548362d20b32 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -22,6 +22,7 @@
 #include <linux/cdev.h>
 #include <linux/fs.h>
 #include <linux/highmem.h>
+#include <linux/rwsem.h>
 #include <crypto/hash_info.h>
 #include <crypto/aes.h>
 
@@ -168,7 +169,7 @@ struct tpm_chip {
 	unsigned int flags;
 
 	int dev_num;		/* /dev/tpm# */
-	unsigned long is_open;	/* only one allowed */
+	struct rw_semaphore open_lock;
 
 	char hwrng_name[64];
 	struct hwrng hwrng;
-- 
2.51.0

[PATCH v2 2/4] tpm: Remove tpm_find_get_ops

[Jonathan McDowell]

From: Jonathan McDowell <noodles@meta.com>

tpm_find_get_ops() looks for the first valid TPM if the caller passes in
NULL. All internal users have been converted to either associate
themselves with a TPM directly, or call tpm_default_chip() as part of
their setup. Remove the no longer necessary tpm_find_get_ops().

Signed-off-by: Jonathan McDowell <noodles@meta.com>
---
 drivers/char/tpm/tpm-chip.c      | 36 --------------------------------
 drivers/char/tpm/tpm-interface.c | 20 ++++++++++++++----
 drivers/char/tpm/tpm.h           |  1 -
 drivers/char/tpm/tpm_tis_core.c  |  3 +--
 4 files changed, 17 insertions(+), 43 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 8c8e9054762a..ba906966721a 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -230,42 +230,6 @@ struct tpm_chip *tpm_default_chip(void)
 }
 EXPORT_SYMBOL_GPL(tpm_default_chip);
 
-/**
- * tpm_find_get_ops() - find and reserve a TPM chip
- * @chip:	a &struct tpm_chip instance, %NULL for the default chip
- *
- * Finds a TPM chip and reserves its class device and operations. The chip must
- * be released with tpm_put_ops() after use.
- * This function is for internal use only. It supports existing TPM callers
- * by accepting NULL, but those callers should be converted to pass in a chip
- * directly.
- *
- * Return:
- * A reserved &struct tpm_chip instance.
- * %NULL if a chip is not found.
- * %NULL if the chip is not available.
- */
-struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip)
-{
-	int rc;
-
-	if (chip) {
-		if (!tpm_try_get_ops(chip))
-			return chip;
-		return NULL;
-	}
-
-	chip = tpm_default_chip();
-	if (!chip)
-		return NULL;
-	rc = tpm_try_get_ops(chip);
-	/* release additional reference we got from tpm_default_chip() */
-	put_device(&chip->dev);
-	if (rc)
-		return NULL;
-	return chip;
-}
-
 /**
  * tpm_dev_release() - free chip memory and the device number
  * @dev: the character device for the TPM chip
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index b71725827743..8f65dc06a157 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -313,10 +313,13 @@ int tpm_is_tpm2(struct tpm_chip *chip)
 {
 	int rc;
 
-	chip = tpm_find_get_ops(chip);
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
 	rc = (chip->flags & TPM_CHIP_FLAG_TPM2) != 0;
 
 	tpm_put_ops(chip);
@@ -338,10 +341,13 @@ int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 {
 	int rc;
 
-	chip = tpm_find_get_ops(chip);
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		rc = tpm2_pcr_read(chip, pcr_idx, digest, NULL);
 	else
@@ -369,10 +375,13 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 	int rc;
 	int i;
 
-	chip = tpm_find_get_ops(chip);
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
 	for (i = 0; i < chip->nr_allocated_banks; i++) {
 		if (digests[i].alg_id != chip->allocated_banks[i].alg_id) {
 			rc = -EINVAL;
@@ -492,10 +501,13 @@ int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
 	if (!out || max > TPM_MAX_RNG_DATA)
 		return -EINVAL;
 
-	chip = tpm_find_get_ops(chip);
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		rc = tpm2_get_random(chip, out, max);
 	else
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 7bb87fa5f7a1..9c158c55c05f 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -267,7 +267,6 @@ static inline void tpm_msleep(unsigned int delay_msec)
 int tpm_chip_bootstrap(struct tpm_chip *chip);
 int tpm_chip_start(struct tpm_chip *chip);
 void tpm_chip_stop(struct tpm_chip *chip);
-struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
 
 struct tpm_chip *tpm_chip_alloc(struct device *dev,
 				const struct tpm_class_ops *ops);
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 4b12c4b9da8b..73b94f4daf4b 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -265,8 +265,7 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
 
 			/*
 			 * Dump stack for forensics, as invalid TPM_STS.x could be
-			 * potentially triggered by impaired tpm_try_get_ops() or
-			 * tpm_find_get_ops().
+			 * potentially triggered by impaired tpm_try_get_ops().
 			 */
 			dump_stack();
 		}
-- 
2.51.0

[PATCH v2 3/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>

[Jonathan McDowell]

From: Jonathan McDowell <noodles@meta.com>

There are situations where userspace might reasonably desire exclusive
access to the TPM, or the kernel's internal context saving + flushing
may cause issues, for example when performing firmware upgrades. Extend
the locking already used for avoiding concurrent userspace access to
prevent internal users of the TPM when /dev/tpm<n> is in use.

The few internal users who already hold the open_lock are changed to use
tpm_(try_get|put)_ops_locked, with the old tpm_(try_get|put)_ops
functions changing to obtain read access to the open_lock.  We return
-EBUSY when another user has exclusive access, rather than adding waits.

Signed-off-by: Jonathan McDowell <noodles@meta.com>
---
v2: Switch to _locked instead of _internal_ for function names.

 drivers/char/tpm/tpm-chip.c       | 53 +++++++++++++++++++++++++------
 drivers/char/tpm/tpm-dev-common.c |  8 ++---
 drivers/char/tpm/tpm.h            |  2 ++
 drivers/char/tpm/tpm2-space.c     |  5 ++-
 4 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index ba906966721a..687f6d8cd601 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -144,7 +144,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
 EXPORT_SYMBOL_GPL(tpm_chip_stop);
 
 /**
- * tpm_try_get_ops() - Get a ref to the tpm_chip
+ * tpm_try_get_ops_locked() - Get a ref to the tpm_chip
  * @chip: Chip to ref
  *
  * The caller must already have some kind of locking to ensure that chip is
@@ -154,7 +154,7 @@ EXPORT_SYMBOL_GPL(tpm_chip_stop);
  *
  * Returns -ERRNO if the chip could not be got.
  */
-int tpm_try_get_ops(struct tpm_chip *chip)
+int tpm_try_get_ops_locked(struct tpm_chip *chip)
 {
 	int rc = -EIO;
 
@@ -185,22 +185,57 @@ int tpm_try_get_ops(struct tpm_chip *chip)
 	put_device(&chip->dev);
 	return rc;
 }
-EXPORT_SYMBOL_GPL(tpm_try_get_ops);
 
 /**
- * tpm_put_ops() - Release a ref to the tpm_chip
+ * tpm_put_ops_locked() - Release a ref to the tpm_chip
  * @chip: Chip to put
  *
- * This is the opposite pair to tpm_try_get_ops(). After this returns chip may
- * be kfree'd.
+ * This is the opposite pair to tpm_try_get_ops_locked(). After this returns
+ * chip may be kfree'd.
  */
-void tpm_put_ops(struct tpm_chip *chip)
+void tpm_put_ops_locked(struct tpm_chip *chip)
 {
 	tpm_chip_stop(chip);
 	mutex_unlock(&chip->tpm_mutex);
 	up_read(&chip->ops_sem);
 	put_device(&chip->dev);
 }
+
+/**
+ * tpm_try_get_ops() - Get a ref to the tpm_chip
+ * @chip: Chip to ref
+ *
+ * The caller must already have some kind of locking to ensure that chip is
+ * valid. This function will attempt to get the open_lock for the chip,
+ * ensuring no other user is expecting exclusive access, before locking the
+ * chip so that the ops member can be accessed safely. The locking prevents
+ * tpm_chip_unregister from completing, so it should not be held for long
+ * periods.
+ *
+ * Returns -ERRNO if the chip could not be got.
+ */
+int tpm_try_get_ops(struct tpm_chip *chip)
+{
+	if (!down_read_trylock(&chip->open_lock))
+		return -EBUSY;
+
+	return tpm_try_get_ops_locked(chip);
+}
+EXPORT_SYMBOL_GPL(tpm_try_get_ops);
+
+/**
+ * tpm_put_ops() - Release a ref to the tpm_chip
+ * @chip: Chip to put
+ *
+ * This is the opposite pair to tpm_try_get_ops(). After this returns
+ * chip may be kfree'd.
+ */
+void tpm_put_ops(struct tpm_chip *chip)
+{
+	tpm_put_ops_locked(chip);
+
+	up_read(&chip->open_lock);
+}
 EXPORT_SYMBOL_GPL(tpm_put_ops);
 
 /**
@@ -644,10 +679,10 @@ void tpm_chip_unregister(struct tpm_chip *chip)
 #ifdef CONFIG_TCG_TPM2_HMAC
 	int rc;
 
-	rc = tpm_try_get_ops(chip);
+	rc = tpm_try_get_ops_locked(chip);
 	if (!rc) {
 		tpm2_end_auth_session(chip);
-		tpm_put_ops(chip);
+		tpm_put_ops_locked(chip);
 	}
 #endif
 
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index f2a5e09257dd..0f5bc63411aa 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -65,7 +65,7 @@ static void tpm_dev_async_work(struct work_struct *work)
 
 	mutex_lock(&priv->buffer_mutex);
 	priv->command_enqueued = false;
-	ret = tpm_try_get_ops(priv->chip);
+	ret = tpm_try_get_ops_locked(priv->chip);
 	if (ret) {
 		priv->response_length = ret;
 		goto out;
@@ -73,7 +73,7 @@ static void tpm_dev_async_work(struct work_struct *work)
 
 	ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
 			       sizeof(priv->data_buffer));
-	tpm_put_ops(priv->chip);
+	tpm_put_ops_locked(priv->chip);
 
 	/*
 	 * If ret is > 0 then tpm_dev_transmit returned the size of the
@@ -220,14 +220,14 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
 	 * lock during this period so that the tpm can be unregistered even if
 	 * the char dev is held open.
 	 */
-	if (tpm_try_get_ops(priv->chip)) {
+	if (tpm_try_get_ops_locked(priv->chip)) {
 		ret = -EPIPE;
 		goto out;
 	}
 
 	ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
 			       sizeof(priv->data_buffer));
-	tpm_put_ops(priv->chip);
+	tpm_put_ops_locked(priv->chip);
 
 	if (ret > 0) {
 		priv->response_length = ret;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 9c158c55c05f..c2ec56e2cd24 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -272,6 +272,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
 				const struct tpm_class_ops *ops);
 struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
 				 const struct tpm_class_ops *ops);
+int tpm_try_get_ops_locked(struct tpm_chip *chip);
+void tpm_put_ops_locked(struct tpm_chip *chip);
 int tpm_chip_register(struct tpm_chip *chip);
 void tpm_chip_unregister(struct tpm_chip *chip);
 
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 60354cd53b5c..0ad5e18355e0 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -58,10 +58,9 @@ int tpm2_init_space(struct tpm_space *space, unsigned int buf_size)
 
 void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
 {
-
-	if (tpm_try_get_ops(chip) == 0) {
+	if (tpm_try_get_ops_locked(chip) == 0) {
 		tpm2_flush_sessions(chip, space);
-		tpm_put_ops(chip);
+		tpm_put_ops_locked(chip);
 	}
 
 	kfree(space->context_buf);
-- 
2.51.0

[PATCH v2 4/4] tpm: Require O_EXCL for exclusive /dev/tpm access

[Jonathan McDowell]

From: Jonathan McDowell <noodles@meta.com>

Given that /dev/tpm has not had exclusive access to the TPM since the
existence of the kernel resource broker and other internal users, stop
defaulted to exclusive access to the first client that opens the device.
Continue to support exclusive access, but only with the use of the
O_EXCL flag on device open.

Signed-off-by: Jonathan McDowell <noodles@meta.com>
---
 drivers/char/tpm/tpm-dev.c | 25 +++++++++++++++++++------
 drivers/char/tpm/tpm-dev.h |  1 +
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 80c4b3f3ad18..8921bbb541c1 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -19,15 +19,21 @@ static int tpm_open(struct inode *inode, struct file *file)
 {
 	struct tpm_chip *chip;
 	struct file_priv *priv;
+	int rc;
 
 	chip = container_of(inode->i_cdev, struct tpm_chip, cdev);
 
 	/*
-	 * Only one client is allowed to have /dev/tpm0 open at a time, so we
-	 * treat it as a write lock. The shared /dev/tpmrm0 is treated as a
-	 * read lock.
+	 * If a client uses the O_EXCL flag then it expects to be the only TPM
+	 * user, so we treat it as a write lock. Otherwise we do as /dev/tpmrm
+	 * and use a read lock.
 	 */
-	if (!down_write_trylock(&chip->open_lock)) {
+	if (file->f_flags & O_EXCL)
+		rc = down_write_trylock(&chip->open_lock);
+	else
+		rc = down_read_trylock(&chip->open_lock);
+
+	if (!rc) {
 		dev_dbg(&chip->dev, "Another process owns this TPM\n");
 		return -EBUSY;
 	}
@@ -35,13 +41,17 @@ static int tpm_open(struct inode *inode, struct file *file)
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (priv == NULL)
 		goto out;
+	priv->exclusive = (file->f_flags & O_EXCL);
 
 	tpm_common_open(file, chip, priv, NULL);
 
 	return 0;
 
  out:
-	up_write(&chip->open_lock);
+	if (file->f_flags & O_EXCL)
+		up_write(&chip->open_lock);
+	else
+		up_read(&chip->open_lock);
 	return -ENOMEM;
 }
 
@@ -53,7 +63,10 @@ static int tpm_release(struct inode *inode, struct file *file)
 	struct file_priv *priv = file->private_data;
 
 	tpm_common_release(file, priv);
-	up_write(&priv->chip->open_lock);
+	if (priv->exclusive)
+		up_write(&priv->chip->open_lock);
+	else
+		up_read(&priv->chip->open_lock);
 	kfree(priv);
 
 	return 0;
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index f3742bcc73e3..0ad8504c73e4 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -17,6 +17,7 @@ struct file_priv {
 	ssize_t response_length;
 	bool response_read;
 	bool command_enqueued;
+	bool exclusive;
 
 	u8 data_buffer[TPM_BUFSIZE];
 };
-- 
2.51.0

Re: [PATCH v2 1/4] tpm: Ensure exclusive userspace access when using /dev/tpm<n>

[Jarkko Sakkinen]

On Tue, Sep 23, 2025 at 06:10:00PM +0100, Jonathan McDowell wrote:
> From: Jonathan McDowell <noodles@meta.com>
> 
> There is an is_open lock on /dev/tpm<n> that dates back to at least
> 2013, but it only prevents multiple accesses via *this* interface. It is
> perfectly possible for userspace to use /dev/tpmrm<n>, or the kernel to
> use the internal interfaces, to access the TPM. For tooling expecting
> exclusive access, such as firmware updates, this can cause issues.
> 
> Close the userspace loophole by changing the simple bit lock to a full
> read/write mutex. Direct /dev/tpm<n> access needs an exclusive write
> lock, the resource broker continues to allow concurrent access *except*
> when /dev/tpm<n> is open.
> 
> Signed-off-by: Jonathan McDowell <noodles@meta.com>

I'm mostly thinking should be tag it as bug fix and backport or not.

> ---
> v2: Change error path label to err instead of out. Rework commit
>     message.
> 
>  drivers/char/tpm/tpm-chip.c  |  1 +
>  drivers/char/tpm/tpm-dev.c   | 14 ++++++++------
>  drivers/char/tpm/tpmrm-dev.c | 20 ++++++++++++++++++--
>  include/linux/tpm.h          |  3 ++-
>  4 files changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index e25daf2396d3..8c8e9054762a 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -338,6 +338,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
>  
>  	mutex_init(&chip->tpm_mutex);
>  	init_rwsem(&chip->ops_sem);
> +	init_rwsem(&chip->open_lock);
>  
>  	chip->ops = ops;
>  
> diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
> index 97c94b5e9340..80c4b3f3ad18 100644
> --- a/drivers/char/tpm/tpm-dev.c
> +++ b/drivers/char/tpm/tpm-dev.c
> @@ -22,10 +22,12 @@ 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)) {
> +	/*
> +	 * Only one client is allowed to have /dev/tpm0 open at a time, so we
> +	 * treat it as a write lock. The shared /dev/tpmrm0 is treated as a
> +	 * read lock.
> +	 */
> +	if (!down_write_trylock(&chip->open_lock)) {
>  		dev_dbg(&chip->dev, "Another process owns this TPM\n");
>  		return -EBUSY;
>  	}
> @@ -39,7 +41,7 @@ static int tpm_open(struct inode *inode, struct file *file)
>  	return 0;
>  
>   out:
> -	clear_bit(0, &chip->is_open);
> +	up_write(&chip->open_lock);
>  	return -ENOMEM;
>  }
>  
> @@ -51,7 +53,7 @@ static int tpm_release(struct inode *inode, struct file *file)
>  	struct file_priv *priv = file->private_data;
>  
>  	tpm_common_release(file, priv);
> -	clear_bit(0, &priv->chip->is_open);
> +	up_write(&priv->chip->open_lock);
>  	kfree(priv);
>  
>  	return 0;
> diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
> index c25df7ea064e..13322dd9ac9e 100644
> --- a/drivers/char/tpm/tpmrm-dev.c
> +++ b/drivers/char/tpm/tpmrm-dev.c
> @@ -17,19 +17,34 @@ static int tpmrm_open(struct inode *inode, struct file *file)
>  	int rc;
>  
>  	chip = container_of(inode->i_cdev, struct tpm_chip, cdevs);
> +
> +	/*
> +	 * Only one client is allowed to have /dev/tpm0 open at a time, so we
> +	 * treat it as a write lock. The shared /dev/tpmrm0 is treated as a
> +	 * read lock.
> +	 */
> +	if (!down_read_trylock(&chip->open_lock)) {
> +		dev_dbg(&chip->dev, "Another process owns this TPM\n");
> +		return -EBUSY;
> +	}
> +
>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>  	if (priv == NULL)
> -		return -ENOMEM;
> +		goto err;
>  
>  	rc = tpm2_init_space(&priv->space, TPM2_SPACE_BUFFER_SIZE);
>  	if (rc) {
>  		kfree(priv);
> -		return -ENOMEM;
> +		goto err;
>  	}
>  
>  	tpm_common_open(file, chip, &priv->priv, &priv->space);
>  
>  	return 0;
> +
> +err:
> +	up_read(&chip->open_lock);
> +	return -ENOMEM;
>  }
>  
>  static int tpmrm_release(struct inode *inode, struct file *file)
> @@ -40,6 +55,7 @@ static int tpmrm_release(struct inode *inode, struct file *file)
>  	tpm_common_release(file, fpriv);
>  	tpm2_del_space(fpriv->chip, &priv->space);
>  	kfree(priv);
> +	up_read(&fpriv->chip->open_lock);
>  
>  	return 0;
>  }
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index b0e9eb5ef022..548362d20b32 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -22,6 +22,7 @@
>  #include <linux/cdev.h>
>  #include <linux/fs.h>
>  #include <linux/highmem.h>
> +#include <linux/rwsem.h>
>  #include <crypto/hash_info.h>
>  #include <crypto/aes.h>
>  
> @@ -168,7 +169,7 @@ struct tpm_chip {
>  	unsigned int flags;
>  
>  	int dev_num;		/* /dev/tpm# */
> -	unsigned long is_open;	/* only one allowed */
> +	struct rw_semaphore open_lock;
>  
>  	char hwrng_name[64];
>  	struct hwrng hwrng;
> -- 
> 2.51.0
> 

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

Re: [PATCH v2 2/4] tpm: Remove tpm_find_get_ops

[Jarkko Sakkinen]

On Tue, Sep 23, 2025 at 06:10:11PM +0100, Jonathan McDowell wrote:
> From: Jonathan McDowell <noodles@meta.com>
> 
> tpm_find_get_ops() looks for the first valid TPM if the caller passes in
> NULL. All internal users have been converted to either associate
> themselves with a TPM directly, or call tpm_default_chip() as part of
> their setup. Remove the no longer necessary tpm_find_get_ops().
> 
> Signed-off-by: Jonathan McDowell <noodles@meta.com>

This patch fits really well with the changes I've been working on
streamlining some parts (mainly tpm_bufs and sanitizing tpm2-sessions).

I might have ended up doing this type of path myself so I'm glad
it's done.

> ---
>  drivers/char/tpm/tpm-chip.c      | 36 --------------------------------
>  drivers/char/tpm/tpm-interface.c | 20 ++++++++++++++----
>  drivers/char/tpm/tpm.h           |  1 -
>  drivers/char/tpm/tpm_tis_core.c  |  3 +--
>  4 files changed, 17 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 8c8e9054762a..ba906966721a 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -230,42 +230,6 @@ struct tpm_chip *tpm_default_chip(void)
>  }
>  EXPORT_SYMBOL_GPL(tpm_default_chip);
>  
> -/**
> - * tpm_find_get_ops() - find and reserve a TPM chip
> - * @chip:	a &struct tpm_chip instance, %NULL for the default chip
> - *
> - * Finds a TPM chip and reserves its class device and operations. The chip must
> - * be released with tpm_put_ops() after use.
> - * This function is for internal use only. It supports existing TPM callers
> - * by accepting NULL, but those callers should be converted to pass in a chip
> - * directly.
> - *
> - * Return:
> - * A reserved &struct tpm_chip instance.
> - * %NULL if a chip is not found.
> - * %NULL if the chip is not available.
> - */
> -struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip)
> -{
> -	int rc;
> -
> -	if (chip) {
> -		if (!tpm_try_get_ops(chip))
> -			return chip;
> -		return NULL;
> -	}
> -
> -	chip = tpm_default_chip();
> -	if (!chip)
> -		return NULL;
> -	rc = tpm_try_get_ops(chip);
> -	/* release additional reference we got from tpm_default_chip() */
> -	put_device(&chip->dev);
> -	if (rc)
> -		return NULL;
> -	return chip;
> -}
> -
>  /**
>   * tpm_dev_release() - free chip memory and the device number
>   * @dev: the character device for the TPM chip
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index b71725827743..8f65dc06a157 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -313,10 +313,13 @@ int tpm_is_tpm2(struct tpm_chip *chip)
>  {
>  	int rc;
>  
> -	chip = tpm_find_get_ops(chip);
>  	if (!chip)
>  		return -ENODEV;
>  
> +	rc = tpm_try_get_ops(chip);
> +	if (rc)
> +		return rc;
> +
>  	rc = (chip->flags & TPM_CHIP_FLAG_TPM2) != 0;
>  
>  	tpm_put_ops(chip);
> @@ -338,10 +341,13 @@ int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>  {
>  	int rc;
>  
> -	chip = tpm_find_get_ops(chip);
>  	if (!chip)
>  		return -ENODEV;
>  
> +	rc = tpm_try_get_ops(chip);
> +	if (rc)
> +		return rc;
> +
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		rc = tpm2_pcr_read(chip, pcr_idx, digest, NULL);
>  	else
> @@ -369,10 +375,13 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>  	int rc;
>  	int i;
>  
> -	chip = tpm_find_get_ops(chip);
>  	if (!chip)
>  		return -ENODEV;
>  
> +	rc = tpm_try_get_ops(chip);
> +	if (rc)
> +		return rc;
> +
>  	for (i = 0; i < chip->nr_allocated_banks; i++) {
>  		if (digests[i].alg_id != chip->allocated_banks[i].alg_id) {
>  			rc = -EINVAL;
> @@ -492,10 +501,13 @@ int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
>  	if (!out || max > TPM_MAX_RNG_DATA)
>  		return -EINVAL;
>  
> -	chip = tpm_find_get_ops(chip);
>  	if (!chip)
>  		return -ENODEV;
>  
> +	rc = tpm_try_get_ops(chip);
> +	if (rc)
> +		return rc;
> +
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		rc = tpm2_get_random(chip, out, max);
>  	else
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 7bb87fa5f7a1..9c158c55c05f 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -267,7 +267,6 @@ static inline void tpm_msleep(unsigned int delay_msec)
>  int tpm_chip_bootstrap(struct tpm_chip *chip);
>  int tpm_chip_start(struct tpm_chip *chip);
>  void tpm_chip_stop(struct tpm_chip *chip);
> -struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
>  
>  struct tpm_chip *tpm_chip_alloc(struct device *dev,
>  				const struct tpm_class_ops *ops);
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 4b12c4b9da8b..73b94f4daf4b 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -265,8 +265,7 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
>  
>  			/*
>  			 * Dump stack for forensics, as invalid TPM_STS.x could be
> -			 * potentially triggered by impaired tpm_try_get_ops() or
> -			 * tpm_find_get_ops().
> +			 * potentially triggered by impaired tpm_try_get_ops().
>  			 */
>  			dump_stack();
>  		}
> -- 
> 2.51.0
> 

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

Re: [PATCH v2 3/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>

[Jarkko Sakkinen]

On Tue, Sep 23, 2025 at 06:10:19PM +0100, Jonathan McDowell wrote:
> From: Jonathan McDowell <noodles@meta.com>
> 
> There are situations where userspace might reasonably desire exclusive
> access to the TPM, or the kernel's internal context saving + flushing
> may cause issues, for example when performing firmware upgrades. Extend
> the locking already used for avoiding concurrent userspace access to
> prevent internal users of the TPM when /dev/tpm<n> is in use.
> 
> The few internal users who already hold the open_lock are changed to use
> tpm_(try_get|put)_ops_locked, with the old tpm_(try_get|put)_ops
> functions changing to obtain read access to the open_lock.  We return
> -EBUSY when another user has exclusive access, rather than adding waits.
> 
> Signed-off-by: Jonathan McDowell <noodles@meta.com>

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

> ---
> v2: Switch to _locked instead of _internal_ for function names.
> 
>  drivers/char/tpm/tpm-chip.c       | 53 +++++++++++++++++++++++++------
>  drivers/char/tpm/tpm-dev-common.c |  8 ++---
>  drivers/char/tpm/tpm.h            |  2 ++
>  drivers/char/tpm/tpm2-space.c     |  5 ++-
>  4 files changed, 52 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index ba906966721a..687f6d8cd601 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -144,7 +144,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
>  EXPORT_SYMBOL_GPL(tpm_chip_stop);
>  
>  /**
> - * tpm_try_get_ops() - Get a ref to the tpm_chip
> + * tpm_try_get_ops_locked() - Get a ref to the tpm_chip
>   * @chip: Chip to ref
>   *
>   * The caller must already have some kind of locking to ensure that chip is
> @@ -154,7 +154,7 @@ EXPORT_SYMBOL_GPL(tpm_chip_stop);
>   *
>   * Returns -ERRNO if the chip could not be got.
>   */
> -int tpm_try_get_ops(struct tpm_chip *chip)
> +int tpm_try_get_ops_locked(struct tpm_chip *chip)
>  {
>  	int rc = -EIO;
>  
> @@ -185,22 +185,57 @@ int tpm_try_get_ops(struct tpm_chip *chip)
>  	put_device(&chip->dev);
>  	return rc;
>  }
> -EXPORT_SYMBOL_GPL(tpm_try_get_ops);
>  
>  /**
> - * tpm_put_ops() - Release a ref to the tpm_chip
> + * tpm_put_ops_locked() - Release a ref to the tpm_chip
>   * @chip: Chip to put
>   *
> - * This is the opposite pair to tpm_try_get_ops(). After this returns chip may
> - * be kfree'd.
> + * This is the opposite pair to tpm_try_get_ops_locked(). After this returns
> + * chip may be kfree'd.
>   */
> -void tpm_put_ops(struct tpm_chip *chip)
> +void tpm_put_ops_locked(struct tpm_chip *chip)
>  {
>  	tpm_chip_stop(chip);
>  	mutex_unlock(&chip->tpm_mutex);
>  	up_read(&chip->ops_sem);
>  	put_device(&chip->dev);
>  }
> +
> +/**
> + * tpm_try_get_ops() - Get a ref to the tpm_chip
> + * @chip: Chip to ref
> + *
> + * The caller must already have some kind of locking to ensure that chip is
> + * valid. This function will attempt to get the open_lock for the chip,
> + * ensuring no other user is expecting exclusive access, before locking the
> + * chip so that the ops member can be accessed safely. The locking prevents
> + * tpm_chip_unregister from completing, so it should not be held for long
> + * periods.
> + *
> + * Returns -ERRNO if the chip could not be got.
> + */
> +int tpm_try_get_ops(struct tpm_chip *chip)
> +{
> +	if (!down_read_trylock(&chip->open_lock))
> +		return -EBUSY;
> +
> +	return tpm_try_get_ops_locked(chip);
> +}
> +EXPORT_SYMBOL_GPL(tpm_try_get_ops);
> +
> +/**
> + * tpm_put_ops() - Release a ref to the tpm_chip
> + * @chip: Chip to put
> + *
> + * This is the opposite pair to tpm_try_get_ops(). After this returns
> + * chip may be kfree'd.
> + */
> +void tpm_put_ops(struct tpm_chip *chip)
> +{
> +	tpm_put_ops_locked(chip);
> +
> +	up_read(&chip->open_lock);
> +}
>  EXPORT_SYMBOL_GPL(tpm_put_ops);
>  
>  /**
> @@ -644,10 +679,10 @@ void tpm_chip_unregister(struct tpm_chip *chip)
>  #ifdef CONFIG_TCG_TPM2_HMAC
>  	int rc;
>  
> -	rc = tpm_try_get_ops(chip);
> +	rc = tpm_try_get_ops_locked(chip);
>  	if (!rc) {
>  		tpm2_end_auth_session(chip);
> -		tpm_put_ops(chip);
> +		tpm_put_ops_locked(chip);
>  	}
>  #endif
>  
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index f2a5e09257dd..0f5bc63411aa 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -65,7 +65,7 @@ static void tpm_dev_async_work(struct work_struct *work)
>  
>  	mutex_lock(&priv->buffer_mutex);
>  	priv->command_enqueued = false;
> -	ret = tpm_try_get_ops(priv->chip);
> +	ret = tpm_try_get_ops_locked(priv->chip);
>  	if (ret) {
>  		priv->response_length = ret;
>  		goto out;
> @@ -73,7 +73,7 @@ static void tpm_dev_async_work(struct work_struct *work)
>  
>  	ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
>  			       sizeof(priv->data_buffer));
> -	tpm_put_ops(priv->chip);
> +	tpm_put_ops_locked(priv->chip);
>  
>  	/*
>  	 * If ret is > 0 then tpm_dev_transmit returned the size of the
> @@ -220,14 +220,14 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
>  	 * lock during this period so that the tpm can be unregistered even if
>  	 * the char dev is held open.
>  	 */
> -	if (tpm_try_get_ops(priv->chip)) {
> +	if (tpm_try_get_ops_locked(priv->chip)) {
>  		ret = -EPIPE;
>  		goto out;
>  	}
>  
>  	ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
>  			       sizeof(priv->data_buffer));
> -	tpm_put_ops(priv->chip);
> +	tpm_put_ops_locked(priv->chip);
>  
>  	if (ret > 0) {
>  		priv->response_length = ret;
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 9c158c55c05f..c2ec56e2cd24 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -272,6 +272,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
>  				const struct tpm_class_ops *ops);
>  struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
>  				 const struct tpm_class_ops *ops);
> +int tpm_try_get_ops_locked(struct tpm_chip *chip);
> +void tpm_put_ops_locked(struct tpm_chip *chip);
>  int tpm_chip_register(struct tpm_chip *chip);
>  void tpm_chip_unregister(struct tpm_chip *chip);
>  
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index 60354cd53b5c..0ad5e18355e0 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -58,10 +58,9 @@ int tpm2_init_space(struct tpm_space *space, unsigned int buf_size)
>  
>  void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
>  {
> -
> -	if (tpm_try_get_ops(chip) == 0) {
> +	if (tpm_try_get_ops_locked(chip) == 0) {
>  		tpm2_flush_sessions(chip, space);
> -		tpm_put_ops(chip);
> +		tpm_put_ops_locked(chip);
>  	}
>  
>  	kfree(space->context_buf);
> -- 
> 2.51.0
> 

I can take these to the next PR, thanks.

BR, Jarkko

Re: [PATCH v2 4/4] tpm: Require O_EXCL for exclusive /dev/tpm access

[Jarkko Sakkinen]

On Tue, Sep 23, 2025 at 06:10:28PM +0100, Jonathan McDowell wrote:
> From: Jonathan McDowell <noodles@meta.com>
> 
> Given that /dev/tpm has not had exclusive access to the TPM since the
> existence of the kernel resource broker and other internal users, stop
> defaulted to exclusive access to the first client that opens the device.
> Continue to support exclusive access, but only with the use of the
> O_EXCL flag on device open.
> 
> Signed-off-by: Jonathan McDowell <noodles@meta.com>
> ---
>  drivers/char/tpm/tpm-dev.c | 25 +++++++++++++++++++------
>  drivers/char/tpm/tpm-dev.h |  1 +
>  2 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
> index 80c4b3f3ad18..8921bbb541c1 100644
> --- a/drivers/char/tpm/tpm-dev.c
> +++ b/drivers/char/tpm/tpm-dev.c
> @@ -19,15 +19,21 @@ static int tpm_open(struct inode *inode, struct file *file)
>  {
>  	struct tpm_chip *chip;
>  	struct file_priv *priv;
> +	int rc;
>  
>  	chip = container_of(inode->i_cdev, struct tpm_chip, cdev);
>  
>  	/*
> -	 * Only one client is allowed to have /dev/tpm0 open at a time, so we
> -	 * treat it as a write lock. The shared /dev/tpmrm0 is treated as a
> -	 * read lock.
> +	 * If a client uses the O_EXCL flag then it expects to be the only TPM
> +	 * user, so we treat it as a write lock. Otherwise we do as /dev/tpmrm
> +	 * and use a read lock.
>  	 */
> -	if (!down_write_trylock(&chip->open_lock)) {
> +	if (file->f_flags & O_EXCL)
> +		rc = down_write_trylock(&chip->open_lock);
> +	else
> +		rc = down_read_trylock(&chip->open_lock);
> +
> +	if (!rc) {
>  		dev_dbg(&chip->dev, "Another process owns this TPM\n");
>  		return -EBUSY;
>  	}
> @@ -35,13 +41,17 @@ static int tpm_open(struct inode *inode, struct file *file)
>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>  	if (priv == NULL)
>  		goto out;
> +	priv->exclusive = (file->f_flags & O_EXCL);
>  
>  	tpm_common_open(file, chip, priv, NULL);
>  
>  	return 0;
>  
>   out:
> -	up_write(&chip->open_lock);
> +	if (file->f_flags & O_EXCL)
> +		up_write(&chip->open_lock);
> +	else
> +		up_read(&chip->open_lock);
>  	return -ENOMEM;
>  }
>  
> @@ -53,7 +63,10 @@ static int tpm_release(struct inode *inode, struct file *file)
>  	struct file_priv *priv = file->private_data;
>  
>  	tpm_common_release(file, priv);
> -	up_write(&priv->chip->open_lock);
> +	if (priv->exclusive)
> +		up_write(&priv->chip->open_lock);
> +	else
> +		up_read(&priv->chip->open_lock);
>  	kfree(priv);
>  
>  	return 0;
> diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
> index f3742bcc73e3..0ad8504c73e4 100644
> --- a/drivers/char/tpm/tpm-dev.h
> +++ b/drivers/char/tpm/tpm-dev.h
> @@ -17,6 +17,7 @@ struct file_priv {
>  	ssize_t response_length;
>  	bool response_read;
>  	bool command_enqueued;
> +	bool exclusive;
>  
>  	u8 data_buffer[TPM_BUFSIZE];
>  };
> -- 
> 2.51.0
> 

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

[PATCH v3 0/4] pm: Ensure exclusive userspace access when using /dev/tpm<n>

[Jonathan McDowell]

I hit a problem where ~ 1% of TPM firmware upgrades were failing.
Investigating revealed the issue was that although the upgrade tool uses
/dev/tpm0 this does not actually prevent access via /dev/tpmrm0, nor
internal kernel users. It *does* prevent access to others via /dev/tpm0

So the upgrade process started, the HW RNG came in to get some
randomness in the middle, did the HMAC context dance, and confused
everything to the point the TPM was no longer visible to the OS even
after a reboot.

Thankfully I've been able to recover those devices, but really what I'd
like is the ability for a userspace tool to exclusively access the TPM
without something coming in behind it. Given the lightweight attempt at
locking that already exists I think this was the original intention.

I've reworked this series based on feedback received.

Firstly, it's been reordered TPM sharing functionality doesn't break
during bisection.

Secondly, the O_EXCL check has been tightend up to ensure the caller is
also opening the device O_RDWR. Callers shouldn't really be opening the
TPM except for reading + writing, but this should help guard against
unexpected flags usage a bit more.

Finally, this revision keeps the prohibition on more than one user of
/dev/tpm#, to avoid unexpected breakages for clients that expect this to
guard against multiple invocations. A client only then needs to use
O_EXCL if it wants to prevent *all* other access, even with
ContextSaves, such as the firmware upgrade case.

(I've sent a separate standalone patch that allows the TPM HW RNG to be
disabled at run time, and it's now in -next, but even with that I think
something like this is a good idea as well.)

Jonathan McDowell (4):
  tpm: Remove tpm_find_get_ops
  tpm: Add O_EXCL for exclusive /dev/tpm access
  tpm: Include /dev/tpmrm<n> when checking exclusive userspace TPM
    access
  tpm: Allow for exclusive TPM access when using /dev/tpm<n>

 drivers/char/tpm/tpm-chip.c       | 90 +++++++++++++++----------------
 drivers/char/tpm/tpm-dev-common.c |  8 +--
 drivers/char/tpm/tpm-dev.c        | 35 ++++++++++--
 drivers/char/tpm/tpm-dev.h        |  1 +
 drivers/char/tpm/tpm-interface.c  | 20 +++++--
 drivers/char/tpm/tpm.h            |  3 +-
 drivers/char/tpm/tpm2-space.c     |  5 +-
 drivers/char/tpm/tpm_tis_core.c   |  3 +-
 drivers/char/tpm/tpmrm-dev.c      | 20 ++++++-
 include/linux/tpm.h               |  4 +-
 10 files changed, 124 insertions(+), 65 deletions(-)

-- 
2.51.0

[PATCH v3 1/4] tpm: Remove tpm_find_get_ops

[Jonathan McDowell]

From: Jonathan McDowell <noodles@meta.com>

tpm_find_get_ops() looks for the first valid TPM if the caller passes in
NULL. All internal users have been converted to either associate
themselves with a TPM directly, or call tpm_default_chip() as part of
their setup. Remove the no longer necessary tpm_find_get_ops().

Signed-off-by: Jonathan McDowell <noodles@meta.com>
---
v2: No changes.
v3: Move to start of patch series

 drivers/char/tpm/tpm-chip.c      | 36 --------------------------------
 drivers/char/tpm/tpm-interface.c | 20 ++++++++++++++----
 drivers/char/tpm/tpm.h           |  1 -
 drivers/char/tpm/tpm_tis_core.c  |  3 +--
 4 files changed, 17 insertions(+), 43 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index e25daf2396d3..30d00219f9f3 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -230,42 +230,6 @@ struct tpm_chip *tpm_default_chip(void)
 }
 EXPORT_SYMBOL_GPL(tpm_default_chip);
 
-/**
- * tpm_find_get_ops() - find and reserve a TPM chip
- * @chip:	a &struct tpm_chip instance, %NULL for the default chip
- *
- * Finds a TPM chip and reserves its class device and operations. The chip must
- * be released with tpm_put_ops() after use.
- * This function is for internal use only. It supports existing TPM callers
- * by accepting NULL, but those callers should be converted to pass in a chip
- * directly.
- *
- * Return:
- * A reserved &struct tpm_chip instance.
- * %NULL if a chip is not found.
- * %NULL if the chip is not available.
- */
-struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip)
-{
-	int rc;
-
-	if (chip) {
-		if (!tpm_try_get_ops(chip))
-			return chip;
-		return NULL;
-	}
-
-	chip = tpm_default_chip();
-	if (!chip)
-		return NULL;
-	rc = tpm_try_get_ops(chip);
-	/* release additional reference we got from tpm_default_chip() */
-	put_device(&chip->dev);
-	if (rc)
-		return NULL;
-	return chip;
-}
-
 /**
  * tpm_dev_release() - free chip memory and the device number
  * @dev: the character device for the TPM chip
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index c9f173001d0e..f745a098908b 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -313,10 +313,13 @@ int tpm_is_tpm2(struct tpm_chip *chip)
 {
 	int rc;
 
-	chip = tpm_find_get_ops(chip);
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
 	rc = (chip->flags & TPM_CHIP_FLAG_TPM2) != 0;
 
 	tpm_put_ops(chip);
@@ -338,10 +341,13 @@ int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 {
 	int rc;
 
-	chip = tpm_find_get_ops(chip);
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		rc = tpm2_pcr_read(chip, pcr_idx, digest, NULL);
 	else
@@ -369,10 +375,13 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 	int rc;
 	int i;
 
-	chip = tpm_find_get_ops(chip);
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
 	for (i = 0; i < chip->nr_allocated_banks; i++) {
 		if (digests[i].alg_id != chip->allocated_banks[i].alg_id) {
 			rc = -EINVAL;
@@ -492,10 +501,13 @@ int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
 	if (!out || max > TPM_MAX_RNG_DATA)
 		return -EINVAL;
 
-	chip = tpm_find_get_ops(chip);
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc)
+		return rc;
+
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		rc = tpm2_get_random(chip, out, max);
 	else
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2726bd38e5ac..02c07fef41ba 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -267,7 +267,6 @@ static inline void tpm_msleep(unsigned int delay_msec)
 int tpm_chip_bootstrap(struct tpm_chip *chip);
 int tpm_chip_start(struct tpm_chip *chip);
 void tpm_chip_stop(struct tpm_chip *chip);
-struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
 
 struct tpm_chip *tpm_chip_alloc(struct device *dev,
 				const struct tpm_class_ops *ops);
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 8954a8660ffc..e2a1769081b1 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -265,8 +265,7 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
 
 			/*
 			 * Dump stack for forensics, as invalid TPM_STS.x could be
-			 * potentially triggered by impaired tpm_try_get_ops() or
-			 * tpm_find_get_ops().
+			 * potentially triggered by impaired tpm_try_get_ops().
 			 */
 			dump_stack();
 		}
-- 
2.51.0

[PATCH v3 2/4] tpm: Add O_EXCL for exclusive /dev/tpm access

[Jonathan McDowell]

From: Jonathan McDowell <noodles@meta.com>

The /dev/tpm interface allows for only a single user, but does not
prevent access to the TPM via other paths (either internal kernel
interfaces or the /dev/tpmrm interface). Pave the way to being to able
request fully exclusive access by supporting the use of the O_EXCL flag
on device open.

Signed-off-by: Jonathan McDowell <noodles@meta.com>
---
v2: No changes.
v3: Move earlier in series to prevent bisection breakage.
    Check for O_RDWR as well as O_EXCL to guard against clients doing
    odd things.
    Retain single /dev/tpm# user even without O_EXCL.

 drivers/char/tpm/tpm-chip.c |  1 +
 drivers/char/tpm/tpm-dev.c  | 35 ++++++++++++++++++++++++++++++++---
 drivers/char/tpm/tpm-dev.h  |  1 +
 include/linux/tpm.h         |  4 +++-
 4 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 30d00219f9f3..ba906966721a 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -302,6 +302,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 
 	mutex_init(&chip->tpm_mutex);
 	init_rwsem(&chip->ops_sem);
+	init_rwsem(&chip->open_lock);
 
 	chip->ops = ops;
 
diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index 97c94b5e9340..819f3e1546da 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -19,20 +19,41 @@ static int tpm_open(struct inode *inode, struct file *file)
 {
 	struct tpm_chip *chip;
 	struct file_priv *priv;
+	int rc;
 
 	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. */
+	/*
+	 * It's assured that the chip will be opened just once via the direct
+	 * /dev/tpm# interface by the check of is_open variable, which is
+	 * protected by driver_lock.
+	 */
 	if (test_and_set_bit(0, &chip->is_open)) {
 		dev_dbg(&chip->dev, "Another process owns this TPM\n");
 		return -EBUSY;
 	}
 
+	/*
+	 * If a client uses the O_EXCL flag then it expects to be the only TPM
+	 * user across all access paths, so we treat it as a write lock.
+	 * Otherwise we use a read lock, allowing for concurrent openers.
+	 * We make sure the client is opening the device for reading + writing;
+	 * opening for exclusive access doesn't make sense if not.
+	 */
+	if ((file->f_flags & (O_ACCMODE|O_EXCL)) == (O_RDWR|O_EXCL))
+		rc = down_write_trylock(&chip->open_lock);
+	else
+		rc = down_read_trylock(&chip->open_lock);
+
+	if (!rc) {
+		dev_dbg(&chip->dev, "Another process owns this TPM\n");
+		return -EBUSY;
+	}
+
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (priv == NULL)
 		goto out;
+	priv->exclusive = (file->f_flags & O_EXCL);
 
 	tpm_common_open(file, chip, priv, NULL);
 
@@ -40,6 +61,10 @@ static int tpm_open(struct inode *inode, struct file *file)
 
  out:
 	clear_bit(0, &chip->is_open);
+	if (file->f_flags & O_EXCL)
+		up_write(&chip->open_lock);
+	else
+		up_read(&chip->open_lock);
 	return -ENOMEM;
 }
 
@@ -52,6 +77,10 @@ static int tpm_release(struct inode *inode, struct file *file)
 
 	tpm_common_release(file, priv);
 	clear_bit(0, &priv->chip->is_open);
+	if (priv->exclusive)
+		up_write(&priv->chip->open_lock);
+	else
+		up_read(&priv->chip->open_lock);
 	kfree(priv);
 
 	return 0;
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index f3742bcc73e3..0ad8504c73e4 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -17,6 +17,7 @@ struct file_priv {
 	ssize_t response_length;
 	bool response_read;
 	bool command_enqueued;
+	bool exclusive;
 
 	u8 data_buffer[TPM_BUFSIZE];
 };
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index dc0338a783f3..3bd3da5cb97e 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -22,6 +22,7 @@
 #include <linux/cdev.h>
 #include <linux/fs.h>
 #include <linux/highmem.h>
+#include <linux/rwsem.h>
 #include <crypto/hash_info.h>
 #include <crypto/aes.h>
 
@@ -168,7 +169,8 @@ struct tpm_chip {
 	unsigned int flags;
 
 	int dev_num;		/* /dev/tpm# */
-	unsigned long is_open;	/* only one allowed */
+	unsigned long is_open;  /* only one tpm# allowed */
+	struct rw_semaphore open_lock;
 
 	char hwrng_name[64];
 	struct hwrng hwrng;
-- 
2.51.0

[PATCH v3 3/4] tpm: Include /dev/tpmrm<n> when checking exclusive userspace TPM access

[Jonathan McDowell]

From: Jonathan McDowell <noodles@meta.com>

The locking on /dev/tpm<n> that dates back to at least 2013, but it only
prevents multiple accesses via *that* interface. It is perfectly
possible for userspace to use /dev/tpmrm<n>, or the kernel to use the
internal interfaces, to access the TPM. For tooling expecting exclusive
access, such as firmware updates, this can cause issues.

Close the userspace loophole by including /dev/tpmrm<n> in the
read/write mutex, as a reader. That way it gets factored in when a
client wants exclusive access to /dev/tpm<n> but otherwise operates as
usual.

Signed-off-by: Jonathan McDowell <noodles@meta.com>
---
v2: Change error path label to err instead of out. Rework commit
    message.
v3: Rework based on O_EXCL patch being moved earlier in the series.


 drivers/char/tpm/tpmrm-dev.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpmrm-dev.c b/drivers/char/tpm/tpmrm-dev.c
index c25df7ea064e..780a17454088 100644
--- a/drivers/char/tpm/tpmrm-dev.c
+++ b/drivers/char/tpm/tpmrm-dev.c
@@ -17,19 +17,34 @@ static int tpmrm_open(struct inode *inode, struct file *file)
 	int rc;
 
 	chip = container_of(inode->i_cdev, struct tpm_chip, cdevs);
+
+	/*
+	 * A client can choose to have exclusive access to the TPM by opening
+	 * /dev/tpm0 with O_EXCL set, in which case we treat it as a write
+	 * lock. All other opens get treated as a read lock.
+	 */
+	if (!down_read_trylock(&chip->open_lock)) {
+		dev_dbg(&chip->dev, "Another process owns this TPM\n");
+		return -EBUSY;
+	}
+
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (priv == NULL)
-		return -ENOMEM;
+		goto err;
 
 	rc = tpm2_init_space(&priv->space, TPM2_SPACE_BUFFER_SIZE);
 	if (rc) {
 		kfree(priv);
-		return -ENOMEM;
+		goto err;
 	}
 
 	tpm_common_open(file, chip, &priv->priv, &priv->space);
 
 	return 0;
+
+err:
+	up_read(&chip->open_lock);
+	return -ENOMEM;
 }
 
 static int tpmrm_release(struct inode *inode, struct file *file)
@@ -40,6 +55,7 @@ static int tpmrm_release(struct inode *inode, struct file *file)
 	tpm_common_release(file, fpriv);
 	tpm2_del_space(fpriv->chip, &priv->space);
 	kfree(priv);
+	up_read(&fpriv->chip->open_lock);
 
 	return 0;
 }
-- 
2.51.0

[PATCH v3 4/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>

[Jonathan McDowell]

From: Jonathan McDowell <noodles@meta.com>

There are situations where userspace might reasonably desire exclusive
access to the TPM, or the kernel's internal context saving + flushing
may cause issues, for example when performing firmware upgrades. Extend
the locking already used for avoiding concurrent userspace access to
prevent internal users of the TPM when /dev/tpm<n> is in use.

The few internal users who already hold the open_lock are changed to use
tpm_internal_(try_get|put)_ops, with the old tpm_(try_get|put)_ops
functions changing to obtain read access to the open_lock.  We return
-EBUSY when another user has exclusive access, rather than adding waits.

Signed-off-by: Jonathan McDowell <noodles@meta.com>
---
v2: Switch to _locked instead of _internal_ for function names.
v3: Move to end of patch series.

 drivers/char/tpm/tpm-chip.c       | 53 +++++++++++++++++++++++++------
 drivers/char/tpm/tpm-dev-common.c |  8 ++---
 drivers/char/tpm/tpm.h            |  2 ++
 drivers/char/tpm/tpm2-space.c     |  5 ++-
 4 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index ba906966721a..687f6d8cd601 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -144,7 +144,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
 EXPORT_SYMBOL_GPL(tpm_chip_stop);
 
 /**
- * tpm_try_get_ops() - Get a ref to the tpm_chip
+ * tpm_try_get_ops_locked() - Get a ref to the tpm_chip
  * @chip: Chip to ref
  *
  * The caller must already have some kind of locking to ensure that chip is
@@ -154,7 +154,7 @@ EXPORT_SYMBOL_GPL(tpm_chip_stop);
  *
  * Returns -ERRNO if the chip could not be got.
  */
-int tpm_try_get_ops(struct tpm_chip *chip)
+int tpm_try_get_ops_locked(struct tpm_chip *chip)
 {
 	int rc = -EIO;
 
@@ -185,22 +185,57 @@ int tpm_try_get_ops(struct tpm_chip *chip)
 	put_device(&chip->dev);
 	return rc;
 }
-EXPORT_SYMBOL_GPL(tpm_try_get_ops);
 
 /**
- * tpm_put_ops() - Release a ref to the tpm_chip
+ * tpm_put_ops_locked() - Release a ref to the tpm_chip
  * @chip: Chip to put
  *
- * This is the opposite pair to tpm_try_get_ops(). After this returns chip may
- * be kfree'd.
+ * This is the opposite pair to tpm_try_get_ops_locked(). After this returns
+ * chip may be kfree'd.
  */
-void tpm_put_ops(struct tpm_chip *chip)
+void tpm_put_ops_locked(struct tpm_chip *chip)
 {
 	tpm_chip_stop(chip);
 	mutex_unlock(&chip->tpm_mutex);
 	up_read(&chip->ops_sem);
 	put_device(&chip->dev);
 }
+
+/**
+ * tpm_try_get_ops() - Get a ref to the tpm_chip
+ * @chip: Chip to ref
+ *
+ * The caller must already have some kind of locking to ensure that chip is
+ * valid. This function will attempt to get the open_lock for the chip,
+ * ensuring no other user is expecting exclusive access, before locking the
+ * chip so that the ops member can be accessed safely. The locking prevents
+ * tpm_chip_unregister from completing, so it should not be held for long
+ * periods.
+ *
+ * Returns -ERRNO if the chip could not be got.
+ */
+int tpm_try_get_ops(struct tpm_chip *chip)
+{
+	if (!down_read_trylock(&chip->open_lock))
+		return -EBUSY;
+
+	return tpm_try_get_ops_locked(chip);
+}
+EXPORT_SYMBOL_GPL(tpm_try_get_ops);
+
+/**
+ * tpm_put_ops() - Release a ref to the tpm_chip
+ * @chip: Chip to put
+ *
+ * This is the opposite pair to tpm_try_get_ops(). After this returns
+ * chip may be kfree'd.
+ */
+void tpm_put_ops(struct tpm_chip *chip)
+{
+	tpm_put_ops_locked(chip);
+
+	up_read(&chip->open_lock);
+}
 EXPORT_SYMBOL_GPL(tpm_put_ops);
 
 /**
@@ -644,10 +679,10 @@ void tpm_chip_unregister(struct tpm_chip *chip)
 #ifdef CONFIG_TCG_TPM2_HMAC
 	int rc;
 
-	rc = tpm_try_get_ops(chip);
+	rc = tpm_try_get_ops_locked(chip);
 	if (!rc) {
 		tpm2_end_auth_session(chip);
-		tpm_put_ops(chip);
+		tpm_put_ops_locked(chip);
 	}
 #endif
 
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index f2a5e09257dd..0f5bc63411aa 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -65,7 +65,7 @@ static void tpm_dev_async_work(struct work_struct *work)
 
 	mutex_lock(&priv->buffer_mutex);
 	priv->command_enqueued = false;
-	ret = tpm_try_get_ops(priv->chip);
+	ret = tpm_try_get_ops_locked(priv->chip);
 	if (ret) {
 		priv->response_length = ret;
 		goto out;
@@ -73,7 +73,7 @@ static void tpm_dev_async_work(struct work_struct *work)
 
 	ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
 			       sizeof(priv->data_buffer));
-	tpm_put_ops(priv->chip);
+	tpm_put_ops_locked(priv->chip);
 
 	/*
 	 * If ret is > 0 then tpm_dev_transmit returned the size of the
@@ -220,14 +220,14 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
 	 * lock during this period so that the tpm can be unregistered even if
 	 * the char dev is held open.
 	 */
-	if (tpm_try_get_ops(priv->chip)) {
+	if (tpm_try_get_ops_locked(priv->chip)) {
 		ret = -EPIPE;
 		goto out;
 	}
 
 	ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
 			       sizeof(priv->data_buffer));
-	tpm_put_ops(priv->chip);
+	tpm_put_ops_locked(priv->chip);
 
 	if (ret > 0) {
 		priv->response_length = ret;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 02c07fef41ba..57ef8589f5f5 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -272,6 +272,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
 				const struct tpm_class_ops *ops);
 struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
 				 const struct tpm_class_ops *ops);
+int tpm_try_get_ops_locked(struct tpm_chip *chip);
+void tpm_put_ops_locked(struct tpm_chip *chip);
 int tpm_chip_register(struct tpm_chip *chip);
 void tpm_chip_unregister(struct tpm_chip *chip);
 
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 60354cd53b5c..0ad5e18355e0 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -58,10 +58,9 @@ int tpm2_init_space(struct tpm_space *space, unsigned int buf_size)
 
 void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
 {
-
-	if (tpm_try_get_ops(chip) == 0) {
+	if (tpm_try_get_ops_locked(chip) == 0) {
 		tpm2_flush_sessions(chip, space);
-		tpm_put_ops(chip);
+		tpm_put_ops_locked(chip);
 	}
 
 	kfree(space->context_buf);
-- 
2.51.0

Re: [PATCH v3 4/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>

[Roberto Sassu]

On Mon, 2025-10-20 at 12:31 +0100, Jonathan McDowell wrote:
> From: Jonathan McDowell <noodles@meta.com>
> 
> There are situations where userspace might reasonably desire exclusive
> access to the TPM, or the kernel's internal context saving + flushing
> may cause issues, for example when performing firmware upgrades. Extend
> the locking already used for avoiding concurrent userspace access to
> prevent internal users of the TPM when /dev/tpm<n> is in use.
> 
> The few internal users who already hold the open_lock are changed to use
> tpm_internal_(try_get|put)_ops, with the old tpm_(try_get|put)_ops
> functions changing to obtain read access to the open_lock.  We return
> -EBUSY when another user has exclusive access, rather than adding waits.
> 
> Signed-off-by: Jonathan McDowell <noodles@meta.com>
> ---
> v2: Switch to _locked instead of _internal_ for function names.
> v3: Move to end of patch series.
> 
>  drivers/char/tpm/tpm-chip.c       | 53 +++++++++++++++++++++++++------
>  drivers/char/tpm/tpm-dev-common.c |  8 ++---
>  drivers/char/tpm/tpm.h            |  2 ++
>  drivers/char/tpm/tpm2-space.c     |  5 ++-
>  4 files changed, 52 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index ba906966721a..687f6d8cd601 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -144,7 +144,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
>  EXPORT_SYMBOL_GPL(tpm_chip_stop);
>  
>  /**
> - * tpm_try_get_ops() - Get a ref to the tpm_chip
> + * tpm_try_get_ops_locked() - Get a ref to the tpm_chip
>   * @chip: Chip to ref
>   *
>   * The caller must already have some kind of locking to ensure that chip is
> @@ -154,7 +154,7 @@ EXPORT_SYMBOL_GPL(tpm_chip_stop);
>   *
>   * Returns -ERRNO if the chip could not be got.
>   */
> -int tpm_try_get_ops(struct tpm_chip *chip)
> +int tpm_try_get_ops_locked(struct tpm_chip *chip)
>  {
>  	int rc = -EIO;
>  
> @@ -185,22 +185,57 @@ int tpm_try_get_ops(struct tpm_chip *chip)
>  	put_device(&chip->dev);
>  	return rc;
>  }
> -EXPORT_SYMBOL_GPL(tpm_try_get_ops);
>  
>  /**
> - * tpm_put_ops() - Release a ref to the tpm_chip
> + * tpm_put_ops_locked() - Release a ref to the tpm_chip
>   * @chip: Chip to put
>   *
> - * This is the opposite pair to tpm_try_get_ops(). After this returns chip may
> - * be kfree'd.
> + * This is the opposite pair to tpm_try_get_ops_locked(). After this returns
> + * chip may be kfree'd.
>   */
> -void tpm_put_ops(struct tpm_chip *chip)
> +void tpm_put_ops_locked(struct tpm_chip *chip)
>  {
>  	tpm_chip_stop(chip);
>  	mutex_unlock(&chip->tpm_mutex);
>  	up_read(&chip->ops_sem);
>  	put_device(&chip->dev);
>  }
> +
> +/**
> + * tpm_try_get_ops() - Get a ref to the tpm_chip
> + * @chip: Chip to ref
> + *
> + * The caller must already have some kind of locking to ensure that chip is
> + * valid. This function will attempt to get the open_lock for the chip,
> + * ensuring no other user is expecting exclusive access, before locking the
> + * chip so that the ops member can be accessed safely. The locking prevents
> + * tpm_chip_unregister from completing, so it should not be held for long
> + * periods.
> + *
> + * Returns -ERRNO if the chip could not be got.
> + */
> +int tpm_try_get_ops(struct tpm_chip *chip)
> +{
> +	if (!down_read_trylock(&chip->open_lock))
> +		return -EBUSY;

Hi Jonathan

do I understand it correctly, that a process might open the TPM with
O_EXCL, and this will prevent IMA from extending a PCR until that
process closes the file descriptor?

If yes, this might be a concern, and I think an additional API to
prevent such behavior would be needed (for example when IMA is active,
i.e. there is a measurement policy loaded).

Thanks

Roberto

> +
> +	return tpm_try_get_ops_locked(chip);
> +}
> +EXPORT_SYMBOL_GPL(tpm_try_get_ops);
> +
> +/**
> + * tpm_put_ops() - Release a ref to the tpm_chip
> + * @chip: Chip to put
> + *
> + * This is the opposite pair to tpm_try_get_ops(). After this returns
> + * chip may be kfree'd.
> + */
> +void tpm_put_ops(struct tpm_chip *chip)
> +{
> +	tpm_put_ops_locked(chip);
> +
> +	up_read(&chip->open_lock);
> +}
>  EXPORT_SYMBOL_GPL(tpm_put_ops);
>  
>  /**
> @@ -644,10 +679,10 @@ void tpm_chip_unregister(struct tpm_chip *chip)
>  #ifdef CONFIG_TCG_TPM2_HMAC
>  	int rc;
>  
> -	rc = tpm_try_get_ops(chip);
> +	rc = tpm_try_get_ops_locked(chip);
>  	if (!rc) {
>  		tpm2_end_auth_session(chip);
> -		tpm_put_ops(chip);
> +		tpm_put_ops_locked(chip);
>  	}
>  #endif
>  
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index f2a5e09257dd..0f5bc63411aa 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -65,7 +65,7 @@ static void tpm_dev_async_work(struct work_struct *work)
>  
>  	mutex_lock(&priv->buffer_mutex);
>  	priv->command_enqueued = false;
> -	ret = tpm_try_get_ops(priv->chip);
> +	ret = tpm_try_get_ops_locked(priv->chip);
>  	if (ret) {
>  		priv->response_length = ret;
>  		goto out;
> @@ -73,7 +73,7 @@ static void tpm_dev_async_work(struct work_struct *work)
>  
>  	ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
>  			       sizeof(priv->data_buffer));
> -	tpm_put_ops(priv->chip);
> +	tpm_put_ops_locked(priv->chip);
>  
>  	/*
>  	 * If ret is > 0 then tpm_dev_transmit returned the size of the
> @@ -220,14 +220,14 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
>  	 * lock during this period so that the tpm can be unregistered even if
>  	 * the char dev is held open.
>  	 */
> -	if (tpm_try_get_ops(priv->chip)) {
> +	if (tpm_try_get_ops_locked(priv->chip)) {
>  		ret = -EPIPE;
>  		goto out;
>  	}
>  
>  	ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
>  			       sizeof(priv->data_buffer));
> -	tpm_put_ops(priv->chip);
> +	tpm_put_ops_locked(priv->chip);
>  
>  	if (ret > 0) {
>  		priv->response_length = ret;
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 02c07fef41ba..57ef8589f5f5 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -272,6 +272,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
>  				const struct tpm_class_ops *ops);
>  struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
>  				 const struct tpm_class_ops *ops);
> +int tpm_try_get_ops_locked(struct tpm_chip *chip);
> +void tpm_put_ops_locked(struct tpm_chip *chip);
>  int tpm_chip_register(struct tpm_chip *chip);
>  void tpm_chip_unregister(struct tpm_chip *chip);
>  
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index 60354cd53b5c..0ad5e18355e0 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -58,10 +58,9 @@ int tpm2_init_space(struct tpm_space *space, unsigned int buf_size)
>  
>  void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
>  {
> -
> -	if (tpm_try_get_ops(chip) == 0) {
> +	if (tpm_try_get_ops_locked(chip) == 0) {
>  		tpm2_flush_sessions(chip, space);
> -		tpm_put_ops(chip);
> +		tpm_put_ops_locked(chip);
>  	}
>  
>  	kfree(space->context_buf);

Re: [PATCH v3 4/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>

[Jonathan McDowell]

On Mon, Oct 20, 2025 at 01:53:30PM +0200, Roberto Sassu wrote:
>On Mon, 2025-10-20 at 12:31 +0100, Jonathan McDowell wrote:
>> From: Jonathan McDowell <noodles@meta.com>
>>
>> There are situations where userspace might reasonably desire exclusive
>> access to the TPM, or the kernel's internal context saving + flushing
>> may cause issues, for example when performing firmware upgrades. Extend
>> the locking already used for avoiding concurrent userspace access to
>> prevent internal users of the TPM when /dev/tpm<n> is in use.
>>
>> The few internal users who already hold the open_lock are changed to use
>> tpm_internal_(try_get|put)_ops, with the old tpm_(try_get|put)_ops
>> functions changing to obtain read access to the open_lock.  We return
>> -EBUSY when another user has exclusive access, rather than adding waits.
>>
>> Signed-off-by: Jonathan McDowell <noodles@meta.com>
>> ---
>> v2: Switch to _locked instead of _internal_ for function names.
>> v3: Move to end of patch series.
>>
>>  drivers/char/tpm/tpm-chip.c       | 53 +++++++++++++++++++++++++------
>>  drivers/char/tpm/tpm-dev-common.c |  8 ++---
>>  drivers/char/tpm/tpm.h            |  2 ++
>>  drivers/char/tpm/tpm2-space.c     |  5 ++-
>>  4 files changed, 52 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index ba906966721a..687f6d8cd601 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -144,7 +144,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
>>  EXPORT_SYMBOL_GPL(tpm_chip_stop);
>>
>>  /**
>> - * tpm_try_get_ops() - Get a ref to the tpm_chip
>> + * tpm_try_get_ops_locked() - Get a ref to the tpm_chip
>>   * @chip: Chip to ref
>>   *
>>   * The caller must already have some kind of locking to ensure that chip is
>> @@ -154,7 +154,7 @@ EXPORT_SYMBOL_GPL(tpm_chip_stop);
>>   *
>>   * Returns -ERRNO if the chip could not be got.
>>   */
>> -int tpm_try_get_ops(struct tpm_chip *chip)
>> +int tpm_try_get_ops_locked(struct tpm_chip *chip)
>>  {
>>  	int rc = -EIO;
>>
>> @@ -185,22 +185,57 @@ int tpm_try_get_ops(struct tpm_chip *chip)
>>  	put_device(&chip->dev);
>>  	return rc;
>>  }
>> -EXPORT_SYMBOL_GPL(tpm_try_get_ops);
>>
>>  /**
>> - * tpm_put_ops() - Release a ref to the tpm_chip
>> + * tpm_put_ops_locked() - Release a ref to the tpm_chip
>>   * @chip: Chip to put
>>   *
>> - * This is the opposite pair to tpm_try_get_ops(). After this returns chip may
>> - * be kfree'd.
>> + * This is the opposite pair to tpm_try_get_ops_locked(). After this returns
>> + * chip may be kfree'd.
>>   */
>> -void tpm_put_ops(struct tpm_chip *chip)
>> +void tpm_put_ops_locked(struct tpm_chip *chip)
>>  {
>>  	tpm_chip_stop(chip);
>>  	mutex_unlock(&chip->tpm_mutex);
>>  	up_read(&chip->ops_sem);
>>  	put_device(&chip->dev);
>>  }
>> +
>> +/**
>> + * tpm_try_get_ops() - Get a ref to the tpm_chip
>> + * @chip: Chip to ref
>> + *
>> + * The caller must already have some kind of locking to ensure that chip is
>> + * valid. This function will attempt to get the open_lock for the chip,
>> + * ensuring no other user is expecting exclusive access, before locking the
>> + * chip so that the ops member can be accessed safely. The locking prevents
>> + * tpm_chip_unregister from completing, so it should not be held for long
>> + * periods.
>> + *
>> + * Returns -ERRNO if the chip could not be got.
>> + */
>> +int tpm_try_get_ops(struct tpm_chip *chip)
>> +{
>> +	if (!down_read_trylock(&chip->open_lock))
>> +		return -EBUSY;
>
>Hi Jonathan
>
>do I understand it correctly, that a process might open the TPM with
>O_EXCL, and this will prevent IMA from extending a PCR until that
>process closes the file descriptor?
>
>If yes, this might be a concern, and I think an additional API to
>prevent such behavior would be needed (for example when IMA is active,
>i.e. there is a measurement policy loaded).

Yes, this definitely provides a path where userspace could prevent a 
measurement hitting the TPM from IMA. I did think about this when 
working out what to do if the lock was held elsewhere, but punted on 
making any changes because there are several other avenues where that 
can already happen.

>> +
>> +	return tpm_try_get_ops_locked(chip);
>> +}
>> +EXPORT_SYMBOL_GPL(tpm_try_get_ops);
>> +
>> +/**
>> + * tpm_put_ops() - Release a ref to the tpm_chip
>> + * @chip: Chip to put
>> + *
>> + * This is the opposite pair to tpm_try_get_ops(). After this returns
>> + * chip may be kfree'd.
>> + */
>> +void tpm_put_ops(struct tpm_chip *chip)
>> +{
>> +	tpm_put_ops_locked(chip);
>> +
>> +	up_read(&chip->open_lock);
>> +}
>>  EXPORT_SYMBOL_GPL(tpm_put_ops);
>>
>>  /**
>> @@ -644,10 +679,10 @@ void tpm_chip_unregister(struct tpm_chip *chip)
>>  #ifdef CONFIG_TCG_TPM2_HMAC
>>  	int rc;
>>
>> -	rc = tpm_try_get_ops(chip);
>> +	rc = tpm_try_get_ops_locked(chip);
>>  	if (!rc) {
>>  		tpm2_end_auth_session(chip);
>> -		tpm_put_ops(chip);
>> +		tpm_put_ops_locked(chip);
>>  	}
>>  #endif
>>
>> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
>> index f2a5e09257dd..0f5bc63411aa 100644
>> --- a/drivers/char/tpm/tpm-dev-common.c
>> +++ b/drivers/char/tpm/tpm-dev-common.c
>> @@ -65,7 +65,7 @@ static void tpm_dev_async_work(struct work_struct *work)
>>
>>  	mutex_lock(&priv->buffer_mutex);
>>  	priv->command_enqueued = false;
>> -	ret = tpm_try_get_ops(priv->chip);
>> +	ret = tpm_try_get_ops_locked(priv->chip);
>>  	if (ret) {
>>  		priv->response_length = ret;
>>  		goto out;
>> @@ -73,7 +73,7 @@ static void tpm_dev_async_work(struct work_struct *work)
>>
>>  	ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
>>  			       sizeof(priv->data_buffer));
>> -	tpm_put_ops(priv->chip);
>> +	tpm_put_ops_locked(priv->chip);
>>
>>  	/*
>>  	 * If ret is > 0 then tpm_dev_transmit returned the size of the
>> @@ -220,14 +220,14 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
>>  	 * lock during this period so that the tpm can be unregistered even if
>>  	 * the char dev is held open.
>>  	 */
>> -	if (tpm_try_get_ops(priv->chip)) {
>> +	if (tpm_try_get_ops_locked(priv->chip)) {
>>  		ret = -EPIPE;
>>  		goto out;
>>  	}
>>
>>  	ret = tpm_dev_transmit(priv->chip, priv->space, priv->data_buffer,
>>  			       sizeof(priv->data_buffer));
>> -	tpm_put_ops(priv->chip);
>> +	tpm_put_ops_locked(priv->chip);
>>
>>  	if (ret > 0) {
>>  		priv->response_length = ret;
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index 02c07fef41ba..57ef8589f5f5 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -272,6 +272,8 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
>>  				const struct tpm_class_ops *ops);
>>  struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
>>  				 const struct tpm_class_ops *ops);
>> +int tpm_try_get_ops_locked(struct tpm_chip *chip);
>> +void tpm_put_ops_locked(struct tpm_chip *chip);
>>  int tpm_chip_register(struct tpm_chip *chip);
>>  void tpm_chip_unregister(struct tpm_chip *chip);
>>
>> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
>> index 60354cd53b5c..0ad5e18355e0 100644
>> --- a/drivers/char/tpm/tpm2-space.c
>> +++ b/drivers/char/tpm/tpm2-space.c
>> @@ -58,10 +58,9 @@ int tpm2_init_space(struct tpm_space *space, unsigned int buf_size)
>>
>>  void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)
>>  {
>> -
>> -	if (tpm_try_get_ops(chip) == 0) {
>> +	if (tpm_try_get_ops_locked(chip) == 0) {
>>  		tpm2_flush_sessions(chip, space);
>> -		tpm_put_ops(chip);
>> +		tpm_put_ops_locked(chip);
>>  	}
>>
>>  	kfree(space->context_buf);
>

J.

-- 
"I'm a compsci. I don't write code." -- Noodles.  "I'm a DB coder.
Neither do I." -- Adrian.

Re: [PATCH v3 0/4] pm: Ensure exclusive userspace access when using /dev/tpm<n>

[Jarkko Sakkinen]

On Mon, Oct 20, 2025 at 12:30:32PM +0100, Jonathan McDowell wrote:
> I hit a problem where ~ 1% of TPM firmware upgrades were failing.
> Investigating revealed the issue was that although the upgrade tool uses
> /dev/tpm0 this does not actually prevent access via /dev/tpmrm0, nor
> internal kernel users. It *does* prevent access to others via /dev/tpm0
> 
> So the upgrade process started, the HW RNG came in to get some
> randomness in the middle, did the HMAC context dance, and confused
> everything to the point the TPM was no longer visible to the OS even
> after a reboot.
> 
> Thankfully I've been able to recover those devices, but really what I'd
> like is the ability for a userspace tool to exclusively access the TPM
> without something coming in behind it. Given the lightweight attempt at
> locking that already exists I think this was the original intention.
> 
> I've reworked this series based on feedback received.
> 
> Firstly, it's been reordered TPM sharing functionality doesn't break
> during bisection.
> 
> Secondly, the O_EXCL check has been tightend up to ensure the caller is
> also opening the device O_RDWR. Callers shouldn't really be opening the
> TPM except for reading + writing, but this should help guard against
> unexpected flags usage a bit more.
> 
> Finally, this revision keeps the prohibition on more than one user of
> /dev/tpm#, to avoid unexpected breakages for clients that expect this to
> guard against multiple invocations. A client only then needs to use
> O_EXCL if it wants to prevent *all* other access, even with
> ContextSaves, such as the firmware upgrade case.
> 
> (I've sent a separate standalone patch that allows the TPM HW RNG to be
> disabled at run time, and it's now in -next, but even with that I think
> something like this is a good idea as well.)
> 
> Jonathan McDowell (4):
>   tpm: Remove tpm_find_get_ops
>   tpm: Add O_EXCL for exclusive /dev/tpm access
>   tpm: Include /dev/tpmrm<n> when checking exclusive userspace TPM
>     access
>   tpm: Allow for exclusive TPM access when using /dev/tpm<n>
> 
>  drivers/char/tpm/tpm-chip.c       | 90 +++++++++++++++----------------
>  drivers/char/tpm/tpm-dev-common.c |  8 +--
>  drivers/char/tpm/tpm-dev.c        | 35 ++++++++++--
>  drivers/char/tpm/tpm-dev.h        |  1 +
>  drivers/char/tpm/tpm-interface.c  | 20 +++++--
>  drivers/char/tpm/tpm.h            |  3 +-
>  drivers/char/tpm/tpm2-space.c     |  5 +-
>  drivers/char/tpm/tpm_tis_core.c   |  3 +-
>  drivers/char/tpm/tpmrm-dev.c      | 20 ++++++-
>  include/linux/tpm.h               |  4 +-
>  10 files changed, 124 insertions(+), 65 deletions(-)
> 
> -- 
> 2.51.0
> 

I will put to queue with my tags but I just want to make first sure that we do not
break anything.

I'll upgrade my test suite first to have TPM 1.2 tests (which is also
needed for my own series) and run it in bunch of configurations.  And on
TPM2 I check the behavior with TSS2 daemon on / off.

I have no doubts on the code changes, and it is most importantly a
security improvement, given that "who has the access and how long" 
can be deduced for a system configuration. I just feel that with
this code change it is better to check and verify everything :-)

BR, Jarkko

Re: [PATCH v3 0/4] pm: Ensure exclusive userspace access when using /dev/tpm<n>

[Mimi Zohar]

On Fri, 2025-10-24 at 21:55 +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 20, 2025 at 12:30:32PM +0100, Jonathan McDowell wrote:
> > I hit a problem where ~ 1% of TPM firmware upgrades were failing.
> > Investigating revealed the issue was that although the upgrade tool uses
> > /dev/tpm0 this does not actually prevent access via /dev/tpmrm0, nor
> > internal kernel users. It *does* prevent access to others via /dev/tpm0
> > 
> > So the upgrade process started, the HW RNG came in to get some
> > randomness in the middle, did the HMAC context dance, and confused
> > everything to the point the TPM was no longer visible to the OS even
> > after a reboot.
> > 
> > Thankfully I've been able to recover those devices, but really what I'd
> > like is the ability for a userspace tool to exclusively access the TPM
> > without something coming in behind it. Given the lightweight attempt at
> > locking that already exists I think this was the original intention.
> > 
> > I've reworked this series based on feedback received.
> > 
> > Firstly, it's been reordered TPM sharing functionality doesn't break
> > during bisection.
> > 
> > Secondly, the O_EXCL check has been tightend up to ensure the caller is
> > also opening the device O_RDWR. Callers shouldn't really be opening the
> > TPM except for reading + writing, but this should help guard against
> > unexpected flags usage a bit more.
> > 
> > Finally, this revision keeps the prohibition on more than one user of
> > /dev/tpm#, to avoid unexpected breakages for clients that expect this to
> > guard against multiple invocations. A client only then needs to use
> > O_EXCL if it wants to prevent *all* other access, even with
> > ContextSaves, such as the firmware upgrade case.
> > 
> > (I've sent a separate standalone patch that allows the TPM HW RNG to be
> > disabled at run time, and it's now in -next, but even with that I think
> > something like this is a good idea as well.)
> > 
> > Jonathan McDowell (4):
> >   tpm: Remove tpm_find_get_ops
> >   tpm: Add O_EXCL for exclusive /dev/tpm access
> >   tpm: Include /dev/tpmrm<n> when checking exclusive userspace TPM
> >     access
> >   tpm: Allow for exclusive TPM access when using /dev/tpm<n>
> > 
> >  drivers/char/tpm/tpm-chip.c       | 90 +++++++++++++++----------------
> >  drivers/char/tpm/tpm-dev-common.c |  8 +--
> >  drivers/char/tpm/tpm-dev.c        | 35 ++++++++++--
> >  drivers/char/tpm/tpm-dev.h        |  1 +
> >  drivers/char/tpm/tpm-interface.c  | 20 +++++--
> >  drivers/char/tpm/tpm.h            |  3 +-
> >  drivers/char/tpm/tpm2-space.c     |  5 +-
> >  drivers/char/tpm/tpm_tis_core.c   |  3 +-
> >  drivers/char/tpm/tpmrm-dev.c      | 20 ++++++-
> >  include/linux/tpm.h               |  4 +-
> >  10 files changed, 124 insertions(+), 65 deletions(-)
> > 
> > -- 
> > 2.51.0
> > 
> 
> I will put to queue with my tags but I just want to make first sure that we do not
> break anything.
> 
> I'll upgrade my test suite first to have TPM 1.2 tests (which is also
> needed for my own series) and run it in bunch of configurations.  And on
> TPM2 I check the behavior with TSS2 daemon on / off.
> 
> I have no doubts on the code changes, and it is most importantly a
> security improvement, given that "who has the access and how long" 
> can be deduced for a system configuration. I just feel that with
> this code change it is better to check and verify everything :-)

Roberto has already commented on this patch set, saying it would affect IMA[1].
I still need to look at the patch set, but please don't break IMA.

[1]https://lore.kernel.org/linux-integrity/cec499d5130f37a7887d39b44efd8538dd361fe3.camel@huaweicloud.com/

-- 
thanks,

Mimi

Re: [PATCH v3 4/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>

[Jarkko Sakkinen]

On Mon, Oct 20, 2025 at 01:53:30PM +0200, Roberto Sassu wrote:
> On Mon, 2025-10-20 at 12:31 +0100, Jonathan McDowell wrote:
> > From: Jonathan McDowell <noodles@meta.com>
> > 
> > There are situations where userspace might reasonably desire exclusive
> > access to the TPM, or the kernel's internal context saving + flushing
> > may cause issues, for example when performing firmware upgrades. Extend
> > the locking already used for avoiding concurrent userspace access to
> > prevent internal users of the TPM when /dev/tpm<n> is in use.
> > 
> > The few internal users who already hold the open_lock are changed to use
> > tpm_internal_(try_get|put)_ops, with the old tpm_(try_get|put)_ops
> > functions changing to obtain read access to the open_lock.  We return
> > -EBUSY when another user has exclusive access, rather than adding waits.
> > 
> > Signed-off-by: Jonathan McDowell <noodles@meta.com>
> > ---
> > v2: Switch to _locked instead of _internal_ for function names.
> > v3: Move to end of patch series.
> > 
> >  drivers/char/tpm/tpm-chip.c       | 53 +++++++++++++++++++++++++------
> >  drivers/char/tpm/tpm-dev-common.c |  8 ++---
> >  drivers/char/tpm/tpm.h            |  2 ++
> >  drivers/char/tpm/tpm2-space.c     |  5 ++-
> >  4 files changed, 52 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index ba906966721a..687f6d8cd601 100644
> > --- a/drivers/char/tpm/tpm-chip.c
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -144,7 +144,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
> >  EXPORT_SYMBOL_GPL(tpm_chip_stop);
> >  
> >  /**
> > - * tpm_try_get_ops() - Get a ref to the tpm_chip
> > + * tpm_try_get_ops_locked() - Get a ref to the tpm_chip
> >   * @chip: Chip to ref
> >   *
> >   * The caller must already have some kind of locking to ensure that chip is
> > @@ -154,7 +154,7 @@ EXPORT_SYMBOL_GPL(tpm_chip_stop);
> >   *
> >   * Returns -ERRNO if the chip could not be got.
> >   */
> > -int tpm_try_get_ops(struct tpm_chip *chip)
> > +int tpm_try_get_ops_locked(struct tpm_chip *chip)
> >  {
> >  	int rc = -EIO;
> >  
> > @@ -185,22 +185,57 @@ int tpm_try_get_ops(struct tpm_chip *chip)
> >  	put_device(&chip->dev);
> >  	return rc;
> >  }
> > -EXPORT_SYMBOL_GPL(tpm_try_get_ops);
> >  
> >  /**
> > - * tpm_put_ops() - Release a ref to the tpm_chip
> > + * tpm_put_ops_locked() - Release a ref to the tpm_chip
> >   * @chip: Chip to put
> >   *
> > - * This is the opposite pair to tpm_try_get_ops(). After this returns chip may
> > - * be kfree'd.
> > + * This is the opposite pair to tpm_try_get_ops_locked(). After this returns
> > + * chip may be kfree'd.
> >   */
> > -void tpm_put_ops(struct tpm_chip *chip)
> > +void tpm_put_ops_locked(struct tpm_chip *chip)
> >  {
> >  	tpm_chip_stop(chip);
> >  	mutex_unlock(&chip->tpm_mutex);
> >  	up_read(&chip->ops_sem);
> >  	put_device(&chip->dev);
> >  }
> > +
> > +/**
> > + * tpm_try_get_ops() - Get a ref to the tpm_chip
> > + * @chip: Chip to ref
> > + *
> > + * The caller must already have some kind of locking to ensure that chip is
> > + * valid. This function will attempt to get the open_lock for the chip,
> > + * ensuring no other user is expecting exclusive access, before locking the
> > + * chip so that the ops member can be accessed safely. The locking prevents
> > + * tpm_chip_unregister from completing, so it should not be held for long
> > + * periods.
> > + *
> > + * Returns -ERRNO if the chip could not be got.
> > + */
> > +int tpm_try_get_ops(struct tpm_chip *chip)
> > +{
> > +	if (!down_read_trylock(&chip->open_lock))
> > +		return -EBUSY;
> 
> Hi Jonathan
> 
> do I understand it correctly, that a process might open the TPM with
> O_EXCL, and this will prevent IMA from extending a PCR until that
> process closes the file descriptor?
> 
> If yes, this might be a concern, and I think an additional API to
> prevent such behavior would be needed (for example when IMA is active,
> i.e. there is a measurement policy loaded).

Also this would be a problem with hwrng.

This probably needs to be refined somehow. I don't have a solution at
hand but "invariant" is that in-kernel caller should override user space
exclusion, even when O_EXCL is used.


> 
> Thanks
> 
> Roberto

BR, Jarkko

Re: [PATCH v3 0/4] pm: Ensure exclusive userspace access when using /dev/tpm<n>

[Jarkko Sakkinen]

On Mon, Oct 27, 2025 at 07:50:46AM -0400, Mimi Zohar wrote:
> On Fri, 2025-10-24 at 21:55 +0300, Jarkko Sakkinen wrote:
> > On Mon, Oct 20, 2025 at 12:30:32PM +0100, Jonathan McDowell wrote:
> > > I hit a problem where ~ 1% of TPM firmware upgrades were failing.
> > > Investigating revealed the issue was that although the upgrade tool uses
> > > /dev/tpm0 this does not actually prevent access via /dev/tpmrm0, nor
> > > internal kernel users. It *does* prevent access to others via /dev/tpm0
> > > 
> > > So the upgrade process started, the HW RNG came in to get some
> > > randomness in the middle, did the HMAC context dance, and confused
> > > everything to the point the TPM was no longer visible to the OS even
> > > after a reboot.
> > > 
> > > Thankfully I've been able to recover those devices, but really what I'd
> > > like is the ability for a userspace tool to exclusively access the TPM
> > > without something coming in behind it. Given the lightweight attempt at
> > > locking that already exists I think this was the original intention.
> > > 
> > > I've reworked this series based on feedback received.
> > > 
> > > Firstly, it's been reordered TPM sharing functionality doesn't break
> > > during bisection.
> > > 
> > > Secondly, the O_EXCL check has been tightend up to ensure the caller is
> > > also opening the device O_RDWR. Callers shouldn't really be opening the
> > > TPM except for reading + writing, but this should help guard against
> > > unexpected flags usage a bit more.
> > > 
> > > Finally, this revision keeps the prohibition on more than one user of
> > > /dev/tpm#, to avoid unexpected breakages for clients that expect this to
> > > guard against multiple invocations. A client only then needs to use
> > > O_EXCL if it wants to prevent *all* other access, even with
> > > ContextSaves, such as the firmware upgrade case.
> > > 
> > > (I've sent a separate standalone patch that allows the TPM HW RNG to be
> > > disabled at run time, and it's now in -next, but even with that I think
> > > something like this is a good idea as well.)
> > > 
> > > Jonathan McDowell (4):
> > >   tpm: Remove tpm_find_get_ops
> > >   tpm: Add O_EXCL for exclusive /dev/tpm access
> > >   tpm: Include /dev/tpmrm<n> when checking exclusive userspace TPM
> > >     access
> > >   tpm: Allow for exclusive TPM access when using /dev/tpm<n>
> > > 
> > >  drivers/char/tpm/tpm-chip.c       | 90 +++++++++++++++----------------
> > >  drivers/char/tpm/tpm-dev-common.c |  8 +--
> > >  drivers/char/tpm/tpm-dev.c        | 35 ++++++++++--
> > >  drivers/char/tpm/tpm-dev.h        |  1 +
> > >  drivers/char/tpm/tpm-interface.c  | 20 +++++--
> > >  drivers/char/tpm/tpm.h            |  3 +-
> > >  drivers/char/tpm/tpm2-space.c     |  5 +-
> > >  drivers/char/tpm/tpm_tis_core.c   |  3 +-
> > >  drivers/char/tpm/tpmrm-dev.c      | 20 ++++++-
> > >  include/linux/tpm.h               |  4 +-
> > >  10 files changed, 124 insertions(+), 65 deletions(-)
> > > 
> > > -- 
> > > 2.51.0
> > > 
> > 
> > I will put to queue with my tags but I just want to make first sure that we do not
> > break anything.
> > 
> > I'll upgrade my test suite first to have TPM 1.2 tests (which is also
> > needed for my own series) and run it in bunch of configurations.  And on
> > TPM2 I check the behavior with TSS2 daemon on / off.
> > 
> > I have no doubts on the code changes, and it is most importantly a
> > security improvement, given that "who has the access and how long" 
> > can be deduced for a system configuration. I just feel that with
> > this code change it is better to check and verify everything :-)
> 
> Roberto has already commented on this patch set, saying it would affect IMA[1].
> I still need to look at the patch set, but please don't break IMA.

See my response in that thread.

> 
> [1]https://lore.kernel.org/linux-integrity/cec499d5130f37a7887d39b44efd8538dd361fe3.camel@huaweicloud.com/
> 
> -- 
> thanks,
> 
> Mimi

BR, Jarkko

Re: [PATCH v3 4/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>

[James Bottomley]

On Mon, 2025-10-27 at 21:38 +0200, Jarkko Sakkinen wrote:
> On Mon, Oct 20, 2025 at 01:53:30PM +0200, Roberto Sassu wrote:
[...]
> > Hi Jonathan
> > 
> > do I understand it correctly, that a process might open the TPM
> > with O_EXCL, and this will prevent IMA from extending a PCR until
> > that process closes the file descriptor?
> > 
> > If yes, this might be a concern, and I think an additional API to
> > prevent such behavior would be needed (for example when IMA is
> > active, i.e. there is a measurement policy loaded).
> 
> Also this would be a problem with hwrng.
> 
> This probably needs to be refined somehow. I don't have a solution at
> hand but "invariant" is that in-kernel caller should override user
> space exclusion, even when O_EXCL is used.

Also, are we sure we need O_EXCL in the first place?  A well
functioning TPM is supposed to be able to cope with field upgrade while
it receives other commands.  When it's in this state, it's supposed to
return TPM_RC_UPGRADE to inappropriate commands, so if we made sure we
can correctly handle that in the kernel, that might be enough to get
all this to work correctly without needing an exclusive open.

Of course, Field Upgrade is likely to be the least well tested of any
TPM capability, so there's a good chance at least one TPM out there
isn't going to behave as the standard says it should.

Regards,

James

Re: [PATCH v3 4/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>

[Jarkko Sakkinen]

On Mon, Oct 27, 2025 at 04:09:35PM -0400, James Bottomley wrote:
> On Mon, 2025-10-27 at 21:38 +0200, Jarkko Sakkinen wrote:
> > On Mon, Oct 20, 2025 at 01:53:30PM +0200, Roberto Sassu wrote:
> [...]
> > > Hi Jonathan
> > > 
> > > do I understand it correctly, that a process might open the TPM
> > > with O_EXCL, and this will prevent IMA from extending a PCR until
> > > that process closes the file descriptor?
> > > 
> > > If yes, this might be a concern, and I think an additional API to
> > > prevent such behavior would be needed (for example when IMA is
> > > active, i.e. there is a measurement policy loaded).
> > 
> > Also this would be a problem with hwrng.
> > 
> > This probably needs to be refined somehow. I don't have a solution at
> > hand but "invariant" is that in-kernel caller should override user
> > space exclusion, even when O_EXCL is used.
> 
> Also, are we sure we need O_EXCL in the first place?  A well
> functioning TPM is supposed to be able to cope with field upgrade while
> it receives other commands.  When it's in this state, it's supposed to
> return TPM_RC_UPGRADE to inappropriate commands, so if we made sure we
> can correctly handle that in the kernel, that might be enough to get
> all this to work correctly without needing an exclusive open.
> 
> Of course, Field Upgrade is likely to be the least well tested of any
> TPM capability, so there's a good chance at least one TPM out there
> isn't going to behave as the standard says it should.
> 
> Regards,
> 
> James

I get that depending on configuration someone really would want to
have guaranteed exclusive access to the device. Since it is opt-in
via O_EXCL, I don't have anything in principle againts adding it.

The patch set needs rework but feature itself is totally fine.

BR, Jarkko

Re: [PATCH v3 4/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>

[Jonathan McDowell]

On Mon, Oct 27, 2025 at 09:38:55PM +0200, Jarkko Sakkinen wrote:
>On Mon, Oct 20, 2025 at 01:53:30PM +0200, Roberto Sassu wrote:
>> On Mon, 2025-10-20 at 12:31 +0100, Jonathan McDowell wrote:
>> > From: Jonathan McDowell <noodles@meta.com>
>> >
>> > There are situations where userspace might reasonably desire exclusive
>> > access to the TPM, or the kernel's internal context saving + flushing
>> > may cause issues, for example when performing firmware upgrades. Extend
>> > the locking already used for avoiding concurrent userspace access to
>> > prevent internal users of the TPM when /dev/tpm<n> is in use.
>> >
>> > The few internal users who already hold the open_lock are changed to use
>> > tpm_internal_(try_get|put)_ops, with the old tpm_(try_get|put)_ops
>> > functions changing to obtain read access to the open_lock.  We return
>> > -EBUSY when another user has exclusive access, rather than adding waits.
>> >
>> > Signed-off-by: Jonathan McDowell <noodles@meta.com>
>> > ---
>> > v2: Switch to _locked instead of _internal_ for function names.
>> > v3: Move to end of patch series.
>> >
>> >  drivers/char/tpm/tpm-chip.c       | 53 +++++++++++++++++++++++++------
>> >  drivers/char/tpm/tpm-dev-common.c |  8 ++---
>> >  drivers/char/tpm/tpm.h            |  2 ++
>> >  drivers/char/tpm/tpm2-space.c     |  5 ++-
>> >  4 files changed, 52 insertions(+), 16 deletions(-)
>> >
>> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> > index ba906966721a..687f6d8cd601 100644
>> > --- a/drivers/char/tpm/tpm-chip.c
>> > +++ b/drivers/char/tpm/tpm-chip.c
>> > @@ -144,7 +144,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
>> >  EXPORT_SYMBOL_GPL(tpm_chip_stop);
>> >
>> >  /**
>> > - * tpm_try_get_ops() - Get a ref to the tpm_chip
>> > + * tpm_try_get_ops_locked() - Get a ref to the tpm_chip
>> >   * @chip: Chip to ref
>> >   *
>> >   * The caller must already have some kind of locking to ensure that chip is
>> > @@ -154,7 +154,7 @@ EXPORT_SYMBOL_GPL(tpm_chip_stop);
>> >   *
>> >   * Returns -ERRNO if the chip could not be got.
>> >   */
>> > -int tpm_try_get_ops(struct tpm_chip *chip)
>> > +int tpm_try_get_ops_locked(struct tpm_chip *chip)
>> >  {
>> >  	int rc = -EIO;
>> >
>> > @@ -185,22 +185,57 @@ int tpm_try_get_ops(struct tpm_chip *chip)
>> >  	put_device(&chip->dev);
>> >  	return rc;
>> >  }
>> > -EXPORT_SYMBOL_GPL(tpm_try_get_ops);
>> >
>> >  /**
>> > - * tpm_put_ops() - Release a ref to the tpm_chip
>> > + * tpm_put_ops_locked() - Release a ref to the tpm_chip
>> >   * @chip: Chip to put
>> >   *
>> > - * This is the opposite pair to tpm_try_get_ops(). After this returns chip may
>> > - * be kfree'd.
>> > + * This is the opposite pair to tpm_try_get_ops_locked(). After this returns
>> > + * chip may be kfree'd.
>> >   */
>> > -void tpm_put_ops(struct tpm_chip *chip)
>> > +void tpm_put_ops_locked(struct tpm_chip *chip)
>> >  {
>> >  	tpm_chip_stop(chip);
>> >  	mutex_unlock(&chip->tpm_mutex);
>> >  	up_read(&chip->ops_sem);
>> >  	put_device(&chip->dev);
>> >  }
>> > +
>> > +/**
>> > + * tpm_try_get_ops() - Get a ref to the tpm_chip
>> > + * @chip: Chip to ref
>> > + *
>> > + * The caller must already have some kind of locking to ensure that chip is
>> > + * valid. This function will attempt to get the open_lock for the chip,
>> > + * ensuring no other user is expecting exclusive access, before locking the
>> > + * chip so that the ops member can be accessed safely. The locking prevents
>> > + * tpm_chip_unregister from completing, so it should not be held for long
>> > + * periods.
>> > + *
>> > + * Returns -ERRNO if the chip could not be got.
>> > + */
>> > +int tpm_try_get_ops(struct tpm_chip *chip)
>> > +{
>> > +	if (!down_read_trylock(&chip->open_lock))
>> > +		return -EBUSY;
>>
>> Hi Jonathan
>>
>> do I understand it correctly, that a process might open the TPM with
>> O_EXCL, and this will prevent IMA from extending a PCR until that
>> process closes the file descriptor?
>>
>> If yes, this might be a concern, and I think an additional API to
>> prevent such behavior would be needed (for example when IMA is active,
>> i.e. there is a measurement policy loaded).
>
>Also this would be a problem with hwrng.
>
>This probably needs to be refined somehow. I don't have a solution at
>hand but "invariant" is that in-kernel caller should override user space
>exclusion, even when O_EXCL is used.

Kernel access is exactly what caused the issue for me, in particular the 
HW RNG access during a firmware upgrade. My patch to be able to disable 
the HW RNG at runtime has landed in -next, which helps a lot, but it 
really would be nice to be able to say "Hands off, I'm busy with this", 
which is what led to this patch set.

To James' query about the fact the upgrade process should be properly 
handled, I think the issue is probably that the HMAC context saving 
around HW RNG access hit errors that were not gracefully handled, and we 
marked the TPM as disabled in tpm2_load_null, causing failure 
mid-upgrade.

J.

-- 
What have you got in your pocket?

Re: [PATCH v3 4/4] tpm: Allow for exclusive TPM access when using /dev/tpm<n>

[Jarkko Sakkinen]

On Mon, Nov 03, 2025 at 06:38:57PM +0000, Jonathan McDowell wrote:
> On Mon, Oct 27, 2025 at 09:38:55PM +0200, Jarkko Sakkinen wrote:
> > On Mon, Oct 20, 2025 at 01:53:30PM +0200, Roberto Sassu wrote:
> > > On Mon, 2025-10-20 at 12:31 +0100, Jonathan McDowell wrote:
> > > > From: Jonathan McDowell <noodles@meta.com>
> > > >
> > > > There are situations where userspace might reasonably desire exclusive
> > > > access to the TPM, or the kernel's internal context saving + flushing
> > > > may cause issues, for example when performing firmware upgrades. Extend
> > > > the locking already used for avoiding concurrent userspace access to
> > > > prevent internal users of the TPM when /dev/tpm<n> is in use.
> > > >
> > > > The few internal users who already hold the open_lock are changed to use
> > > > tpm_internal_(try_get|put)_ops, with the old tpm_(try_get|put)_ops
> > > > functions changing to obtain read access to the open_lock.  We return
> > > > -EBUSY when another user has exclusive access, rather than adding waits.
> > > >
> > > > Signed-off-by: Jonathan McDowell <noodles@meta.com>
> > > > ---
> > > > v2: Switch to _locked instead of _internal_ for function names.
> > > > v3: Move to end of patch series.
> > > >
> > > >  drivers/char/tpm/tpm-chip.c       | 53 +++++++++++++++++++++++++------
> > > >  drivers/char/tpm/tpm-dev-common.c |  8 ++---
> > > >  drivers/char/tpm/tpm.h            |  2 ++
> > > >  drivers/char/tpm/tpm2-space.c     |  5 ++-
> > > >  4 files changed, 52 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > > > index ba906966721a..687f6d8cd601 100644
> > > > --- a/drivers/char/tpm/tpm-chip.c
> > > > +++ b/drivers/char/tpm/tpm-chip.c
> > > > @@ -144,7 +144,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
> > > >  EXPORT_SYMBOL_GPL(tpm_chip_stop);
> > > >
> > > >  /**
> > > > - * tpm_try_get_ops() - Get a ref to the tpm_chip
> > > > + * tpm_try_get_ops_locked() - Get a ref to the tpm_chip
> > > >   * @chip: Chip to ref
> > > >   *
> > > >   * The caller must already have some kind of locking to ensure that chip is
> > > > @@ -154,7 +154,7 @@ EXPORT_SYMBOL_GPL(tpm_chip_stop);
> > > >   *
> > > >   * Returns -ERRNO if the chip could not be got.
> > > >   */
> > > > -int tpm_try_get_ops(struct tpm_chip *chip)
> > > > +int tpm_try_get_ops_locked(struct tpm_chip *chip)
> > > >  {
> > > >  	int rc = -EIO;
> > > >
> > > > @@ -185,22 +185,57 @@ int tpm_try_get_ops(struct tpm_chip *chip)
> > > >  	put_device(&chip->dev);
> > > >  	return rc;
> > > >  }
> > > > -EXPORT_SYMBOL_GPL(tpm_try_get_ops);
> > > >
> > > >  /**
> > > > - * tpm_put_ops() - Release a ref to the tpm_chip
> > > > + * tpm_put_ops_locked() - Release a ref to the tpm_chip
> > > >   * @chip: Chip to put
> > > >   *
> > > > - * This is the opposite pair to tpm_try_get_ops(). After this returns chip may
> > > > - * be kfree'd.
> > > > + * This is the opposite pair to tpm_try_get_ops_locked(). After this returns
> > > > + * chip may be kfree'd.
> > > >   */
> > > > -void tpm_put_ops(struct tpm_chip *chip)
> > > > +void tpm_put_ops_locked(struct tpm_chip *chip)
> > > >  {
> > > >  	tpm_chip_stop(chip);
> > > >  	mutex_unlock(&chip->tpm_mutex);
> > > >  	up_read(&chip->ops_sem);
> > > >  	put_device(&chip->dev);
> > > >  }
> > > > +
> > > > +/**
> > > > + * tpm_try_get_ops() - Get a ref to the tpm_chip
> > > > + * @chip: Chip to ref
> > > > + *
> > > > + * The caller must already have some kind of locking to ensure that chip is
> > > > + * valid. This function will attempt to get the open_lock for the chip,
> > > > + * ensuring no other user is expecting exclusive access, before locking the
> > > > + * chip so that the ops member can be accessed safely. The locking prevents
> > > > + * tpm_chip_unregister from completing, so it should not be held for long
> > > > + * periods.
> > > > + *
> > > > + * Returns -ERRNO if the chip could not be got.
> > > > + */
> > > > +int tpm_try_get_ops(struct tpm_chip *chip)
> > > > +{
> > > > +	if (!down_read_trylock(&chip->open_lock))
> > > > +		return -EBUSY;
> > > 
> > > Hi Jonathan
> > > 
> > > do I understand it correctly, that a process might open the TPM with
> > > O_EXCL, and this will prevent IMA from extending a PCR until that
> > > process closes the file descriptor?
> > > 
> > > If yes, this might be a concern, and I think an additional API to
> > > prevent such behavior would be needed (for example when IMA is active,
> > > i.e. there is a measurement policy loaded).
> > 
> > Also this would be a problem with hwrng.
> > 
> > This probably needs to be refined somehow. I don't have a solution at
> > hand but "invariant" is that in-kernel caller should override user space
> > exclusion, even when O_EXCL is used.
> 
> Kernel access is exactly what caused the issue for me, in particular the HW
> RNG access during a firmware upgrade. My patch to be able to disable the HW
> RNG at runtime has landed in -next, which helps a lot, but it really would
> be nice to be able to say "Hands off, I'm busy with this", which is what led
> to this patch set.

If there is a situation when kernel needs to be excluded from itself,
then there should really be a kernel uapi to implement that use case.

I'd rather have e.g. ioctl (perhaps just picking one possible tool for
the job) for firmware upgrade than allow user space to arbitarily lock
TPM access.

> 
> To James' query about the fact the upgrade process should be properly
> handled, I think the issue is probably that the HMAC context saving around
> HW RNG access hit errors that were not gracefully handled, and we marked the
> TPM as disabled in tpm2_load_null, causing failure mid-upgrade.
> 
> J.
> 
> -- 
> What have you got in your pocket?

BR, Jarkko