diff mbox series

[v2,02/10] tpm: Allow PCR 23 to be restricted to kernel-only use

Message ID 20220823152108.v2.2.I9ded8c8caad27403e9284dfc78ad6cbd845bc98d@changeid
State New
Headers show
Series Encrypted Hibernation | expand

Commit Message

Evan Green Aug. 23, 2022, 10:25 p.m. UTC 
From: Matthew Garrett <matthewgarrett@google.com>

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.

From: Matthew Garrett <mjg59@google.com>
Signed-off-by: Matthew Garrett <mjg59@google.com>

Signed-off-by: Evan Green <evgreen@chromium.org>
---
Matthew's original version of this patch is at:
https://patchwork.kernel.org/patch/12096491/

Changes in v2:
 - Fixed sparse warnings

 drivers/char/tpm/Kconfig          | 10 +++++++++
 drivers/char/tpm/tpm-dev-common.c |  8 +++++++
 drivers/char/tpm/tpm.h            | 21 +++++++++++++++++++
 drivers/char/tpm/tpm1-cmd.c       | 35 +++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm2-cmd.c       | 22 +++++++++++++++++++
 drivers/char/tpm/tpm2-space.c     |  2 +-
 6 files changed, 97 insertions(+), 1 deletion(-)

Comments

Jarkko Sakkinen Aug. 26, 2022, 3:02 a.m. UTC | #1 
On Tue, Aug 23, 2022 at 03:25:18PM -0700, Evan Green wrote:
> From: Matthew Garrett <matthewgarrett@google.com>
> 
> 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.
> 
> From: Matthew Garrett <mjg59@google.com>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>

Same issues as the other patch.

> ---
> Matthew's original version of this patch is at:
> https://patchwork.kernel.org/patch/12096491/

Suggestion: find a lore URL instead for Matthew's patch,
and then add "Link: <lore URL>" to your commit before
sob's. It's a useful reference also in the commit log.

