Date:   Fri, 11 Nov 2022 12:04:11 -0800
From:   Evan Green <evgreen@...omium.org>
To:     Jarkko Sakkinen <jarkko@...nel.org>
Cc:     linux-kernel@...r.kernel.org, corbet@....net,
        linux-pm@...r.kernel.org, rjw@...ysocki.net, gwendal@...omium.org,
        apronin@...omium.org, Pavel Machek <pavel@....cz>,
        Kees Cook <keescook@...omium.org>,
        Matthew Garrett <mgarrett@...ora.tech>,
        linux-integrity@...r.kernel.org, jejb@...ux.ibm.com,
        zohar@...ux.ibm.com, dlunev@...gle.com,
        Eric Biggers <ebiggers@...nel.org>,
        Ben Boeckel <me@...boeckel.net>,
        Jason Gunthorpe <jgg@...pe.ca>, Peter Huewe <peterhuewe@....de>
Subject: Re: [PATCH v4 03/11] tpm: Allow PCR 23 to be restricted to
 kernel-only use

On Mon, Nov 7, 2022 at 10:15 AM Evan Green <evgreen@...omium.org> wrote:
>
> On Mon, Nov 7, 2022 at 3:40 AM Jarkko Sakkinen <jarkko@...nel.org> wrote:
> >
> > On Thu, Nov 03, 2022 at 11:01:11AM -0700, Evan Green wrote:
> > > From: Matthew Garrett <matthewgarrett@...gle.com>
> > >
> > > Introduce a new Kconfig, TCG_TPM_RESTRICT_PCR, which if enabled
> > > restricts usermode's ability to extend or reset PCR 23.
> > >
> > > Under certain circumstances it might be desirable to enable the creation
> > > of TPM-backed secrets that are only accessible to the kernel. In an
> > > ideal world this could be achieved by using TPM localities, but these
> > > don't appear to be available on consumer systems. An alternative is to
> > > simply block userland from modifying one of the resettable PCRs, leaving
> > > it available to the kernel. If the kernel ensures that no userland can
> > > access the TPM while it is carrying out work, it can reset PCR 23,
> > > extend it to an arbitrary value, create or load a secret, and then reset
> > > the PCR again. Even if userland somehow obtains the sealed material, it
> > > will be unable to unseal it since PCR 23 will never be in the
> > > appropriate state.
> > >
> > > This Kconfig is only properly supported for systems with TPM2 devices.
> > > For systems with TPM1 devices, having this Kconfig enabled completely
> > > restricts usermode's access to the TPM. TPM1 contains support for
> > > tunnelled transports, which usermode could use to smuggle commands
> > > through that this Kconfig is attempting to restrict.
> > >
> > > Link: https://lore.kernel.org/lkml/20210220013255.1083202-3-matthewgarrett@google.com/
> > > Signed-off-by: Matthew Garrett <mjg59@...gle.com>
> > > Signed-off-by: Evan Green <evgreen@...omium.org>
> > > ---
> > >
> > > Changes in v4:
> > >  - Augment the commit message (Jarkko)
> > >
> > > Changes in v3:
> > >  - Fix up commit message (Jarkko)
> > >  - tpm2_find_and_validate_cc() was split (Jarkko)
> > >  - Simply fully restrict TPM1 since v2 failed to account for tunnelled
> > >    transport sessions (Stefan and Jarkko).
> > >
> > > Changes in v2:
> > >  - Fixed sparse warnings
> > >
> > >  drivers/char/tpm/Kconfig          | 12 ++++++++++++
> > >  drivers/char/tpm/tpm-dev-common.c |  8 ++++++++
> > >  drivers/char/tpm/tpm.h            | 19 +++++++++++++++++++
> > >  drivers/char/tpm/tpm1-cmd.c       | 13 +++++++++++++
> > >  drivers/char/tpm/tpm2-cmd.c       | 22 ++++++++++++++++++++++
> > >  5 files changed, 74 insertions(+)
> > >
> > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > index 927088b2c3d3f2..c8ed54c66e399a 100644
> > > --- a/drivers/char/tpm/Kconfig
> > > +++ b/drivers/char/tpm/Kconfig
> > > @@ -211,4 +211,16 @@ config TCG_FTPM_TEE
> > >         This driver proxies for firmware TPM running in TEE.
> > >
> > >  source "drivers/char/tpm/st33zp24/Kconfig"
> > > +
> > > +config TCG_TPM_RESTRICT_PCR
> > > +     bool "Restrict userland access to PCR 23"
> > > +     depends on TCG_TPM
> > > +     help
> > > +       If set, block userland from extending or resetting PCR 23. This allows it
> > > +       to be restricted to in-kernel use, preventing userland from being able to
> > > +       make use of data sealed to the TPM by the kernel. This is required for
> > > +       secure hibernation support, but should be left disabled if any userland
> > > +       may require access to PCR23. This is a TPM2-only feature, and if enabled
> > > +       on a TPM1 machine will cause all usermode TPM commands to return EPERM due
> > > +       to the complications introduced by tunnelled sessions in TPM1.2.
> > >  endif # TCG_TPM
> > > diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> > > index dc4c0a0a512903..7a4e618c7d1942 100644
> > > --- a/drivers/char/tpm/tpm-dev-common.c
> > > +++ b/drivers/char/tpm/tpm-dev-common.c
> > > @@ -198,6 +198,14 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
> > >       priv->response_read = false;
> > >       *off = 0;
> > >
> > > +     if (priv->chip->flags & TPM_CHIP_FLAG_TPM2)
> > > +             ret = tpm2_cmd_restricted(priv->chip, priv->data_buffer, size);
> > > +     else
> > > +             ret = tpm1_cmd_restricted(priv->chip, priv->data_buffer, size);
> > > +
> > > +     if (ret)
> > > +             goto out;
> > > +
> > >       /*
> > >        * If in nonblocking mode schedule an async job to send
> > >        * the command return the size.
> > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > > index f1e0f490176f01..c0845e3f9eda17 100644
> > > --- a/drivers/char/tpm/tpm.h
> > > +++ b/drivers/char/tpm/tpm.h
> > > @@ -245,4 +245,23 @@ void tpm_bios_log_setup(struct tpm_chip *chip);
> > >  void tpm_bios_log_teardown(struct tpm_chip *chip);
> > >  int tpm_dev_common_init(void);
> > >  void tpm_dev_common_exit(void);
> > > +
> > > +#ifdef CONFIG_TCG_TPM_RESTRICT_PCR
> > > +#define TPM_RESTRICTED_PCR 23
> > > +
> > > +int tpm1_cmd_restricted(struct tpm_chip *chip, u8 *buffer, size_t size);
> > > +int tpm2_cmd_restricted(struct tpm_chip *chip, u8 *buffer, size_t size);
> > > +#else
> > > +static inline int tpm1_cmd_restricted(struct tpm_chip *chip, u8 *buffer,
> > > +                                   size_t size)
> > > +{
> > > +     return 0;
> > > +}
> > > +
> > > +static inline int tpm2_cmd_restricted(struct tpm_chip *chip, u8 *buffer,
> > > +                                   size_t size)
> > > +{
> > > +     return 0;
> > > +}
> > > +#endif
> > >  #endif
> > > diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
> > > index cf64c738510529..1869e89215fcb9 100644
> > > --- a/drivers/char/tpm/tpm1-cmd.c
> > > +++ b/drivers/char/tpm/tpm1-cmd.c
> > > @@ -811,3 +811,16 @@ int tpm1_get_pcr_allocation(struct tpm_chip *chip)
> > >
> > >       return 0;
> > >  }
> > > +
> > > +#ifdef CONFIG_TCG_TPM_RESTRICT_PCR
> > > +int tpm1_cmd_restricted(struct tpm_chip *chip, u8 *buffer, size_t size)
> > > +{
> > > +     /*
> > > +      * Restrict all usermode commands on TPM1.2. Ideally we'd just restrict
> > > +      * TPM_ORD_PCR_EXTEND and TPM_ORD_PCR_RESET, but TPM1.2 also supports
> > > +      * tunnelled transport sessions where the kernel would be unable to filter
> > > +      * commands.
> > > +      */
> > > +     return -EPERM;
> > > +}
> > > +#endif
> > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > > index 303ce2ea02a4b0..e0503cfd7bcfee 100644
> > > --- a/drivers/char/tpm/tpm2-cmd.c
> > > +++ b/drivers/char/tpm/tpm2-cmd.c
> > > @@ -778,3 +778,25 @@ int tpm2_find_cc(struct tpm_chip *chip, u32 cc)
> > >
> > >       return -1;
> > >  }
> > > +
> > > +#ifdef CONFIG_TCG_TPM_RESTRICT_PCR
> > > +int tpm2_cmd_restricted(struct tpm_chip *chip, u8 *buffer, size_t size)
> > > +{
> > > +     int cc = tpm2_find_and_validate_cc(chip, NULL, buffer, size);
> > > +     __be32 *handle;
> > > +
> > > +     switch (cc) {
> > > +     case TPM2_CC_PCR_EXTEND:
> > > +     case TPM2_CC_PCR_RESET:
> > > +             if (size < (TPM_HEADER_SIZE + sizeof(u32)))
> > > +                     return -EINVAL;
> > > +
> > > +             handle = (__be32 *)&buffer[TPM_HEADER_SIZE];
> > > +             if (be32_to_cpu(*handle) == TPM_RESTRICTED_PCR)
> > > +                     return -EPERM;
> > > +             break;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +#endif
> > > --
> > > 2.38.1.431.g37b22c650d-goog
> > >
> >
> > This looks otherwise good but I have still one remark: what is the reason
> > for restricting PCR23 for TPM 1.x?
>
> Mostly I was trying to do the least surprising thing for someone who
> had compiled with this RESTRICT_PCR Kconfig enabled but booted a TPM1
> system. If we do nothing for TPM1, then the encrypted hibernation
> mechanism appears to work fine, but leaves a gaping hole where
> usermode can manipulate PCR23 themselves to create forged encrypted
> hibernate images. Denying all usermode access makes the Kconfig
> correct on TPM1 systems, at the expense of all usermode access (rather
> than just access to PCR23).
>
> An alternative that might be friendlier to users would be to do a
> runtime check in the encrypted hibernate code to simply fail if this
> isn't TPM2. The tradeoff there is that it waters down the Kconfig
> significantly to "RESTRICT_PCR sometimes, if you can, otherwise meh".
> That seemed a bit dangerous, as any future features that may want to
> rely on this Kconfig would have to remember to restrict their support
> to TPM2 as well.

I got talked into revising my stance here, in that breaking usermode
access to TPM1.2 if this Kconfig is set means virtually nobody can
enable this Kconfig. Plus I think doing nothing for TPM1.2 will make
Jarkko happier :). So my new plan is to rename this config to
TCG_TPM2_RESTRICT_PCR, and then try to document very clearly that this
Kconfig only restricts usermode access to the PCR on TPM2.0 devices.
The hibernate code already blocks TPM1.2 devices, so from this series'
perspective the upcoming change should be a no-op.



-Evan

Powered by blists - more mailing lists