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

[Jarkko Sakkinen <jarkko@kernel.org>]

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