> 
> Changes in v2:
>  - Fixed sparse warnings
> 
>  drivers/char/tpm/Kconfig          | 10 +++++++++
>  drivers/char/tpm/tpm-dev-common.c |  8 +++++++
>  drivers/char/tpm/tpm.h            | 21 +++++++++++++++++++
>  drivers/char/tpm/tpm1-cmd.c       | 35 +++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm2-cmd.c       | 22 +++++++++++++++++++
>  drivers/char/tpm/tpm2-space.c     |  2 +-
>  6 files changed, 97 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index 927088b2c3d3f2..4483b61a428b11 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -211,4 +211,14 @@ 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.
>  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 a80b341d38eb8c..077c3ca0a127ba 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -229,6 +229,8 @@ void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
>  unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
>  int tpm2_probe(struct tpm_chip *chip);
>  int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip);
> +int tpm_find_and_validate_cc(struct tpm_chip *chip, struct tpm_space *space,
> +			     const void *buf, size_t bufsiz);
>  int tpm2_find_cc(struct tpm_chip *chip, u32 cc);
>  int tpm2_init_space(struct tpm_space *space, unsigned int buf_size);
>  void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space);
> @@ -244,4 +246,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 8ec743dec26544..318e75ae42fb85 100644
> --- a/drivers/char/tpm/tpm1-cmd.c
> +++ b/drivers/char/tpm/tpm1-cmd.c
> @@ -845,3 +845,38 @@ 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)
> +{
> +	struct tpm_header *header = (struct tpm_header *)buffer;
> +	char len, offset;
> +	__be32 *pcr;
> +	int pos;
> +
> +	switch (be32_to_cpu(header->ordinal)) {
> +	case TPM_ORD_PCR_EXTEND:
> +		if (size < (TPM_HEADER_SIZE + sizeof(u32)))
> +			return -EINVAL;
> +		pcr = (__be32 *)&buffer[TPM_HEADER_SIZE];
> +		if (be32_to_cpu(*pcr) == TPM_RESTRICTED_PCR)
> +			return -EPERM;
> +		break;
> +	case TPM_ORD_PCR_RESET:
> +		if (size < (TPM_HEADER_SIZE + 1))
> +			return -EINVAL;
> +		len = buffer[TPM_HEADER_SIZE];
> +		if (size < (TPM_HEADER_SIZE + 1 + len))
> +			return -EINVAL;
> +		offset = TPM_RESTRICTED_PCR/3;
> +		if (len < offset)
> +			break;
> +		pos = TPM_HEADER_SIZE + 1 + offset;
> +		if (buffer[pos] & (1 << (TPM_RESTRICTED_PCR - 2 * offset)))
> +			return -EPERM;
> +		break;
> +	}
> +
> +	return 0;
> +}
> +#endif
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 69126a6770386e..dbf7f5552c6782 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -821,3 +821,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 = tpm_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
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index ffb35f0154c16c..6f51cd92c6400f 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -262,7 +262,7 @@ static int tpm2_map_command(struct tpm_chip *chip, u32 cc, u8 *cmd)
>  	return 0;
>  }
>  
> -static int tpm_find_and_validate_cc(struct tpm_chip *chip,
> +int tpm_find_and_validate_cc(struct tpm_chip *chip,
>  				    struct tpm_space *space,
>  				    const void *cmd, size_t len)

Split the export to a separate commit.

>  {
> -- 
> 2.31.0
> 

BR, Jarkko
Stefan Berger Sept. 13, 2022, 12:26 p.m. UTC | #2 
On 8/23/22 18:25, Evan Green wrote:
> From: Matthew Garrett <matthewgarrett@google.com>
> 
> 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.
> 
> From: Matthew Garrett <mjg59@google.com>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
> Matthew's original version of this patch is at:
> https://patchwork.kernel.org/patch/12096491/
> 
> Changes in v2:
>   - Fixed sparse warnings
> 
>   drivers/char/tpm/Kconfig          | 10 +++++++++
>   drivers/char/tpm/tpm-dev-common.c |  8 +++++++
>   drivers/char/tpm/tpm.h            | 21 +++++++++++++++++++
>   drivers/char/tpm/tpm1-cmd.c       | 35 +++++++++++++++++++++++++++++++
>   drivers/char/tpm/tpm2-cmd.c       | 22 +++++++++++++++++++
>   drivers/char/tpm/tpm2-space.c     |  2 +-
>   6 files changed, 97 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index 927088b2c3d3f2..4483b61a428b11 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -211,4 +211,14 @@ 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.
>   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 a80b341d38eb8c..077c3ca0a127ba 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -229,6 +229,8 @@ void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
>   unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
>   int tpm2_probe(struct tpm_chip *chip);
>   int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip);
> +int tpm_find_and_validate_cc(struct tpm_chip *chip, struct tpm_space *space,
> +			     const void *buf, size_t bufsiz);
>   int tpm2_find_cc(struct tpm_chip *chip, u32 cc);
>   int tpm2_init_space(struct tpm_space *space, unsigned int buf_size);
>   void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space);
> @@ -244,4 +246,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 8ec743dec26544..318e75ae42fb85 100644
> --- a/drivers/char/tpm/tpm1-cmd.c
> +++ b/drivers/char/tpm/tpm1-cmd.c
> @@ -845,3 +845,38 @@ 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)
> +{
> +	struct tpm_header *header = (struct tpm_header *)buffer;
> +	char len, offset;
> +	__be32 *pcr;
> +	int pos;
> +
> +	switch (be32_to_cpu(header->ordinal)) {
> +	case TPM_ORD_PCR_EXTEND:
> +		if (size < (TPM_HEADER_SIZE + sizeof(u32)))
> +			return -EINVAL;
> +		pcr = (__be32 *)&buffer[TPM_HEADER_SIZE];
> +		if (be32_to_cpu(*pcr) == TPM_RESTRICTED_PCR)
> +			return -EPERM;

FYI: TPM 1.2 has transport sessions where the command is tunneled in an 
encrypted channel and this check could be circumvented...
Evan Green Sept. 21, 2022, 3:35 p.m. UTC | #3 
On Mon, Sep 19, 2022 at 9:51 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Tue, Sep 13, 2022 at 08:26:09AM -0400, Stefan Berger wrote:
> >
> >
> > On 8/23/22 18:25, Evan Green wrote:
> > > From: Matthew Garrett <matthewgarrett@google.com>
> > >
> > > 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.
> > >
> > > From: Matthew Garrett <mjg59@google.com>
> > > Signed-off-by: Matthew Garrett <mjg59@google.com>
> > >
> > > Signed-off-by: Evan Green <evgreen@chromium.org>
> > > ---
> > > Matthew's original version of this patch is at:
> > > https://patchwork.kernel.org/patch/12096491/
> > >
> > > Changes in v2:
> > >   - Fixed sparse warnings
> > >
> > >   drivers/char/tpm/Kconfig          | 10 +++++++++
> > >   drivers/char/tpm/tpm-dev-common.c |  8 +++++++
> > >   drivers/char/tpm/tpm.h            | 21 +++++++++++++++++++
> > >   drivers/char/tpm/tpm1-cmd.c       | 35 +++++++++++++++++++++++++++++++
> > >   drivers/char/tpm/tpm2-cmd.c       | 22 +++++++++++++++++++
> > >   drivers/char/tpm/tpm2-space.c     |  2 +-
> > >   6 files changed, 97 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > index 927088b2c3d3f2..4483b61a428b11 100644
> > > --- a/drivers/char/tpm/Kconfig
> > > +++ b/drivers/char/tpm/Kconfig
> > > @@ -211,4 +211,14 @@ 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.
> > >   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 a80b341d38eb8c..077c3ca0a127ba 100644
> > > --- a/drivers/char/tpm/tpm.h
> > > +++ b/drivers/char/tpm/tpm.h
> > > @@ -229,6 +229,8 @@ void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
> > >   unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
> > >   int tpm2_probe(struct tpm_chip *chip);
> > >   int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip);
> > > +int tpm_find_and_validate_cc(struct tpm_chip *chip, struct tpm_space *space,
> > > +                        const void *buf, size_t bufsiz);
> > >   int tpm2_find_cc(struct tpm_chip *chip, u32 cc);
> > >   int tpm2_init_space(struct tpm_space *space, unsigned int buf_size);
> > >   void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space);
> > > @@ -244,4 +246,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 8ec743dec26544..318e75ae42fb85 100644
> > > --- a/drivers/char/tpm/tpm1-cmd.c
> > > +++ b/drivers/char/tpm/tpm1-cmd.c
> > > @@ -845,3 +845,38 @@ 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)
> > > +{
> > > +   struct tpm_header *header = (struct tpm_header *)buffer;
> > > +   char len, offset;
> > > +   __be32 *pcr;
> > > +   int pos;
> > > +
> > > +   switch (be32_to_cpu(header->ordinal)) {
> > > +   case TPM_ORD_PCR_EXTEND:
> > > +           if (size < (TPM_HEADER_SIZE + sizeof(u32)))
> > > +                   return -EINVAL;
> > > +           pcr = (__be32 *)&buffer[TPM_HEADER_SIZE];
> > > +           if (be32_to_cpu(*pcr) == TPM_RESTRICTED_PCR)
> > > +                   return -EPERM;
> >
> > FYI: TPM 1.2 has transport sessions where the command is tunneled in an
> > encrypted channel and this check could be circumvented...
>
> BTW, Why do we want to support TPM 1.2 at all.
>
> I would not support it for new features. This could be just TPM2 only
> feeature.

I didn't know about the TPM1.2 tunnelling thing, thanks Stefan. Yes,
maybe in light of that and Jarkko's comment we shouldn't bend over
backwards to make this work on TPM1 and just make it a TPM2-only
feature.

Downstream of this decision, in the other patch, "Add support for
in-kernel resetting of PCRs", my instinct is to keep the addition of
tpm1_pcr_reset() just so the newly introduced generic tpm_pcr_reset()
is fully implemented. Let me know if instead I should also drop the
tpm1 side of that as well, in the name of "don't add stuff you're not
using".
-Evan
Jarkko Sakkinen Sept. 21, 2022, 6:02 p.m. UTC | #4 
On Wed, Sep 21, 2022 at 08:35:35AM -0700, Evan Green wrote:
> On Mon, Sep 19, 2022 at 9:51 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >
> > On Tue, Sep 13, 2022 at 08:26:09AM -0400, Stefan Berger wrote:
> > >
> > >
> > > On 8/23/22 18:25, Evan Green wrote:
> > > > From: Matthew Garrett <matthewgarrett@google.com>
> > > >
> > > > 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.
> > > >
> > > > From: Matthew Garrett <mjg59@google.com>
> > > > Signed-off-by: Matthew Garrett <mjg59@google.com>
> > > >
> > > > Signed-off-by: Evan Green <evgreen@chromium.org>
> > > > ---
> > > > Matthew's original version of this patch is at:
> > > > https://patchwork.kernel.org/patch/12096491/
> > > >
> > > > Changes in v2:
> > > >   - Fixed sparse warnings
> > > >
> > > >   drivers/char/tpm/Kconfig          | 10 +++++++++
> > > >   drivers/char/tpm/tpm-dev-common.c |  8 +++++++
> > > >   drivers/char/tpm/tpm.h            | 21 +++++++++++++++++++
> > > >   drivers/char/tpm/tpm1-cmd.c       | 35 +++++++++++++++++++++++++++++++
> > > >   drivers/char/tpm/tpm2-cmd.c       | 22 +++++++++++++++++++
> > > >   drivers/char/tpm/tpm2-space.c     |  2 +-
> > > >   6 files changed, 97 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > > index 927088b2c3d3f2..4483b61a428b11 100644
> > > > --- a/drivers/char/tpm/Kconfig
> > > > +++ b/drivers/char/tpm/Kconfig
> > > > @@ -211,4 +211,14 @@ 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.
> > > >   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 a80b341d38eb8c..077c3ca0a127ba 100644
> > > > --- a/drivers/char/tpm/tpm.h
> > > > +++ b/drivers/char/tpm/tpm.h
> > > > @@ -229,6 +229,8 @@ void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
> > > >   unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
> > > >   int tpm2_probe(struct tpm_chip *chip);
> > > >   int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip);
> > > > +int tpm_find_and_validate_cc(struct tpm_chip *chip, struct tpm_space *space,
> > > > +                        const void *buf, size_t bufsiz);
> > > >   int tpm2_find_cc(struct tpm_chip *chip, u32 cc);
> > > >   int tpm2_init_space(struct tpm_space *space, unsigned int buf_size);
> > > >   void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space);
> > > > @@ -244,4 +246,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 8ec743dec26544..318e75ae42fb85 100644
> > > > --- a/drivers/char/tpm/tpm1-cmd.c
> > > > +++ b/drivers/char/tpm/tpm1-cmd.c
> > > > @@ -845,3 +845,38 @@ 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)
> > > > +{
> > > > +   struct tpm_header *header = (struct tpm_header *)buffer;
> > > > +   char len, offset;
> > > > +   __be32 *pcr;
> > > > +   int pos;
> > > > +
> > > > +   switch (be32_to_cpu(header->ordinal)) {
> > > > +   case TPM_ORD_PCR_EXTEND:
> > > > +           if (size < (TPM_HEADER_SIZE + sizeof(u32)))
> > > > +                   return -EINVAL;
> > > > +           pcr = (__be32 *)&buffer[TPM_HEADER_SIZE];
> > > > +           if (be32_to_cpu(*pcr) == TPM_RESTRICTED_PCR)
> > > > +                   return -EPERM;
> > >
> > > FYI: TPM 1.2 has transport sessions where the command is tunneled in an
> > > encrypted channel and this check could be circumvented...
> >
> > BTW, Why do we want to support TPM 1.2 at all.
> >
> > I would not support it for new features. This could be just TPM2 only
> > feeature.
> 
> I didn't know about the TPM1.2 tunnelling thing, thanks Stefan. Yes,
> maybe in light of that and Jarkko's comment we shouldn't bend over
> backwards to make this work on TPM1 and just make it a TPM2-only
> feature.
> 
> Downstream of this decision, in the other patch, "Add support for
> in-kernel resetting of PCRs", my instinct is to keep the addition of
> tpm1_pcr_reset() just so the newly introduced generic tpm_pcr_reset()
> is fully implemented. Let me know if instead I should also drop the
> tpm1 side of that as well, in the name of "don't add stuff you're not
> using".
> -Evan

You should drop TPM 1.2 support.

General policy with TPM 1.2:

1. Support legacy.
2. Do no extend the functionality.

BR, Jarkko
diff mbox series

Patch

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 927088b2c3d3f2..4483b61a428b11 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -211,4 +211,14 @@  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.
 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 a80b341d38eb8c..077c3ca0a127ba 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -229,6 +229,8 @@  void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
 unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
 int tpm2_probe(struct tpm_chip *chip);
 int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip);
+int tpm_find_and_validate_cc(struct tpm_chip *chip, struct tpm_space *space,
+			     const void *buf, size_t bufsiz);
 int tpm2_find_cc(struct tpm_chip *chip, u32 cc);
 int tpm2_init_space(struct tpm_space *space, unsigned int buf_size);
 void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space);
