| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 7 Dec 2017 17:56:17 +0000
From: "Roberts, William C" <william.c.roberts@...el.com>
To: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
"Javier Martinez Canillas" <javierm@...hat.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Peter Huewe <peterhuewe@....de>,
Jerry Snitselaar <jsnitsel@...hat.com>,
Jason Gunthorpe <jgg@...pe.ca>,
"Tricca, Philip B" <philip.b.tricca@...el.com>,
"Jason Gunthorpe" <jgunthorpe@...idianresearch.com>,
"linux-integrity@...r.kernel.org" <linux-integrity@...r.kernel.org>,
"James Bottomley" <James.Bottomley@...senPartnership.com>
Subject: RE: [PATCH] tpm: return a TPM_RC_COMMAND_CODE response if a command
isn't implemented
> -----Original Message-----
> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@...ux.intel.com]
> Sent: Wednesday, December 6, 2017 5:32 PM
> To: Javier Martinez Canillas <javierm@...hat.com>
> Cc: linux-kernel@...r.kernel.org; Peter Huewe <peterhuewe@....de>; Jerry
> Snitselaar <jsnitsel@...hat.com>; Jason Gunthorpe <jgg@...pe.ca>; Tricca,
> Philip B <philip.b.tricca@...el.com>; Jason Gunthorpe
> <jgunthorpe@...idianresearch.com>; linux-integrity@...r.kernel.org; Roberts,
> William C <william.c.roberts@...el.com>; James Bottomley
> <James.Bottomley@...senPartnership.com>
> Subject: Re: [PATCH] tpm: return a TPM_RC_COMMAND_CODE response if a
> command isn't implemented
>
> On Mon, Nov 27, 2017 at 12:30:12AM +0100, Javier Martinez Canillas wrote:
> > According to the TPM Library Specification, a TPM device must do a
> > command header validation before processing and return a
> > TPM_RC_COMMAND_CODE code if the command is not implemented.
> >
> > So user-space will expect to handle that response as an error. But if
> > the in-kernel resource manager is used (/dev/tpmrm?), an -EINVAL errno
> > code is returned instead if the command isn't implemented. This
> > confuses userspace since it doesn't expect that error value.
> >
> > This also isn't consistent with the behavior when not using TPM spaces
> > and accessing the TPM directly (/dev/tpm?). In this case, the command
> > is sent to the TPM even when not implemented and the TPM responds with an
> error.
> >
> > Instead of returning an -EINVAL errno code when the
> > tpm_validate_command() function fails, synthesize a TPM command
> > response so user-space can get a TPM_RC_COMMAND_CODE as expected
> when a chip doesn't implement the command.
> >
> > The TPM only sets 12 of the 32 bits in the TPM_RC response, so the TSS
> > and TAB specifications define that higher layers in the stack should
> > use some of the unused 20 bits to specify from which level of the
> > stack the error is coming from.
> >
> > Since the TPM_RC_COMMAND_CODE response code is sent by the kernel
> > resource manager, set the error level to the TAB/RM layer so
> > user-space is aware of this.
> >
> > Suggested-by: Jason Gunthorpe <jgg@...pe.ca>
> > Signed-off-by: Javier Martinez Canillas <javierm@...hat.com>
> >
> > ---
> >
> > Changes since RFCv2:
> > - Set the error level to the TAB/RM layer so user-space is aware that the error
> > is not coming from the TPM (suggested by Philip Tricca and Jarkko Sakkinen).
> >
> > Changes since RFCv1:
> > - Don't pass not validated commands to the TPM, instead return a synthesized
> > response with the correct TPM return code (suggested by Jason Gunthorpe).
> >
> > And example of user-space getting confused by the TPM chardev
> > returning -EINVAL when sending a not supported TPM command can be seen in
> this tpm2-tools issue:
> >
> > https://github.com/intel/tpm2-tools/issues/621
> >
> > Best regards,
> > Javier
> >
> > drivers/char/tpm/tpm-interface.c | 28 ++++++++++++++++++++--------
> > drivers/char/tpm/tpm.h | 8 ++++++++
> > 2 files changed, 28 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index ebe0a1d36d8c..9391811c5f83 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -328,7 +328,7 @@ unsigned long tpm_calc_ordinal_duration(struct
> > tpm_chip *chip, } EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
> >
> > -static bool tpm_validate_command(struct tpm_chip *chip,
> > +static int tpm_validate_command(struct tpm_chip *chip,
> > struct tpm_space *space,
> > const u8 *cmd,
> > size_t len)
> > @@ -340,10 +340,10 @@ static bool tpm_validate_command(struct tpm_chip
> *chip,
> > unsigned int nr_handles;
> >
> > if (len < TPM_HEADER_SIZE)
> > - return false;
> > + return -EINVAL;
> >
> > if (!space)
> > - return true;
> > + return 0;
> >
> > if (chip->flags & TPM_CHIP_FLAG_TPM2 && chip->nr_commands) {
> > cc = be32_to_cpu(header->ordinal);
> > @@ -352,7 +352,7 @@ static bool tpm_validate_command(struct tpm_chip
> *chip,
> > if (i < 0) {
> > dev_dbg(&chip->dev, "0x%04X is an invalid command\n",
> > cc);
> > - return false;
> > + return -EOPNOTSUPP;
> > }
> >
> > attrs = chip->cc_attrs_tbl[i];
> > @@ -362,11 +362,11 @@ static bool tpm_validate_command(struct tpm_chip
> *chip,
> > goto err_len;
> > }
> >
> > - return true;
> > + return 0;
> > err_len:
> > dev_dbg(&chip->dev,
> > "%s: insufficient command length %zu", __func__, len);
> > - return false;
> > + return -EINVAL;
> > }
> >
> > /**
> > @@ -391,8 +391,20 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct
> tpm_space *space,
> > unsigned long stop;
> > bool need_locality;
> >
> > - if (!tpm_validate_command(chip, space, buf, bufsiz))
> > - return -EINVAL;
> > + rc = tpm_validate_command(chip, space, buf, bufsiz);
> > + if (rc == -EINVAL)
> > + return rc;
> > + /*
> > + * If the command is not implemented by the TPM, synthesize a
> > + * response with a TPM2_RC_COMMAND_CODE return for user-space.
> > + */
> > + if (rc == -EOPNOTSUPP) {
> > + header->length = cpu_to_be32(sizeof(*header));
> > + header->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
> > + header->return_code =
> cpu_to_be32(TPM2_RC_COMMAND_CODE |
> > +
> TPM2_RESMGRTPM_ERROR_LEVEL);
> > + return bufsiz;
> > + }
> >
> > if (bufsiz > TPM_BUFSIZE)
> > bufsiz = TPM_BUFSIZE;
> > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index
> > c1866cc02e30..b3f9108d3d1f 100644
> > --- a/drivers/char/tpm/tpm.h
> > +++ b/drivers/char/tpm/tpm.h
> > @@ -94,12 +94,20 @@ enum tpm2_structures {
> > TPM2_ST_SESSIONS = 0x8002,
> > };
> >
> > +/* Indicates from what level of the software stack the error comes from */
> > +#define TPM2_RC_LEVEL_SHIFT 16
> > +
> > +#define TPM2_RESMGRTPM_ERROR_LEVEL (11 << TPM2_RC_LEVEL_SHIFT)
> > +#define TPM2_RESMGR_ERROR_LEVEL (12 << TPM2_RC_LEVEL_SHIFT)
> > +#define TPM2_DRIVER_ERROR_LEVEL (13 << TPM2_RC_LEVEL_SHIFT)
> > +
> > enum tpm2_return_codes {
> > TPM2_RC_SUCCESS = 0x0000,
> > TPM2_RC_HASH = 0x0083, /* RC_FMT1 */
> > TPM2_RC_HANDLE = 0x008B,
> > TPM2_RC_INITIALIZE = 0x0100, /* RC_VER1 */
> > TPM2_RC_DISABLED = 0x0120,
> > + TPM2_RC_COMMAND_CODE = 0x0143,
> > TPM2_RC_TESTING = 0x090A, /* RC_WARN */
> > TPM2_RC_REFERENCE_H0 = 0x0910,
> > };
> > --
> > 2.14.3
> >
>
> Please use next time --subject-prefix="PATCH v3".
>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
LGTM
Reviewed-by: William Roberts <william.c.roberts@...el.com>
>
> /Jarkko
Powered by blists - more mailing lists