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.0On 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);
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
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?
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
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
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
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.
© 2016 - 2026 Red Hat, Inc.