@@ -244,4 +246,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 8ec743dec26544..318e75ae42fb85 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -845,3 +845,38 @@  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)
+{
+	struct tpm_header *header = (struct tpm_header *)buffer;
+	char len, offset;
+	__be32 *pcr;
+	int pos;
+
+	switch (be32_to_cpu(header->ordinal)) {
+	case TPM_ORD_PCR_EXTEND:
+		if (size < (TPM_HEADER_SIZE + sizeof(u32)))
+			return -EINVAL;
+		pcr = (__be32 *)&buffer[TPM_HEADER_SIZE];
+		if (be32_to_cpu(*pcr) == TPM_RESTRICTED_PCR)
+			return -EPERM;
+		break;
+	case TPM_ORD_PCR_RESET:
+		if (size < (TPM_HEADER_SIZE + 1))
+			return -EINVAL;
+		len = buffer[TPM_HEADER_SIZE];
+		if (size < (TPM_HEADER_SIZE + 1 + len))
+			return -EINVAL;
+		offset = TPM_RESTRICTED_PCR/3;
+		if (len < offset)
+			break;
+		pos = TPM_HEADER_SIZE + 1 + offset;
+		if (buffer[pos] & (1 << (TPM_RESTRICTED_PCR - 2 * offset)))
+			return -EPERM;
+		break;
+	}
+
+	return 0;
+}
+#endif
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 69126a6770386e..dbf7f5552c6782 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -821,3 +821,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 = tpm_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
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index ffb35f0154c16c..6f51cd92c6400f 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -262,7 +262,7 @@  static int tpm2_map_command(struct tpm_chip *chip, u32 cc, u8 *cmd)
 	return 0;
 }
 
-static int tpm_find_and_validate_cc(struct tpm_chip *chip,
+int tpm_find_and_validate_cc(struct tpm_chip *chip,
 				    struct tpm_space *space,
 				    const void *cmd, size_t len)
 {